| Message ID | 20221213160045.28170-11-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/xenstore: do some cleanup and fixes | expand |
Hi, On 13/12/2022 16:00, Juergen Gross wrote: > Rework the interface and the internals of the per-domain node > accounting: > > - rename the functions to domain_nbentry_*() in order to better match > the related counter name > > - switch from node pointer to domid as interface, as all nodes have the > owner filled in The downside is now you have may place open-coding "...->perms->p[0].id". IHMO this is making the code more complicated. So can you introduce a few wrappers that would take a node and then convert to the owner? > > - use a common internal function for adding a value to the counter > > For the transaction case add a helper function to get the list head > of the per-transaction changed domains, enabling to eliminate the > transaction_entry_*() functions. > > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > tools/xenstore/xenstored_core.c | 22 ++--- > tools/xenstore/xenstored_domain.c | 122 +++++++++++-------------- > tools/xenstore/xenstored_domain.h | 10 +- > tools/xenstore/xenstored_transaction.c | 15 +-- > tools/xenstore/xenstored_transaction.h | 7 +- > 5 files changed, 72 insertions(+), 104 deletions(-) > > diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c > index f96714e1b8..61569cecbb 100644 > --- a/tools/xenstore/xenstored_core.c > +++ b/tools/xenstore/xenstored_core.c > @@ -1459,7 +1459,7 @@ static void destroy_node_rm(struct connection *conn, struct node *node) > static int destroy_node(struct connection *conn, struct node *node) > { > destroy_node_rm(conn, node); > - domain_entry_dec(conn, node); > + domain_nbentry_dec(conn, node->perms.p[0].id); > > /* > * It is not possible to easily revert the changes in a transaction. > @@ -1498,7 +1498,7 @@ static struct node *create_node(struct connection *conn, const void *ctx, > for (i = node; i; i = i->parent) { > /* i->parent is set for each new node, so check quota. */ > if (i->parent && > - domain_entry(conn) >= quota_nb_entry_per_domain) { > + domain_nbentry(conn) >= quota_nb_entry_per_domain) { > ret = ENOSPC; > goto err; > } > @@ -1509,7 +1509,7 @@ static struct node *create_node(struct connection *conn, const void *ctx, > > /* Account for new node */ > if (i->parent) { > - if (domain_entry_inc(conn, i)) { > + if (domain_nbentry_inc(conn, i->perms.p[0].id)) { > destroy_node_rm(conn, i); > return NULL; > } > @@ -1662,7 +1662,7 @@ static int delnode_sub(const void *ctx, struct connection *conn, > watch_exact = strcmp(root, node->name); > fire_watches(conn, ctx, node->name, node, watch_exact, NULL); > > - domain_entry_dec(conn, node); > + domain_nbentry_dec(conn, node->perms.p[0].id); > > return WALK_TREE_RM_CHILDENTRY; > } > @@ -1802,25 +1802,25 @@ static int do_set_perms(const void *ctx, struct connection *conn, > return EPERM; > > old_perms = node->perms; > - domain_entry_dec(conn, node); > + domain_nbentry_dec(conn, node->perms.p[0].id); > node->perms = perms; > - if (domain_entry_inc(conn, node)) { > + if (domain_nbentry_inc(conn, node->perms.p[0].id)) { > node->perms = old_perms; > /* > * This should never fail because we had a reference on the > * domain before and Xenstored is single-threaded. > */ > - domain_entry_inc(conn, node); > + domain_nbentry_inc(conn, node->perms.p[0].id); > return ENOMEM; > } > > if (write_node(conn, node, false)) { > int saved_errno = errno; > > - domain_entry_dec(conn, node); > + domain_nbentry_dec(conn, node->perms.p[0].id); > node->perms = old_perms; > /* No failure possible as above. */ > - domain_entry_inc(conn, node); > + domain_nbentry_inc(conn, node->perms.p[0].id); > > errno = saved_errno; > return errno; > @@ -2392,7 +2392,7 @@ void setup_structure(bool live_update) > manual_node("/tool/xenstored", NULL); > manual_node("@releaseDomain", NULL); > manual_node("@introduceDomain", NULL); > - domain_entry_fix(dom0_domid, 5, true); > + domain_nbentry_fix(dom0_domid, 5, true); > } > > check_store(); > @@ -3400,7 +3400,7 @@ void read_state_node(const void *ctx, const void *state) > if (write_node_raw(NULL, &key, node, true)) > barf("write node error restoring node"); > > - if (domain_entry_inc(&conn, node)) > + if (domain_nbentry_inc(&conn, node->perms.p[0].id)) > barf("node accounting error restoring node"); > > talloc_free(node); > diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c > index 3216119e83..40b24056c5 100644 > --- a/tools/xenstore/xenstored_domain.c > +++ b/tools/xenstore/xenstored_domain.c > @@ -249,7 +249,7 @@ static int domain_tree_remove_sub(const void *ctx, struct connection *conn, > domain->nbentry--; > node->perms.p[0].id = priv_domid; > node->acc.memory = 0; > - domain_entry_inc(NULL, node); > + domain_nbentry_inc(NULL, priv_domid); > if (write_node_raw(NULL, &key, node, true)) { > /* That's unfortunate. We only can try to continue. */ > syslog(LOG_ERR, > @@ -559,7 +559,7 @@ int acc_fix_domains(struct list_head *head, bool update) > int cnt; > > list_for_each_entry(cd, head, list) { > - cnt = domain_entry_fix(cd->domid, cd->nbentry, update); > + cnt = domain_nbentry_fix(cd->domid, cd->nbentry, update); > if (!update) { > if (cnt >= quota_nb_entry_per_domain) > return ENOSPC; > @@ -604,18 +604,19 @@ static struct changed_domain *acc_get_changed_domain(const void *ctx, > return cd; > } > > -int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val, > - unsigned int domid) > +static int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val, > + unsigned int domid) > { > struct changed_domain *cd; > > cd = acc_get_changed_domain(ctx, head, domid); > if (!cd) > - return errno; > + return 0; > > + errno = 0; > cd->nbentry += val; > > - return 0; > + return cd->nbentry; You just introduced this helper in the previous patch (i.e. #9). So can you get the interface correct from the start? This will make easier to review the series. I don't mind too much if you add the static here. Although, it would have been nice if we avoid changing code just introduced. > } > > static void domain_conn_reset(struct domain *domain) > @@ -988,30 +989,6 @@ void domain_deinit(void) > xenevtchn_unbind(xce_handle, virq_port); > } > > -int domain_entry_inc(struct connection *conn, struct node *node) > -{ > - struct domain *d; > - unsigned int domid; > - > - if (!node->perms.p) > - return 0; > - > - domid = node->perms.p[0].id; > - > - if (conn && conn->transaction) { > - transaction_entry_inc(conn->transaction, domid); > - } else { > - d = (conn && domid == conn->id && conn->domain) ? conn->domain > - : find_or_alloc_existing_domain(domid); > - if (d) > - d->nbentry++; > - else > - return ENOMEM; > - } > - > - return 0; > -} > - > /* > * Check whether a domain was created before or after a specific generation > * count (used for testing whether a node permission is older than a domain). > @@ -1079,62 +1056,67 @@ int domain_adjust_node_perms(struct node *node) > return 0; > } > > -void domain_entry_dec(struct connection *conn, struct node *node) > +static int domain_nbentry_add(struct connection *conn, unsigned int domid, > + int add, bool dom_exists) The name of the variable suggests that that if it is false then it doesn't exists. However, looking at how you use it, it is more a "Can struct domain be allocated?". So I would rename it to "dom_alloc_allowed" or similar. > { > struct domain *d; > - unsigned int domid; > - > - if (!node->perms.p) > - return; > + struct list_head *head; > + int ret; > > - domid = node->perms.p ? node->perms.p[0].id : conn->id; > + if (conn && domid == conn->id && conn->domain) > + d = conn->domain; > + else if (dom_exists) { > + d = find_domain_struct(domid); > + if (!d) { > + errno = ENOENT; > + corrupt(conn, "Missing domain %u\n", domid); > + return -1; > + } > + } else { > + d = find_or_alloc_existing_domain(domid); > + if (!d) { > + errno = ENOMEM; > + return -1; > + } > + } > > if (conn && conn->transaction) { > - transaction_entry_dec(conn->transaction, domid); > - } else { > - d = (conn && domid == conn->id && conn->domain) ? conn->domain > - : find_domain_struct(domid); > - if (d) { > - d->nbentry--; > - } else { > - errno = ENOENT; > - corrupt(conn, > - "Node \"%s\" owned by non-existing domain %u\n", > - node->name, domid); > + head = transaction_get_changed_domains(conn->transaction); > + ret = acc_add_dom_nbentry(conn->transaction, head, add, domid); > + if (errno) { > + fail_transaction(conn->transaction); > + return -1; > } > + return d->nbentry + ret; It is not entirely clear why you are return "d->nbentry + ret" here. If it is ... > } > + > + d->nbentry += add; > + > + return d->nbentry; > } > > -int domain_entry_fix(unsigned int domid, int num, bool update) > +int domain_nbentry_inc(struct connection *conn, unsigned int domid) > { > - struct domain *d; > - int cnt; > + return (domain_nbentry_add(conn, domid, 1, false) < 0) ? errno : 0; > +} > > - if (update) { > - d = find_domain_struct(domid); > - assert(d); > - } else { > - /* > - * We are called first with update == false in order to catch > - * any error. So do a possible allocation and check for error > - * only in this case, as in the case of update == true nothing > - * can go wrong anymore as the allocation already happened. > - */ > - d = find_or_alloc_existing_domain(domid); > - if (!d) > - return -1; > - } > +int domain_nbentry_dec(struct connection *conn, unsigned int domid) > +{ > + return (domain_nbentry_add(conn, domid, -1, true) < 0) ? errno : 0; ... to make sure domain_nbentry_add() is not returning a negative value. Then it would not work. A good example imagine you have a transaction removing nodes from tree but not adding any. Then the "ret" would be negative. Meanwhile the nodes are also removed outside of the transaction. So the sum of "d->nbentry + ret" would be negative resulting to a failure here. Such change of behavior should pointed in the commit message. But then I am not convinced this should be part of this commit which is mainly reworking an interface (e.g. no functional change is expected). Cheers,
On 20.12.22 21:15, Julien Grall wrote: > Hi, > > On 13/12/2022 16:00, Juergen Gross wrote: >> Rework the interface and the internals of the per-domain node >> accounting: >> >> - rename the functions to domain_nbentry_*() in order to better match >> the related counter name >> >> - switch from node pointer to domid as interface, as all nodes have the >> owner filled in > > The downside is now you have may place open-coding "...->perms->p[0].id". IHMO > this is making the code more complicated. So can you introduce a few wrappers > that would take a node and then convert to the owner? Okay. > >> >> - use a common internal function for adding a value to the counter >> >> For the transaction case add a helper function to get the list head >> of the per-transaction changed domains, enabling to eliminate the >> transaction_entry_*() functions. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> tools/xenstore/xenstored_core.c | 22 ++--- >> tools/xenstore/xenstored_domain.c | 122 +++++++++++-------------- >> tools/xenstore/xenstored_domain.h | 10 +- >> tools/xenstore/xenstored_transaction.c | 15 +-- >> tools/xenstore/xenstored_transaction.h | 7 +- >> 5 files changed, 72 insertions(+), 104 deletions(-) >> >> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c >> index f96714e1b8..61569cecbb 100644 >> --- a/tools/xenstore/xenstored_core.c >> +++ b/tools/xenstore/xenstored_core.c >> @@ -1459,7 +1459,7 @@ static void destroy_node_rm(struct connection *conn, >> struct node *node) >> static int destroy_node(struct connection *conn, struct node *node) >> { >> destroy_node_rm(conn, node); >> - domain_entry_dec(conn, node); >> + domain_nbentry_dec(conn, node->perms.p[0].id); >> /* >> * It is not possible to easily revert the changes in a transaction. >> @@ -1498,7 +1498,7 @@ static struct node *create_node(struct connection *conn, >> const void *ctx, >> for (i = node; i; i = i->parent) { >> /* i->parent is set for each new node, so check quota. */ >> if (i->parent && >> - domain_entry(conn) >= quota_nb_entry_per_domain) { >> + domain_nbentry(conn) >= quota_nb_entry_per_domain) { >> ret = ENOSPC; >> goto err; >> } >> @@ -1509,7 +1509,7 @@ static struct node *create_node(struct connection *conn, >> const void *ctx, >> /* Account for new node */ >> if (i->parent) { >> - if (domain_entry_inc(conn, i)) { >> + if (domain_nbentry_inc(conn, i->perms.p[0].id)) { >> destroy_node_rm(conn, i); >> return NULL; >> } >> @@ -1662,7 +1662,7 @@ static int delnode_sub(const void *ctx, struct >> connection *conn, >> watch_exact = strcmp(root, node->name); >> fire_watches(conn, ctx, node->name, node, watch_exact, NULL); >> - domain_entry_dec(conn, node); >> + domain_nbentry_dec(conn, node->perms.p[0].id); >> return WALK_TREE_RM_CHILDENTRY; >> } >> @@ -1802,25 +1802,25 @@ static int do_set_perms(const void *ctx, struct >> connection *conn, >> return EPERM; >> old_perms = node->perms; >> - domain_entry_dec(conn, node); >> + domain_nbentry_dec(conn, node->perms.p[0].id); >> node->perms = perms; >> - if (domain_entry_inc(conn, node)) { >> + if (domain_nbentry_inc(conn, node->perms.p[0].id)) { >> node->perms = old_perms; >> /* >> * This should never fail because we had a reference on the >> * domain before and Xenstored is single-threaded. >> */ >> - domain_entry_inc(conn, node); >> + domain_nbentry_inc(conn, node->perms.p[0].id); >> return ENOMEM; >> } >> if (write_node(conn, node, false)) { >> int saved_errno = errno; >> - domain_entry_dec(conn, node); >> + domain_nbentry_dec(conn, node->perms.p[0].id); >> node->perms = old_perms; >> /* No failure possible as above. */ >> - domain_entry_inc(conn, node); >> + domain_nbentry_inc(conn, node->perms.p[0].id); >> errno = saved_errno; >> return errno; >> @@ -2392,7 +2392,7 @@ void setup_structure(bool live_update) >> manual_node("/tool/xenstored", NULL); >> manual_node("@releaseDomain", NULL); >> manual_node("@introduceDomain", NULL); >> - domain_entry_fix(dom0_domid, 5, true); >> + domain_nbentry_fix(dom0_domid, 5, true); >> } >> check_store(); >> @@ -3400,7 +3400,7 @@ void read_state_node(const void *ctx, const void *state) >> if (write_node_raw(NULL, &key, node, true)) >> barf("write node error restoring node"); >> - if (domain_entry_inc(&conn, node)) >> + if (domain_nbentry_inc(&conn, node->perms.p[0].id)) >> barf("node accounting error restoring node"); >> talloc_free(node); >> diff --git a/tools/xenstore/xenstored_domain.c >> b/tools/xenstore/xenstored_domain.c >> index 3216119e83..40b24056c5 100644 >> --- a/tools/xenstore/xenstored_domain.c >> +++ b/tools/xenstore/xenstored_domain.c >> @@ -249,7 +249,7 @@ static int domain_tree_remove_sub(const void *ctx, struct >> connection *conn, >> domain->nbentry--; >> node->perms.p[0].id = priv_domid; >> node->acc.memory = 0; >> - domain_entry_inc(NULL, node); >> + domain_nbentry_inc(NULL, priv_domid); >> if (write_node_raw(NULL, &key, node, true)) { >> /* That's unfortunate. We only can try to continue. */ >> syslog(LOG_ERR, >> @@ -559,7 +559,7 @@ int acc_fix_domains(struct list_head *head, bool update) >> int cnt; >> list_for_each_entry(cd, head, list) { >> - cnt = domain_entry_fix(cd->domid, cd->nbentry, update); >> + cnt = domain_nbentry_fix(cd->domid, cd->nbentry, update); >> if (!update) { >> if (cnt >= quota_nb_entry_per_domain) >> return ENOSPC; >> @@ -604,18 +604,19 @@ static struct changed_domain >> *acc_get_changed_domain(const void *ctx, >> return cd; >> } >> -int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val, >> - unsigned int domid) >> +static int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val, >> + unsigned int domid) >> { >> struct changed_domain *cd; >> cd = acc_get_changed_domain(ctx, head, domid); >> if (!cd) >> - return errno; >> + return 0; >> + errno = 0; >> cd->nbentry += val; >> - return 0; >> + return cd->nbentry; > > You just introduced this helper in the previous patch (i.e. #9). So can you get > the interface correct from the start? This will make easier to review the series. > > I don't mind too much if you add the static here. Although, it would have been > nice if we avoid changing code just introduced. Fine with me. > >> } >> static void domain_conn_reset(struct domain *domain) >> @@ -988,30 +989,6 @@ void domain_deinit(void) >> xenevtchn_unbind(xce_handle, virq_port); >> } >> -int domain_entry_inc(struct connection *conn, struct node *node) >> -{ >> - struct domain *d; >> - unsigned int domid; >> - >> - if (!node->perms.p) >> - return 0; >> - >> - domid = node->perms.p[0].id; >> - >> - if (conn && conn->transaction) { >> - transaction_entry_inc(conn->transaction, domid); >> - } else { >> - d = (conn && domid == conn->id && conn->domain) ? conn->domain >> - : find_or_alloc_existing_domain(domid); >> - if (d) >> - d->nbentry++; >> - else >> - return ENOMEM; >> - } >> - >> - return 0; >> -} >> - >> /* >> * Check whether a domain was created before or after a specific generation >> * count (used for testing whether a node permission is older than a domain). >> @@ -1079,62 +1056,67 @@ int domain_adjust_node_perms(struct node *node) >> return 0; >> } >> -void domain_entry_dec(struct connection *conn, struct node *node) >> +static int domain_nbentry_add(struct connection *conn, unsigned int domid, >> + int add, bool dom_exists) > > The name of the variable suggests that that if it is false then it doesn't > exists. However, looking at how you use it, it is more a "Can struct domain be > allocated?". So I would rename it to "dom_alloc_allowed" or similar. I'll name it "no_dom_alloc". > >> { >> struct domain *d; >> - unsigned int domid; >> - >> - if (!node->perms.p) >> - return; >> + struct list_head *head; >> + int ret; >> - domid = node->perms.p ? node->perms.p[0].id : conn->id; >> + if (conn && domid == conn->id && conn->domain) >> + d = conn->domain; >> + else if (dom_exists) { >> + d = find_domain_struct(domid); >> + if (!d) { >> + errno = ENOENT; >> + corrupt(conn, "Missing domain %u\n", domid); >> + return -1; >> + } >> + } else { >> + d = find_or_alloc_existing_domain(domid); >> + if (!d) { >> + errno = ENOMEM; >> + return -1; >> + } >> + } >> if (conn && conn->transaction) { >> - transaction_entry_dec(conn->transaction, domid); >> - } else { >> - d = (conn && domid == conn->id && conn->domain) ? conn->domain >> - : find_domain_struct(domid); >> - if (d) { >> - d->nbentry--; >> - } else { >> - errno = ENOENT; >> - corrupt(conn, >> - "Node \"%s\" owned by non-existing domain %u\n", >> - node->name, domid); >> + head = transaction_get_changed_domains(conn->transaction); >> + ret = acc_add_dom_nbentry(conn->transaction, head, add, domid); >> + if (errno) { >> + fail_transaction(conn->transaction); >> + return -1; >> } >> + return d->nbentry + ret; > > It is not entirely clear why you are return "d->nbentry + ret" here. If it is ... > >> } >> + >> + d->nbentry += add; >> + >> + return d->nbentry; >> } >> -int domain_entry_fix(unsigned int domid, int num, bool update) >> +int domain_nbentry_inc(struct connection *conn, unsigned int domid) >> { >> - struct domain *d; >> - int cnt; >> + return (domain_nbentry_add(conn, domid, 1, false) < 0) ? errno : 0; >> +} >> - if (update) { >> - d = find_domain_struct(domid); >> - assert(d); >> - } else { >> - /* >> - * We are called first with update == false in order to catch >> - * any error. So do a possible allocation and check for error >> - * only in this case, as in the case of update == true nothing >> - * can go wrong anymore as the allocation already happened. >> - */ >> - d = find_or_alloc_existing_domain(domid); >> - if (!d) >> - return -1; >> - } >> +int domain_nbentry_dec(struct connection *conn, unsigned int domid) >> +{ >> + return (domain_nbentry_add(conn, domid, -1, true) < 0) ? errno : 0; > > ... to make sure domain_nbentry_add() is not returning a negative value. Then it > would not work. > > A good example imagine you have a transaction removing nodes from tree but not > adding any. Then the "ret" would be negative. > > Meanwhile the nodes are also removed outside of the transaction. So the sum of > "d->nbentry + ret" would be negative resulting to a failure here. Thanks for catching this. I think the correct way to handle this is to return max(d->nbentry + ret, 0) in domain_nbentry_add(). The value might be imprecise, but always >= 0 and never wrong outside of a transaction collision. > > Such change of behavior should pointed in the commit message. But then I am not > convinced this should be part of this commit which is mainly reworking an > interface (e.g. no functional change is expected). Juergen
Hi Juergen, On 11/01/2023 08:59, Juergen Gross wrote: >> ... to make sure domain_nbentry_add() is not returning a negative >> value. Then it would not work. >> >> A good example imagine you have a transaction removing nodes from tree >> but not adding any. Then the "ret" would be negative. >> >> Meanwhile the nodes are also removed outside of the transaction. So >> the sum of "d->nbentry + ret" would be negative resulting to a failure >> here. > > Thanks for catching this. > > I think the correct way to handle this is to return max(d->nbentry + > ret, 0) in > domain_nbentry_add(). The value might be imprecise, but always >= 0 and > never > wrong outside of a transaction collision. I am bit confused with your proposal. If the return value is imprecise, then what's the point of returning max(...) instead of simply 0? Cheers,
On 11.01.23 18:48, Julien Grall wrote: > Hi Juergen, > > On 11/01/2023 08:59, Juergen Gross wrote: >>> ... to make sure domain_nbentry_add() is not returning a negative value. Then >>> it would not work. >>> >>> A good example imagine you have a transaction removing nodes from tree but >>> not adding any. Then the "ret" would be negative. >>> >>> Meanwhile the nodes are also removed outside of the transaction. So the sum >>> of "d->nbentry + ret" would be negative resulting to a failure here. >> >> Thanks for catching this. >> >> I think the correct way to handle this is to return max(d->nbentry + ret, 0) in >> domain_nbentry_add(). The value might be imprecise, but always >= 0 and never >> wrong outside of a transaction collision. > > I am bit confused with your proposal. If the return value is imprecise, then > what's the point of returning max(...) instead of simply 0? Please have a look at the use case especially in domain_nbentry(). Returning always 0 would clearly break quota checks. Juergen
Hi Juergen, On 12/01/2023 05:49, Juergen Gross wrote: > On 11.01.23 18:48, Julien Grall wrote: >> Hi Juergen, >> >> On 11/01/2023 08:59, Juergen Gross wrote: >>>> ... to make sure domain_nbentry_add() is not returning a negative >>>> value. Then it would not work. >>>> >>>> A good example imagine you have a transaction removing nodes from >>>> tree but not adding any. Then the "ret" would be negative. >>>> >>>> Meanwhile the nodes are also removed outside of the transaction. So >>>> the sum of "d->nbentry + ret" would be negative resulting to a >>>> failure here. >>> >>> Thanks for catching this. >>> >>> I think the correct way to handle this is to return max(d->nbentry + >>> ret, 0) in >>> domain_nbentry_add(). The value might be imprecise, but always >= 0 >>> and never >>> wrong outside of a transaction collision. >> >> I am bit confused with your proposal. If the return value is >> imprecise, then what's the point of returning max(...) instead of >> simply 0? > > Please have a look at the use case especially in domain_nbentry(). > Returning > always 0 would clearly break quota checks. I am a bit concerned that we would have a code checking the quota based on an imprecise value. At the moment, I don't have a better suggestion. But we should at least document in the code when we think the value is imprecise and explain why bypassing the quota check is OK (IOW who will check it?). Cheers,
On 13.01.23 10:53, Julien Grall wrote: > Hi Juergen, > > On 12/01/2023 05:49, Juergen Gross wrote: >> On 11.01.23 18:48, Julien Grall wrote: >>> Hi Juergen, >>> >>> On 11/01/2023 08:59, Juergen Gross wrote: >>>>> ... to make sure domain_nbentry_add() is not returning a negative value. >>>>> Then it would not work. >>>>> >>>>> A good example imagine you have a transaction removing nodes from tree but >>>>> not adding any. Then the "ret" would be negative. >>>>> >>>>> Meanwhile the nodes are also removed outside of the transaction. So the sum >>>>> of "d->nbentry + ret" would be negative resulting to a failure here. >>>> >>>> Thanks for catching this. >>>> >>>> I think the correct way to handle this is to return max(d->nbentry + ret, 0) in >>>> domain_nbentry_add(). The value might be imprecise, but always >= 0 and never >>>> wrong outside of a transaction collision. >>> >>> I am bit confused with your proposal. If the return value is imprecise, then >>> what's the point of returning max(...) instead of simply 0? >> >> Please have a look at the use case especially in domain_nbentry(). Returning >> always 0 would clearly break quota checks. > > I am a bit concerned that we would have a code checking the quota based on an > imprecise value. > > At the moment, I don't have a better suggestion. But we should at least document > in the code when we think the value is imprecise and explain why bypassing the > quota check is OK (IOW who will check it?). The imprecise value will never be too low, it can only be too high (i.e. 0 instead of negative), and that will only happen in a transaction which can't succeed. Adding a comment is good idea, though. Juergen
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c index f96714e1b8..61569cecbb 100644 --- a/tools/xenstore/xenstored_core.c +++ b/tools/xenstore/xenstored_core.c @@ -1459,7 +1459,7 @@ static void destroy_node_rm(struct connection *conn, struct node *node) static int destroy_node(struct connection *conn, struct node *node) { destroy_node_rm(conn, node); - domain_entry_dec(conn, node); + domain_nbentry_dec(conn, node->perms.p[0].id); /* * It is not possible to easily revert the changes in a transaction. @@ -1498,7 +1498,7 @@ static struct node *create_node(struct connection *conn, const void *ctx, for (i = node; i; i = i->parent) { /* i->parent is set for each new node, so check quota. */ if (i->parent && - domain_entry(conn) >= quota_nb_entry_per_domain) { + domain_nbentry(conn) >= quota_nb_entry_per_domain) { ret = ENOSPC; goto err; } @@ -1509,7 +1509,7 @@ static struct node *create_node(struct connection *conn, const void *ctx, /* Account for new node */ if (i->parent) { - if (domain_entry_inc(conn, i)) { + if (domain_nbentry_inc(conn, i->perms.p[0].id)) { destroy_node_rm(conn, i); return NULL; } @@ -1662,7 +1662,7 @@ static int delnode_sub(const void *ctx, struct connection *conn, watch_exact = strcmp(root, node->name); fire_watches(conn, ctx, node->name, node, watch_exact, NULL); - domain_entry_dec(conn, node); + domain_nbentry_dec(conn, node->perms.p[0].id); return WALK_TREE_RM_CHILDENTRY; } @@ -1802,25 +1802,25 @@ static int do_set_perms(const void *ctx, struct connection *conn, return EPERM; old_perms = node->perms; - domain_entry_dec(conn, node); + domain_nbentry_dec(conn, node->perms.p[0].id); node->perms = perms; - if (domain_entry_inc(conn, node)) { + if (domain_nbentry_inc(conn, node->perms.p[0].id)) { node->perms = old_perms; /* * This should never fail because we had a reference on the * domain before and Xenstored is single-threaded. */ - domain_entry_inc(conn, node); + domain_nbentry_inc(conn, node->perms.p[0].id); return ENOMEM; } if (write_node(conn, node, false)) { int saved_errno = errno; - domain_entry_dec(conn, node); + domain_nbentry_dec(conn, node->perms.p[0].id); node->perms = old_perms; /* No failure possible as above. */ - domain_entry_inc(conn, node); + domain_nbentry_inc(conn, node->perms.p[0].id); errno = saved_errno; return errno; @@ -2392,7 +2392,7 @@ void setup_structure(bool live_update) manual_node("/tool/xenstored", NULL); manual_node("@releaseDomain", NULL); manual_node("@introduceDomain", NULL); - domain_entry_fix(dom0_domid, 5, true); + domain_nbentry_fix(dom0_domid, 5, true); } check_store(); @@ -3400,7 +3400,7 @@ void read_state_node(const void *ctx, const void *state) if (write_node_raw(NULL, &key, node, true)) barf("write node error restoring node"); - if (domain_entry_inc(&conn, node)) + if (domain_nbentry_inc(&conn, node->perms.p[0].id)) barf("node accounting error restoring node"); talloc_free(node); diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c index 3216119e83..40b24056c5 100644 --- a/tools/xenstore/xenstored_domain.c +++ b/tools/xenstore/xenstored_domain.c @@ -249,7 +249,7 @@ static int domain_tree_remove_sub(const void *ctx, struct connection *conn, domain->nbentry--; node->perms.p[0].id = priv_domid; node->acc.memory = 0; - domain_entry_inc(NULL, node); + domain_nbentry_inc(NULL, priv_domid); if (write_node_raw(NULL, &key, node, true)) { /* That's unfortunate. We only can try to continue. */ syslog(LOG_ERR, @@ -559,7 +559,7 @@ int acc_fix_domains(struct list_head *head, bool update) int cnt; list_for_each_entry(cd, head, list) { - cnt = domain_entry_fix(cd->domid, cd->nbentry, update); + cnt = domain_nbentry_fix(cd->domid, cd->nbentry, update); if (!update) { if (cnt >= quota_nb_entry_per_domain) return ENOSPC; @@ -604,18 +604,19 @@ static struct changed_domain *acc_get_changed_domain(const void *ctx, return cd; } -int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val, - unsigned int domid) +static int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val, + unsigned int domid) { struct changed_domain *cd; cd = acc_get_changed_domain(ctx, head, domid); if (!cd) - return errno; + return 0; + errno = 0; cd->nbentry += val; - return 0; + return cd->nbentry; } static void domain_conn_reset(struct domain *domain) @@ -988,30 +989,6 @@ void domain_deinit(void) xenevtchn_unbind(xce_handle, virq_port); } -int domain_entry_inc(struct connection *conn, struct node *node) -{ - struct domain *d; - unsigned int domid; - - if (!node->perms.p) - return 0; - - domid = node->perms.p[0].id; - - if (conn && conn->transaction) { - transaction_entry_inc(conn->transaction, domid); - } else { - d = (conn && domid == conn->id && conn->domain) ? conn->domain - : find_or_alloc_existing_domain(domid); - if (d) - d->nbentry++; - else - return ENOMEM; - } - - return 0; -} - /* * Check whether a domain was created before or after a specific generation * count (used for testing whether a node permission is older than a domain). @@ -1079,62 +1056,67 @@ int domain_adjust_node_perms(struct node *node) return 0; } -void domain_entry_dec(struct connection *conn, struct node *node) +static int domain_nbentry_add(struct connection *conn, unsigned int domid, + int add, bool dom_exists) { struct domain *d; - unsigned int domid; - - if (!node->perms.p) - return; + struct list_head *head; + int ret; - domid = node->perms.p ? node->perms.p[0].id : conn->id; + if (conn && domid == conn->id && conn->domain) + d = conn->domain; + else if (dom_exists) { + d = find_domain_struct(domid); + if (!d) { + errno = ENOENT; + corrupt(conn, "Missing domain %u\n", domid); + return -1; + } + } else { + d = find_or_alloc_existing_domain(domid); + if (!d) { + errno = ENOMEM; + return -1; + } + } if (conn && conn->transaction) { - transaction_entry_dec(conn->transaction, domid); - } else { - d = (conn && domid == conn->id && conn->domain) ? conn->domain - : find_domain_struct(domid); - if (d) { - d->nbentry--; - } else { - errno = ENOENT; - corrupt(conn, - "Node \"%s\" owned by non-existing domain %u\n", - node->name, domid); + head = transaction_get_changed_domains(conn->transaction); + ret = acc_add_dom_nbentry(conn->transaction, head, add, domid); + if (errno) { + fail_transaction(conn->transaction); + return -1; } + return d->nbentry + ret; } + + d->nbentry += add; + + return d->nbentry; } -int domain_entry_fix(unsigned int domid, int num, bool update) +int domain_nbentry_inc(struct connection *conn, unsigned int domid) { - struct domain *d; - int cnt; + return (domain_nbentry_add(conn, domid, 1, false) < 0) ? errno : 0; +} - if (update) { - d = find_domain_struct(domid); - assert(d); - } else { - /* - * We are called first with update == false in order to catch - * any error. So do a possible allocation and check for error - * only in this case, as in the case of update == true nothing - * can go wrong anymore as the allocation already happened. - */ - d = find_or_alloc_existing_domain(domid); - if (!d) - return -1; - } +int domain_nbentry_dec(struct connection *conn, unsigned int domid) +{ + return (domain_nbentry_add(conn, domid, -1, true) < 0) ? errno : 0; +} - cnt = d->nbentry + num; - assert(cnt >= 0); +int domain_nbentry_fix(unsigned int domid, int num, bool update) +{ + int ret; - if (update) - d->nbentry = cnt; + ret = domain_nbentry_add(NULL, domid, update ? num : 0, update); + if (ret < 0 || update) + return ret; - return domid_is_unprivileged(domid) ? cnt : 0; + return domid_is_unprivileged(domid) ? ret + num : 0; } -int domain_entry(struct connection *conn) +int domain_nbentry(struct connection *conn) { return (domain_is_unprivileged(conn)) ? conn->domain->nbentry diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h index 9e20d2b17d..1e402f2609 100644 --- a/tools/xenstore/xenstored_domain.h +++ b/tools/xenstore/xenstored_domain.h @@ -66,10 +66,10 @@ int domain_adjust_node_perms(struct node *node); int domain_alloc_permrefs(struct node_perms *perms); /* Quota manipulation */ -int domain_entry_inc(struct connection *conn, struct node *); -void domain_entry_dec(struct connection *conn, struct node *); -int domain_entry_fix(unsigned int domid, int num, bool update); -int domain_entry(struct connection *conn); +int domain_nbentry_inc(struct connection *conn, unsigned int domid); +int domain_nbentry_dec(struct connection *conn, unsigned int domid); +int domain_nbentry_fix(unsigned int domid, int num, bool update); +int domain_nbentry(struct connection *conn); int domain_memory_add(unsigned int domid, int mem, bool no_quota_check); /* @@ -99,8 +99,6 @@ void domain_outstanding_domid_dec(unsigned int domid); int domain_get_quota(const void *ctx, struct connection *conn, unsigned int domid); int acc_fix_domains(struct list_head *head, bool update); -int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val, - unsigned int domid); /* Write rate limiting */ diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c index 89b92f0baf..82e5e66c18 100644 --- a/tools/xenstore/xenstored_transaction.c +++ b/tools/xenstore/xenstored_transaction.c @@ -548,20 +548,9 @@ int do_transaction_end(const void *ctx, struct connection *conn, return 0; } -void transaction_entry_inc(struct transaction *trans, unsigned int domid) +struct list_head *transaction_get_changed_domains(struct transaction *trans) { - if (acc_add_dom_nbentry(trans, &trans->changed_domains, 1, domid)) { - /* Let the transaction fail. */ - trans->fail = true; - } -} - -void transaction_entry_dec(struct transaction *trans, unsigned int domid) -{ - if (acc_add_dom_nbentry(trans, &trans->changed_domains, -1, domid)) { - /* Let the transaction fail. */ - trans->fail = true; - } + return &trans->changed_domains; } void fail_transaction(struct transaction *trans) diff --git a/tools/xenstore/xenstored_transaction.h b/tools/xenstore/xenstored_transaction.h index 3417303f94..b6f8cb7d0a 100644 --- a/tools/xenstore/xenstored_transaction.h +++ b/tools/xenstore/xenstored_transaction.h @@ -36,10 +36,6 @@ int do_transaction_end(const void *ctx, struct connection *conn, struct transaction *transaction_lookup(struct connection *conn, uint32_t id); -/* inc/dec entry number local to trans while changing a node */ -void transaction_entry_inc(struct transaction *trans, unsigned int domid); -void transaction_entry_dec(struct transaction *trans, unsigned int domid); - /* This node was accessed. */ int __must_check access_node(struct connection *conn, struct node *node, enum node_access_type type, TDB_DATA *key); @@ -54,6 +50,9 @@ void transaction_prepend(struct connection *conn, const char *name, /* Mark the transaction as failed. This will prevent it to be committed. */ void fail_transaction(struct transaction *trans); +/* Get the list head of the changed domains. */ +struct list_head *transaction_get_changed_domains(struct transaction *trans); + void conn_delete_all_transactions(struct connection *conn); int check_transactions(struct hashtable *hash);
Rework the interface and the internals of the per-domain node accounting: - rename the functions to domain_nbentry_*() in order to better match the related counter name - switch from node pointer to domid as interface, as all nodes have the owner filled in - use a common internal function for adding a value to the counter For the transaction case add a helper function to get the list head of the per-transaction changed domains, enabling to eliminate the transaction_entry_*() functions. Signed-off-by: Juergen Gross <jgross@suse.com> --- tools/xenstore/xenstored_core.c | 22 ++--- tools/xenstore/xenstored_domain.c | 122 +++++++++++-------------- tools/xenstore/xenstored_domain.h | 10 +- tools/xenstore/xenstored_transaction.c | 15 +-- tools/xenstore/xenstored_transaction.h | 7 +- 5 files changed, 72 insertions(+), 104 deletions(-)