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
