Re: [Devel] [PATCH hci-8.1] drivers/vhost/blk: add support for worker ioctls

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] vhost-blk: completed bio is stranded when vq->worker is NULL, hanging vhost_blk_flush()
--

commit 813c45791bfed20701bbd685b3f3a40178259b8d
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

drivers/vhost/blk: add support for worker ioctls

This patch routes VHOST_NEW_WORKER, VHOST_FREE_WORKER,
VHOST_ATTACH_VRING_WORKER and VHOST_GET_VRING_WORKER to
vhost_worker_ioctl() under the device mutex in vhost_blk_ioctl(). This lets
userspace create vhost workers and bind them to vhost-blk virtqueues.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-107537

> diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
> index b170439a1806..ab3b8c4c2d5a 100644
> --- a/drivers/vhost/blk.c
> +++ b/drivers/vhost/blk.c

[ ... ]

> @@ -909,6 +909,14 @@ static long vhost_blk_ioctl(struct file *f, unsigned int ioctl,
>  		return vhost_blk_set_features(blk, features);
>  	case VHOST_RESET_OWNER:
>  		return vhost_blk_reset_owner(blk);
> +	case VHOST_NEW_WORKER:
> +	case VHOST_FREE_WORKER:
> +	case VHOST_ATTACH_VRING_WORKER:
> +	case VHOST_GET_VRING_WORKER:
> +		mutex_lock(&blk->dev.mutex);
> +		ret = vhost_worker_ioctl(&blk->dev, ioctl, argp);
> +		mutex_unlock(&blk->dev.mutex);
> +		return ret;

[Severity: Medium]
Can a completed request be stranded here, hanging vhost_blk_flush()?

Once user-created workers can be attached to vhost-blk virtqueues, a vq
can lose its worker. When a worker dies, vhost_worker_killed() does:

    rcu_assign_pointer(vq->worker, NULL);

After that, a bio that completes goes through vhost_blk_req_done():

    if (atomic_dec_and_test(&req->bio_nr)) {
        llist_add(&req->llnode, &req->blk_vq->llhead);
        vhost_vq_work_queue(&req->blk_vq->vq, &req->blk_vq->work);
    }

vhost_vq_work_queue() finds rcu_dereference(vq->worker) == NULL, queues
nothing and returns false. vhost_blk_req_done() ignores that return value.

Only vhost_blk_handle_host_kick() calls vhost_blk_req_cleanup() and
forget_request(), and that work will never run. The request therefore
stays on llhead. Its bio, iov and bounce buffer are not freed, and
req_inflight[] is never decremented.

vhost_blk_flush() then does an untimed wait:

    wait_event(blk->flush_wait,
               !atomic_read(&blk->req_inflight[flush_bin]));

vhost_dev_flush() skips killed workers, because __vhost_worker_flush()
returns early when worker->killed is set, so nothing wakes this wait.

This wait is reachable from several paths:

    vhost_blk_release()->vhost_blk_drop_backend()->vhost_blk_flush()
    vhost_blk_reset_owner()->vhost_blk_drop_backend()->vhost_blk_flush()
    vhost_blk_set_features()->vhost_blk_flush()

The release path can hang, so the backend file reference is not dropped
and the vhost_blk is not freed.

The hang can already occur for the default worker in vhost_task mode.
This patch also exposes it to user-created workers, through
VHOST_NEW_WORKER and VHOST_ATTACH_VRING_WORKER.

Should vhost_blk_req_done() handle a false return from
vhost_vq_work_queue(), for example by cleaning up the request directly?

>  	default:
>  		mutex_lock(&blk->dev.mutex);
>  		ret = vhost_dev_ioctl(&blk->dev, ioctl, argp);

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261029110554.3315693-1-alexander.ivanov@virtuozzo.com?part=1

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel