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

[Julien Grall <julien@xxxxxxx>]

On 20/02/2023 13:49, Juergen Gross wrote:
> 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.
This could have been mentioned after ---. This could allow people to 
understand the concern and then participate.
> > > > 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.
I view the two different.

> 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.
Are you referring to the quota check within the transaction?

> 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).
This doesn't sound correct. To do you have any test I could use to check 
the behavior?
Cheers,

--
Julien Grall