|
From: | Hanna Reitz |
Subject: | Re: [PATCH] block/stream: Drain subtree around graph change |
Date: | Tue, 5 Apr 2022 14:05:23 +0200 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 |
On 05.04.22 13:47, Hanna Reitz wrote:
On 05.04.22 12:14, Kevin Wolf wrote:
[...]
At the same time they probably do too little, because what you're describing you're protecting against is not I/O, but graph modifications done by callbacks invoked in the AIO_WAIT_WHILE() when replacing the backing file. The callback could be invoked by I/O on an entirely different subgraph (maybe if the other thing is a mirror job)or it could be a BH or anything else really. bdrv_drain_all() would increase your chances, but I'm not sure if even that would be guaranteed to be enough - because it's really another instance of abusing drain for locking, we're not really interested in the _I/O_ of the node.
[...]
I’m not sure what you’re arguing for, so I can only assume. Perhaps you’re arguing for reverting this patch, which I wouldn’t want to do, because at least it fixes the one known use-after-free case. Perhaps you’re arguing that we need something better, and then I completely agree.
Perhaps I should also note that what actually fixes the use-after-free is the bdrv_ref()/unref() pair. The drained section is just there to ensure that the graph is actually correct (i.e. if a concurrently finishing job removes @base before the stream job’s bdrv_set_backing_hd() can set it as the top node’s backing node, that we won’t reinstate this @base that the other job just removed). So even if this does too little, at least there won’t be a use-after-free.
OTOH, if it does much too much, we can drop the drain and keep the ref/unref. I don’t want to have a release with a use-after-free that I know of, but I’d be fine if the block graph is “just” outdated.
Hanna
[Prev in Thread] | Current Thread | [Next in Thread] |