Re: [PATCH] block/stream: Drain subtree around graph change

[Hanna Reitz <hreitz@redhat.com>]

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