* [PATCH] tools/xenstore: fix quota check in acc_fix_domains()
@ 2023-02-24 15:58 Juergen Gross
2023-03-23 12:38 ` Julien Grall
0 siblings, 1 reply; 7+ messages in thread
From: Juergen Gross @ 2023-02-24 15:58 UTC (permalink / raw
To: xen-devel; +Cc: Juergen Gross, Wei Liu, Julien Grall, Anthony PERARD
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>
---
tools/xenstore/xenstored_core.c | 3 +++
tools/xenstore/xenstored_domain.c | 4 ++--
tools/xenstore/xenstored_domain.h | 2 +-
tools/xenstore/xenstored_transaction.c | 16 ++++++++++++++--
tools/xenstore/xenstored_transaction.h | 3 +++
5 files changed, 23 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..ec6aa00cc7 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -96,7 +96,7 @@ 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);
+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);
--
2.35.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] tools/xenstore: fix quota check in acc_fix_domains()
2023-02-24 15:58 [PATCH] tools/xenstore: fix quota check in acc_fix_domains() Juergen Gross
@ 2023-03-23 12:38 ` Julien Grall
2023-03-23 12:53 ` Juergen Gross
0 siblings, 1 reply; 7+ messages in thread
From: Julien Grall @ 2023-03-23 12:38 UTC (permalink / raw
To: Juergen Gross, xen-devel; +Cc: Wei Liu, Anthony PERARD
Hi Juergen,
On 24/02/2023 15:58, 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>
> ---
> tools/xenstore/xenstored_core.c | 3 +++
> tools/xenstore/xenstored_domain.c | 4 ++--
> tools/xenstore/xenstored_domain.h | 2 +-
> tools/xenstore/xenstored_transaction.c | 16 ++++++++++++++--
> tools/xenstore/xenstored_transaction.h | 3 +++
> 5 files changed, 23 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..ec6aa00cc7 100644
> --- a/tools/xenstore/xenstored_domain.h
> +++ b/tools/xenstore/xenstored_domain.h
> @@ -96,7 +96,7 @@ 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);
> +int acc_fix_domains(struct list_head *head, bool chk_quota, bool update);
Depending on the answer below, I would suggest to write that 'chk_quota'
is ignored when ``update`` is true.
>
> /* 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);
In theory, shouldn't we pass 'chk_quota' rather than false? In practice,
I know it doesn't make any difference between this is an update.
>
> 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);
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tools/xenstore: fix quota check in acc_fix_domains()
2023-03-23 12:38 ` Julien Grall
@ 2023-03-23 12:53 ` Juergen Gross
2023-03-23 14:20 ` Julien Grall
0 siblings, 1 reply; 7+ messages in thread
From: Juergen Gross @ 2023-03-23 12:53 UTC (permalink / raw
To: Julien Grall, xen-devel; +Cc: Wei Liu, Anthony PERARD
[-- Attachment #1.1.1: Type: text/plain, Size: 6472 bytes --]
On 23.03.23 13:38, Julien Grall wrote:
> Hi Juergen,
>
> On 24/02/2023 15:58, 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>
>> ---
>> tools/xenstore/xenstored_core.c | 3 +++
>> tools/xenstore/xenstored_domain.c | 4 ++--
>> tools/xenstore/xenstored_domain.h | 2 +-
>> tools/xenstore/xenstored_transaction.c | 16 ++++++++++++++--
>> tools/xenstore/xenstored_transaction.h | 3 +++
>> 5 files changed, 23 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..ec6aa00cc7 100644
>> --- a/tools/xenstore/xenstored_domain.h
>> +++ b/tools/xenstore/xenstored_domain.h
>> @@ -96,7 +96,7 @@ 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);
>> +int acc_fix_domains(struct list_head *head, bool chk_quota, bool update);
>
> Depending on the answer below, I would suggest to write that 'chk_quota' is
> ignored when ``update`` is true.
With the answer below, do you agree that no additional comment is needed?
I'm fine either way.
>
>> /* 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);
>
> In theory, shouldn't we pass 'chk_quota' rather than false? In practice, I know
> it doesn't make any difference between this is an update.
We explicitly don't want to check quota in the "update" case. So specifying
"false" is the correct thing to do IMHO.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tools/xenstore: fix quota check in acc_fix_domains()
2023-03-23 12:53 ` Juergen Gross
@ 2023-03-23 14:20 ` Julien Grall
2023-03-23 14:21 ` Juergen Gross
0 siblings, 1 reply; 7+ messages in thread
From: Julien Grall @ 2023-03-23 14:20 UTC (permalink / raw
To: Juergen Gross, xen-devel; +Cc: Wei Liu, Anthony PERARD
Hi,
On 23/03/2023 12:53, Juergen Gross wrote:
> On 23.03.23 13:38, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 24/02/2023 15:58, 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>
>>> ---
>>> tools/xenstore/xenstored_core.c | 3 +++
>>> tools/xenstore/xenstored_domain.c | 4 ++--
>>> tools/xenstore/xenstored_domain.h | 2 +-
>>> tools/xenstore/xenstored_transaction.c | 16 ++++++++++++++--
>>> tools/xenstore/xenstored_transaction.h | 3 +++
>>> 5 files changed, 23 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..ec6aa00cc7 100644
>>> --- a/tools/xenstore/xenstored_domain.h
>>> +++ b/tools/xenstore/xenstored_domain.h
>>> @@ -96,7 +96,7 @@ 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);
>>> +int acc_fix_domains(struct list_head *head, bool chk_quota, bool
>>> update);
>>
>> Depending on the answer below, I would suggest to write that
>> 'chk_quota' is ignored when ``update`` is true.
>
> With the answer below, do you agree that no additional comment is needed?
> I'm fine either way.
>
>>
>>> /* 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);
>>
>> In theory, shouldn't we pass 'chk_quota' rather than false? In
>> practice, I know it doesn't make any difference between this is an
>> update.
>
> We explicitly don't want to check quota in the "update" case. So specifying
> "false" is the correct thing to do IMHO.
Let me rephrase my comment differently. What would happen if the user
pass 'true'? Would we check the quota or not?
I suspect this is a no, hence why I was suggested the comment to say the
field is ignored. Alternatively, we could add an assert that ensure that
chk_quota is false when update is true.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tools/xenstore: fix quota check in acc_fix_domains()
2023-03-23 14:20 ` Julien Grall
@ 2023-03-23 14:21 ` Juergen Gross
2023-03-23 14:29 ` Julien Grall
0 siblings, 1 reply; 7+ messages in thread
From: Juergen Gross @ 2023-03-23 14:21 UTC (permalink / raw
To: Julien Grall, xen-devel; +Cc: Wei Liu, Anthony PERARD
[-- Attachment #1.1.1: Type: text/plain, Size: 7208 bytes --]
On 23.03.23 15:20, Julien Grall wrote:
> Hi,
>
> On 23/03/2023 12:53, Juergen Gross wrote:
>> On 23.03.23 13:38, Julien Grall wrote:
>>> Hi Juergen,
>>>
>>> On 24/02/2023 15:58, 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>
>>>> ---
>>>> tools/xenstore/xenstored_core.c | 3 +++
>>>> tools/xenstore/xenstored_domain.c | 4 ++--
>>>> tools/xenstore/xenstored_domain.h | 2 +-
>>>> tools/xenstore/xenstored_transaction.c | 16 ++++++++++++++--
>>>> tools/xenstore/xenstored_transaction.h | 3 +++
>>>> 5 files changed, 23 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..ec6aa00cc7 100644
>>>> --- a/tools/xenstore/xenstored_domain.h
>>>> +++ b/tools/xenstore/xenstored_domain.h
>>>> @@ -96,7 +96,7 @@ 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);
>>>> +int acc_fix_domains(struct list_head *head, bool chk_quota, bool update);
>>>
>>> Depending on the answer below, I would suggest to write that 'chk_quota' is
>>> ignored when ``update`` is true.
>>
>> With the answer below, do you agree that no additional comment is needed?
>> I'm fine either way.
>>
>>>
>>>> /* 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);
>>>
>>> In theory, shouldn't we pass 'chk_quota' rather than false? In practice, I
>>> know it doesn't make any difference between this is an update.
>>
>> We explicitly don't want to check quota in the "update" case. So specifying
>> "false" is the correct thing to do IMHO.
>
> Let me rephrase my comment differently. What would happen if the user pass
> 'true'? Would we check the quota or not?
>
> I suspect this is a no, hence why I was suggested the comment to say the field
> is ignored. Alternatively, we could add an assert that ensure that chk_quota is
> false when update is true.
Okay, I'll add the comment.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tools/xenstore: fix quota check in acc_fix_domains()
2023-03-23 14:21 ` Juergen Gross
@ 2023-03-23 14:29 ` Julien Grall
2023-03-23 14:30 ` Juergen Gross
0 siblings, 1 reply; 7+ messages in thread
From: Julien Grall @ 2023-03-23 14:29 UTC (permalink / raw
To: Juergen Gross, xen-devel; +Cc: Wei Liu, Anthony PERARD
On 23/03/2023 14:21, Juergen Gross wrote:
> On 23.03.23 15:20, Julien Grall wrote:
>> Hi,
>>
>> On 23/03/2023 12:53, Juergen Gross wrote:
>>> On 23.03.23 13:38, Julien Grall wrote:
>>>> Hi Juergen,
>>>>
>>>> On 24/02/2023 15:58, 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>
>>>>> ---
>>>>> tools/xenstore/xenstored_core.c | 3 +++
>>>>> tools/xenstore/xenstored_domain.c | 4 ++--
>>>>> tools/xenstore/xenstored_domain.h | 2 +-
>>>>> tools/xenstore/xenstored_transaction.c | 16 ++++++++++++++--
>>>>> tools/xenstore/xenstored_transaction.h | 3 +++
>>>>> 5 files changed, 23 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..ec6aa00cc7 100644
>>>>> --- a/tools/xenstore/xenstored_domain.h
>>>>> +++ b/tools/xenstore/xenstored_domain.h
>>>>> @@ -96,7 +96,7 @@ 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);
>>>>> +int acc_fix_domains(struct list_head *head, bool chk_quota, bool
>>>>> update);
>>>>
>>>> Depending on the answer below, I would suggest to write that
>>>> 'chk_quota' is ignored when ``update`` is true.
>>>
>>> With the answer below, do you agree that no additional comment is
>>> needed?
>>> I'm fine either way.
>>>
>>>>
>>>>> /* 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);
>>>>
>>>> In theory, shouldn't we pass 'chk_quota' rather than false? In
>>>> practice, I know it doesn't make any difference between this is an
>>>> update.
>>>
>>> We explicitly don't want to check quota in the "update" case. So
>>> specifying
>>> "false" is the correct thing to do IMHO.
>>
>> Let me rephrase my comment differently. What would happen if the user
>> pass 'true'? Would we check the quota or not?
>>
>> I suspect this is a no, hence why I was suggested the comment to say
>> the field is ignored. Alternatively, we could add an assert that
>> ensure that chk_quota is false when update is true.
>
> Okay, I'll add the comment.
Thanks! No need to send a new version. I tested the code and confirmed
that it solved the problem I reported. I would be happy to add the
comment on commit once we agree on it.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tools/xenstore: fix quota check in acc_fix_domains()
2023-03-23 14:29 ` Julien Grall
@ 2023-03-23 14:30 ` Juergen Gross
0 siblings, 0 replies; 7+ messages in thread
From: Juergen Gross @ 2023-03-23 14:30 UTC (permalink / raw
To: Julien Grall, xen-devel; +Cc: Wei Liu, Anthony PERARD
[-- Attachment #1.1.1: Type: text/plain, Size: 7827 bytes --]
On 23.03.23 15:29, Julien Grall wrote:
>
>
> On 23/03/2023 14:21, Juergen Gross wrote:
>> On 23.03.23 15:20, Julien Grall wrote:
>>> Hi,
>>>
>>> On 23/03/2023 12:53, Juergen Gross wrote:
>>>> On 23.03.23 13:38, Julien Grall wrote:
>>>>> Hi Juergen,
>>>>>
>>>>> On 24/02/2023 15:58, 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>
>>>>>> ---
>>>>>> tools/xenstore/xenstored_core.c | 3 +++
>>>>>> tools/xenstore/xenstored_domain.c | 4 ++--
>>>>>> tools/xenstore/xenstored_domain.h | 2 +-
>>>>>> tools/xenstore/xenstored_transaction.c | 16 ++++++++++++++--
>>>>>> tools/xenstore/xenstored_transaction.h | 3 +++
>>>>>> 5 files changed, 23 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..ec6aa00cc7 100644
>>>>>> --- a/tools/xenstore/xenstored_domain.h
>>>>>> +++ b/tools/xenstore/xenstored_domain.h
>>>>>> @@ -96,7 +96,7 @@ 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);
>>>>>> +int acc_fix_domains(struct list_head *head, bool chk_quota, bool update);
>>>>>
>>>>> Depending on the answer below, I would suggest to write that 'chk_quota' is
>>>>> ignored when ``update`` is true.
>>>>
>>>> With the answer below, do you agree that no additional comment is needed?
>>>> I'm fine either way.
>>>>
>>>>>
>>>>>> /* 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);
>>>>>
>>>>> In theory, shouldn't we pass 'chk_quota' rather than false? In practice, I
>>>>> know it doesn't make any difference between this is an update.
>>>>
>>>> We explicitly don't want to check quota in the "update" case. So specifying
>>>> "false" is the correct thing to do IMHO.
>>>
>>> Let me rephrase my comment differently. What would happen if the user pass
>>> 'true'? Would we check the quota or not?
>>>
>>> I suspect this is a no, hence why I was suggested the comment to say the
>>> field is ignored. Alternatively, we could add an assert that ensure that
>>> chk_quota is false when update is true.
>>
>> Okay, I'll add the comment.
>
> Thanks! No need to send a new version. I tested the code and confirmed that it
> solved the problem I reported. I would be happy to add the comment on commit
> once we agree on it.
Thanks,
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-03-23 14:31 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-24 15:58 [PATCH] tools/xenstore: fix quota check in acc_fix_domains() Juergen Gross
2023-03-23 12:38 ` Julien Grall
2023-03-23 12:53 ` Juergen Gross
2023-03-23 14:20 ` Julien Grall
2023-03-23 14:21 ` Juergen Gross
2023-03-23 14:29 ` Julien Grall
2023-03-23 14:30 ` Juergen Gross
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).