Re: [PATCH 1/2] vhost-vdpa: don't install the eventfd_ctx_fdget() error in config_ctx

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Lack of synchronization between `vhost_vdpa_set_config_call()` and the config interrupt handler `vhost_vdpa_config_cb()` leads to a Data Race and a Use-After-Free on `v->config_ctx`.
--

--- Patch [1]: [PATCH 1/2] vhost-vdpa: don't install the eventfd_ctx_fdget() error in config_ctx ---
commit 97c24916f7205e9db996343c66b6d1658f9125bd
Author: Yu Zhang <[email protected]>

vhost-vdpa: don't install the eventfd_ctx_fdget() error in config_ctx

This commit avoids storing an error pointer in the configuration context on
failure. It validates the file descriptor prior to swapping it, preventing
config interrupts from encountering an invalid pointer.

> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -536,18 +536,14 @@ static long vhost_vdpa_set_config_call(struct vhost_vdpa *v, u32 __user *argp)
>  		return  -EFAULT;
>  
>  	ctx = fd == VHOST_FILE_UNBIND ? NULL : eventfd_ctx_fdget(fd);
> +	if (IS_ERR(ctx))
> +		return PTR_ERR(ctx);
> +
>  	swap(ctx, v->config_ctx);
>  
> -	if (!IS_ERR_OR_NULL(ctx))
> +	if (ctx)
>  		eventfd_ctx_put(ctx);

[Severity: Critical]
This isn't a bug introduced by this patch, but can this sequence lead to a
use-after-free if a config interrupt occurs concurrently?

In the interrupt handler, the pointer is read locklessly:

drivers/vhost/vdpa.c:vhost_vdpa_config_cb() {
    struct eventfd_ctx *config_ctx = v->config_ctx;

    if (config_ctx)
        eventfd_signal(config_ctx);
    ...
}

If the ioctl thread executing vhost_vdpa_set_config_call() swaps out the old
context and immediately calls eventfd_ctx_put(), it might free the context
while the interrupt handler has already read the old pointer but hasn't yet
called eventfd_signal().

Does this code need synchronization before freeing the old eventfd context to
wait for concurrent interrupt handlers to finish?

> -	if (IS_ERR(v->config_ctx)) {
> -		long ret = PTR_ERR(v->config_ctx);
> -
> -		v->config_ctx = NULL;
> -		return ret;
> -	}
> -
>  	v->vdpa->config->set_config_cb(v->vdpa, &cb);
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1