|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 01/13] tools/xenstore: don't allow creating too many nodes in a transaction
On 20.02.23 15:15, Julien Grall wrote: On 20/02/2023 13:49, Juergen Gross wrote:This could have been mentioned after ---. This could allow people to understand the concern and then participate.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. Will do so in future. 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? I'm referring to the quota check in create_node(). 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? Try it: - create nodes in a guest until you hit the ENOSPC return code due to too many nodes - start a transaction deleting some nodes and then trying to create another one, which fail fail with ENOSPC. Juergen Attachment:
OpenPGP_0xB0DE9DD628BF132F.asc Attachment:
OpenPGP_signature
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |