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

[Julien Grall <julien@xxxxxxx>]

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...
> > 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.
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.
> 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.
We also have quotas to limit the number of nodes accessed in a transaction.

> >    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.
>  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.
So the overall behavior is changed.

> > In both situation, the transaction will still be committed. This will 
> > now be prevented by this patch.
> As said above, only in the first case.
Cheers,

--
Julien Grall