Re: [PATCH v2 01/13] tools/xenstore: don't allow creating too many nodes in a transaction

[Juergen Gross <jgross@xxxxxxxx>]

On 20.02.23 13:07, Julien Grall wrote:
> Hi Juergen,
>
> On 20/02/2023 11:04, Juergen Gross wrote:
> > On 20.02.23 10:46, Julien Grall wrote:
> > > Hi Juergen,
> > >
> > > On 20/01/2023 10:00, Juergen Gross wrote:
> > > > The accounting for the number of nodes of a domain in an active
> > > > transaction is not working correctly, as it allows to create arbitrary
> > > > number of nodes. The transaction will finally fail due to exceeding
> > > > the number of nodes quota, but before closing the transaction an
> > > > unprivileged guest could cause Xenstore to use a lot of memory.
> > > I know I said I would delay my decision on this patch. However, I was still 
> > > expecting the commit message to be updated based on our previous discussion.
> > As said before, I was waiting on the settlement of our discussion before
> > doing the update.
> This is not a very good practice to resend a patch without recording the 
> disagreement because new reviewers may not be aware of the disagreement and this 
> could end up to be committed by mistake...
You said you wanted to look into this patch in detail after the previous
series, so I move it into this series. The wording update would strongly
depend on the outcome of the discussion IMO, so I didn't do it yet.

Not adding the patch in this series would require some additional rebase
effort, so I kept the patch as is.

> > > Also thinking more about it, "The transaction will finally fail due to 
> > > exceeding the number of nodes quota" may not be true for a couple of reasons:
> > >    1) The transaction may removed a node afterwards.
> > Yes, and? Just because it is a transaction, this is still a violation of
> > the quota. Even outside a transaction you could use the same reasoning,
> > but you don't (which is correct, of course).
> I can understand your point. However, to me, the goal of the transaction is to 
> commit everything in one go at the end. So the violations in the middle should 
> not matter.
Of course they should.

We wouldn't allow to write over-sized nodes, even if they could be rewritten in
the same transaction with a smaller size.

We wouldn't allow to create nodes which would violate overall memory
consumption.

We wouldn't allow nodes with more permission entries than allowed, even if they
could be reduced in the same transaction again.

I don't see why the number of nodes shouldn't be taken into account.

> Furthermore, I would expect a transaction to work on a snapshot of the system. 
> So it feels really strange to me that we are constantly checking the quota with 
> the updated values rather than the initial one.
You are aware that the code without this patch is just neglecting the nodes
created in the transaction? It just is using the current number of nodes
outside any transaction for quota check. So I could easily create a scenario
where my new code will succeed, but the current one is failing:

Assume node quota is 1000, and at start of the transaction the guest is owning
999 nodes. In the transaction the guest is deleting 10 nodes, then dom0 is
creating 5 additional nodes owned by the guest. The central node counter is now
1004 (over quota), while the in-transaction count is 994. In the transaction the
guest can now happily create a new node (#995) with my patch, but will fail to
do so without (it sees the 1004 due to the transaction count being ignored).

> > In case you never finish the transaction, you are owner of more than
> > allowed nodes.
> How so? The nodes would not be committed so you don't really own them.
But you can use them inside the transaction.

> We also have quotas to limit the number of nodes accessed in a transaction.
Yes, but you can use the excess nodes in the transaction until that quota
is reached.

> > >    2) A node may have been removed outside of the transaction.
> > If the removed node hasn't been touched by the transaction, it will be
> > accounted for correctly.
> It depends on when the node was removed. If it is removed *after* the quota was 
> hit then your transaction will fail.
Yes. That is why the quota check at the finalization of the transaction has
to be kept.

> >  If it has been touched, the transaction will
> > fail anyway.
> So the transaction will fail to commit because there is a conflict. So the 
> client is expected to retry it. We should not expected the client to retry for 
> error like quota.
Correct. The failure I'm mentioning isn't due to quota, but due to a conflict.

> So the overall behavior is changed.
We do so for any bug-fix. And IMO my patch is a bug-fix.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature