Re: [PATCH v4 06/16] vfio/pci: Store incoming Live Update state in struct vfio_pci_core_device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing cleanup of `liveupdate_incoming_state` on error paths and device closure
- [High] Data race on `liveupdate_incoming_state` modification
--

commit d8de06a6f6edeb0c9ca334c98f9b961bfa08a639
Author: David Matlack <dmatlack@google.com>

vfio/pci: Store incoming Live Update state in struct vfio_pci_core_device

This commit adds a pointer to the incoming Live Update state in struct
vfio_pci_core_device. It modifies vfio_pci_core_enable() to clear this
pointer on the success path to signal that the state has been consumed.
It also updates vfio_pci_liveupdate_can_finish() to wait for this pointer
to become NULL under the dev_set lock.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 1726d4d41b579..da013b4b1e94d 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -568,7 +568,7 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
>  	if (!vfio_vga_disabled() && vfio_pci_is_vga(pdev))
>  		vdev->has_vga = true;
>  
> -
> +	vdev->liveupdate_incoming_state = NULL;
>  	return 0;
>  
>  out_free_zdev:

If device enablement fails and the code jumps to an error label like
out_free_zdev, liveupdate_incoming_state will not be cleared.  Will this
cause vfio_pci_liveupdate_can_finish() to stall indefinitely waiting for
this pointer to become NULL?

Additionally, if the orchestrator times out and frees the KHO memory,
could this leave a dangling pointer that results in a use-after-free on
subsequent attempts to open the device?

> diff --git a/drivers/vfio/pci/vfio_pci_liveupdate.c b/drivers/vfio/pci/vfio_pci_liveupdate.c
> index 731a3e34085fb..7b841a1e7fcc5 100644
> --- a/drivers/vfio/pci/vfio_pci_liveupdate.c
> +++ b/drivers/vfio/pci/vfio_pci_liveupdate.c
[ ... ]
> @@ -265,6 +266,9 @@ static int vfio_pci_liveupdate_retrieve(struct liveupdate_file_op_args *args)
>  		goto out;
>  	}
>  
> +	vdev = container_of(device, struct vfio_pci_core_device, vdev);
> +	vdev->liveupdate_incoming_state = ser;

The commit message states "This is synchronized by vfio_pci_core.c setting
vdev->liveupdate_incoming_state to NULL under dev_set lock once it's done
using it."

However, the assignment here does not appear to take device->dev_set->lock.

If userspace triggers this retrieve operation concurrently with device
enablement, does this create a data race?  An unprotected write here
could clobber the NULL value just set by a successful device enablement,
leaving the pointer non-NULL and causing vfio_pci_liveupdate_can_finish()
to wait forever.

> +
>  	args->file = file;
>  out:
>  	/* Drop the reference from vfio_find_device() */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511234802.2280368-1-vipinsh@google.com?part=6