[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] tools/xenstore: fix quota check in acc_fix_domains()
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@xxxxxxx>
Fixes: f2bebf72c4d5 ("xenstore: rework of transaction handling")
Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
---
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
|
