Re: [PATCH v2] tools/xenstore: fix quota check in acc_fix_domains()

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxx>
> Fixes: f2bebf72c4d5 ("xenstore: rework of transaction handling")
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>

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);
--
Julien Grall