| Message ID | 20230328144327.6562-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] tools/xenstore: fix quota check in acc_fix_domains() | expand |
Hi Juergen, On 28/03/2023 15:43, Juergen Gross wrote: > Today when finalizing a transaction the number of node quota is checked > to not being exceeded after the transaction. This check is always done, > even if the transaction is being performed by a privileged connection, > or if there were no nodes created in the transaction. > > Correct that by checking quota only if: > - the transaction is being performed by an unprivileged guest, and > - at least one node was created in the transaction > > Reported-by: Julien Grall <julien@xen.org> > Fixes: f2bebf72c4d5 ("xenstore: rework of transaction handling") > Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Julien Grall <jgrall@amazon.com> And committed. Cheers, > --- > V2: > - add comment (Julien Grall) > --- > tools/xenstore/xenstored_core.c | 3 +++ > tools/xenstore/xenstored_domain.c | 4 ++-- > tools/xenstore/xenstored_domain.h | 7 ++++++- > tools/xenstore/xenstored_transaction.c | 16 ++++++++++++++-- > tools/xenstore/xenstored_transaction.h | 3 +++ > 5 files changed, 28 insertions(+), 5 deletions(-) > > diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c > index a61db2db2d..3ca68681e3 100644 > --- a/tools/xenstore/xenstored_core.c > +++ b/tools/xenstore/xenstored_core.c > @@ -1472,6 +1472,9 @@ static struct node *create_node(struct connection *conn, const void *ctx, > if (!node) > return NULL; > > + if (conn && conn->transaction) > + ta_node_created(conn->transaction); > + > node->data = data; > node->datalen = datalen; > > diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c > index d7fc2fafc7..f62be2245c 100644 > --- a/tools/xenstore/xenstored_domain.c > +++ b/tools/xenstore/xenstored_domain.c > @@ -544,7 +544,7 @@ static struct domain *find_domain_by_domid(unsigned int domid) > return (d && d->introduced) ? d : NULL; > } > > -int acc_fix_domains(struct list_head *head, bool update) > +int acc_fix_domains(struct list_head *head, bool chk_quota, bool update) > { > struct changed_domain *cd; > int cnt; > @@ -552,7 +552,7 @@ int acc_fix_domains(struct list_head *head, bool update) > list_for_each_entry(cd, head, list) { > cnt = domain_nbentry_fix(cd->domid, cd->nbentry, update); > if (!update) { > - if (cnt >= quota_nb_entry_per_domain) > + if (chk_quota && cnt >= quota_nb_entry_per_domain) > return ENOSPC; > if (cnt < 0) > return ENOMEM; > diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h > index dc4660861e..279cccb3ad 100644 > --- a/tools/xenstore/xenstored_domain.h > +++ b/tools/xenstore/xenstored_domain.h > @@ -96,7 +96,12 @@ void domain_outstanding_dec(struct connection *conn); > 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); > + > +/* > + * Update or check number of nodes per domain at the end of a transaction. > + * If "update" is true, "chk_quota" is ignored. > + */ > +int acc_fix_domains(struct list_head *head, bool chk_quota, bool update); > > /* Write rate limiting */ > > diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c > index 1aa9d3cb3d..2b15506953 100644 > --- a/tools/xenstore/xenstored_transaction.c > +++ b/tools/xenstore/xenstored_transaction.c > @@ -160,12 +160,20 @@ struct transaction > /* List of changed domains - to record the changed domain entry number */ > struct list_head changed_domains; > > + /* There was at least one node created in the transaction. */ > + bool node_created; > + > /* Flag for letting transaction fail. */ > bool fail; > }; > > uint64_t generation; > > +void ta_node_created(struct transaction *trans) > +{ > + trans->node_created = true; > +} > + > static struct accessed_node *find_accessed_node(struct transaction *trans, > const char *name) > { > @@ -509,6 +517,7 @@ int do_transaction_end(const void *ctx, struct connection *conn, > const char *arg = onearg(in); > struct transaction *trans; > bool is_corrupt = false; > + bool chk_quota; > int ret; > > if (!arg || (!streq(arg, "T") && !streq(arg, "F"))) > @@ -523,13 +532,16 @@ int do_transaction_end(const void *ctx, struct connection *conn, > if (!conn->transaction_started) > conn->ta_start_time = 0; > > + chk_quota = trans->node_created && domain_is_unprivileged(conn); > + > /* Attach transaction to ctx for auto-cleanup */ > talloc_steal(ctx, trans); > > if (streq(arg, "T")) { > if (trans->fail) > return ENOMEM; > - ret = acc_fix_domains(&trans->changed_domains, false); > + ret = acc_fix_domains(&trans->changed_domains, chk_quota, > + false); > if (ret) > return ret; > ret = finalize_transaction(conn, trans, &is_corrupt); > @@ -539,7 +551,7 @@ int do_transaction_end(const void *ctx, struct connection *conn, > wrl_apply_debit_trans_commit(conn); > > /* fix domain entry for each changed domain */ > - acc_fix_domains(&trans->changed_domains, true); > + acc_fix_domains(&trans->changed_domains, false, true); > > if (is_corrupt) > corrupt(conn, "transaction inconsistency"); > diff --git a/tools/xenstore/xenstored_transaction.h b/tools/xenstore/xenstored_transaction.h > index b6f8cb7d0a..883145163f 100644 > --- a/tools/xenstore/xenstored_transaction.h > +++ b/tools/xenstore/xenstored_transaction.h > @@ -36,6 +36,9 @@ int do_transaction_end(const void *ctx, struct connection *conn, > > struct transaction *transaction_lookup(struct connection *conn, uint32_t id); > > +/* Set flag for created node. */ > +void ta_node_created(struct transaction *trans); > + > /* This node was accessed. */ > int __must_check access_node(struct connection *conn, struct node *node, > enum node_access_type type, TDB_DATA *key);
On 28.03.2023 16:43, Juergen Gross wrote: > Today when finalizing a transaction the number of node quota is checked > to not being exceeded after the transaction. This check is always done, > even if the transaction is being performed by a privileged connection, > or if there were no nodes created in the transaction. > > Correct that by checking quota only if: > - the transaction is being performed by an unprivileged guest, and > - at least one node was created in the transaction > > Reported-by: Julien Grall <julien@xen.org> > Fixes: f2bebf72c4d5 ("xenstore: rework of transaction handling") > Signed-off-by: Juergen Gross <jgross@suse.com> Considering the referenced commit is about 6 years old, I thought this would be a backporting candidate. The function mentioned in the title doesn't appear to exist on 4.17, though. Therefore, if there is an intention for this to be backported, may I please ask that a suitable backport be provided? Thanks, Jan
On 24.04.23 10:56, Jan Beulich wrote: > On 28.03.2023 16:43, Juergen Gross wrote: >> Today when finalizing a transaction the number of node quota is checked >> to not being exceeded after the transaction. This check is always done, >> even if the transaction is being performed by a privileged connection, >> or if there were no nodes created in the transaction. >> >> Correct that by checking quota only if: >> - the transaction is being performed by an unprivileged guest, and >> - at least one node was created in the transaction >> >> Reported-by: Julien Grall <julien@xen.org> >> Fixes: f2bebf72c4d5 ("xenstore: rework of transaction handling") >> Signed-off-by: Juergen Gross <jgross@suse.com> > > Considering the referenced commit is about 6 years old, I thought this > would be a backporting candidate. The function mentioned in the title > doesn't appear to exist on 4.17, though. Therefore, if there is an > intention for this to be backported, may I please ask that a suitable > backport be provided? This should do the job. Juergen
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c index a61db2db2d..3ca68681e3 100644 --- a/tools/xenstore/xenstored_core.c +++ b/tools/xenstore/xenstored_core.c @@ -1472,6 +1472,9 @@ static struct node *create_node(struct connection *conn, const void *ctx, if (!node) return NULL; + if (conn && conn->transaction) + ta_node_created(conn->transaction); + node->data = data; node->datalen = datalen; diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c index d7fc2fafc7..f62be2245c 100644 --- a/tools/xenstore/xenstored_domain.c +++ b/tools/xenstore/xenstored_domain.c @@ -544,7 +544,7 @@ static struct domain *find_domain_by_domid(unsigned int domid) return (d && d->introduced) ? d : NULL; } -int acc_fix_domains(struct list_head *head, bool update) +int acc_fix_domains(struct list_head *head, bool chk_quota, bool update) { struct changed_domain *cd; int cnt; @@ -552,7 +552,7 @@ int acc_fix_domains(struct list_head *head, bool update) list_for_each_entry(cd, head, list) { cnt = domain_nbentry_fix(cd->domid, cd->nbentry, update); if (!update) { - if (cnt >= quota_nb_entry_per_domain) + if (chk_quota && cnt >= quota_nb_entry_per_domain) return ENOSPC; if (cnt < 0) return ENOMEM; diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h index dc4660861e..279cccb3ad 100644 --- a/tools/xenstore/xenstored_domain.h +++ b/tools/xenstore/xenstored_domain.h @@ -96,7 +96,12 @@ void domain_outstanding_dec(struct connection *conn); 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); + +/* + * Update or check number of nodes per domain at the end of a transaction. + * If "update" is true, "chk_quota" is ignored. + */ +int acc_fix_domains(struct list_head *head, bool chk_quota, bool update); /* Write rate limiting */ diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c index 1aa9d3cb3d..2b15506953 100644 --- a/tools/xenstore/xenstored_transaction.c +++ b/tools/xenstore/xenstored_transaction.c @@ -160,12 +160,20 @@ struct transaction /* List of changed domains - to record the changed domain entry number */ struct list_head changed_domains; + /* There was at least one node created in the transaction. */ + bool node_created; + /* Flag for letting transaction fail. */ bool fail; }; uint64_t generation; +void ta_node_created(struct transaction *trans) +{ + trans->node_created = true; +} + static struct accessed_node *find_accessed_node(struct transaction *trans, const char *name) { @@ -509,6 +517,7 @@ int do_transaction_end(const void *ctx, struct connection *conn, const char *arg = onearg(in); struct transaction *trans; bool is_corrupt = false; + bool chk_quota; int ret; if (!arg || (!streq(arg, "T") && !streq(arg, "F"))) @@ -523,13 +532,16 @@ int do_transaction_end(const void *ctx, struct connection *conn, if (!conn->transaction_started) conn->ta_start_time = 0; + chk_quota = trans->node_created && domain_is_unprivileged(conn); + /* Attach transaction to ctx for auto-cleanup */ talloc_steal(ctx, trans); if (streq(arg, "T")) { if (trans->fail) return ENOMEM; - ret = acc_fix_domains(&trans->changed_domains, false); + ret = acc_fix_domains(&trans->changed_domains, chk_quota, + false); if (ret) return ret; ret = finalize_transaction(conn, trans, &is_corrupt); @@ -539,7 +551,7 @@ int do_transaction_end(const void *ctx, struct connection *conn, wrl_apply_debit_trans_commit(conn); /* fix domain entry for each changed domain */ - acc_fix_domains(&trans->changed_domains, true); + acc_fix_domains(&trans->changed_domains, false, true); if (is_corrupt) corrupt(conn, "transaction inconsistency"); diff --git a/tools/xenstore/xenstored_transaction.h b/tools/xenstore/xenstored_transaction.h index b6f8cb7d0a..883145163f 100644 --- a/tools/xenstore/xenstored_transaction.h +++ b/tools/xenstore/xenstored_transaction.h @@ -36,6 +36,9 @@ int do_transaction_end(const void *ctx, struct connection *conn, struct transaction *transaction_lookup(struct connection *conn, uint32_t id); +/* Set flag for created node. */ +void ta_node_created(struct transaction *trans); + /* This node was accessed. */ int __must_check access_node(struct connection *conn, struct node *node, enum node_access_type type, TDB_DATA *key);
Today when finalizing a transaction the number of node quota is checked to not being exceeded after the transaction. This check is always done, even if the transaction is being performed by a privileged connection, or if there were no nodes created in the transaction. Correct that by checking quota only if: - the transaction is being performed by an unprivileged guest, and - at least one node was created in the transaction Reported-by: Julien Grall <julien@xen.org> Fixes: f2bebf72c4d5 ("xenstore: rework of transaction handling") Signed-off-by: Juergen Gross <jgross@suse.com> --- V2: - add comment (Julien Grall) --- tools/xenstore/xenstored_core.c | 3 +++ tools/xenstore/xenstored_domain.c | 4 ++-- tools/xenstore/xenstored_domain.h | 7 ++++++- tools/xenstore/xenstored_transaction.c | 16 ++++++++++++++-- tools/xenstore/xenstored_transaction.h | 3 +++ 5 files changed, 28 insertions(+), 5 deletions(-)