Re: [PATCH 1/7] block/nbd: avoid touching freed connect_thread

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

15.03.2021 09:06, Roman Kagan wrote:
> When the NBD connection is being torn down, the connection thread gets
> canceled and "detached", meaning it is about to get freed.
> 
> If this happens while the connection coroutine yielded waiting for the
> connection thread to complete, when it resumes it may access the
> invalidated connection thread data.
> 
> To prevent this, revalidate the ->connect_thread pointer in
> nbd_co_establish_connection_cancel before using after the the yield.
> 
> Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>

Seems possible. Do you have a reproducer? Would be great to make an iotest.

> ---
>   block/nbd.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index c26dc5a54f..447d176b76 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -486,6 +486,15 @@ nbd_co_establish_connection(BlockDriverState *bs, Error **errp)
>       s->wait_connect = true;
>       qemu_coroutine_yield();
>   
> +    /*
> +     * If nbd_co_establish_connection_cancel had a chance to run it may have
> +     * invalidated ->connect_thread.
> +     */
> +    thr = s->connect_thread;
> +    if (!thr) {
> +        return -ECONNABORTED;
> +    }
> +
>       qemu_mutex_lock(&thr->mutex);
>   
>       switch (thr->state) {
> 

Patch looks a bit like s->connect_thread may become something other than NULL during the yield.. But it can't actually. Maybe it will be possible with further patches?

Anyway, make sense to me:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir