Re: [PATCH] PCI/VGA: Fix lost wakeup when waiting for a VGA resource

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

Pre-existing issues:
- [High] vga_tryget() incorrectly masks actual error codes from __vga_tryget() into -EBUSY, and its caller vgaarb_write() has inverted logic that treats this error as success.
- [High] Missing rollback in __vga_tryget() leaves conflicting devices permanently disabled on error.
--

--- Patch [1]: [PATCH] PCI/VGA: Fix lost wakeup when waiting for a VGA resource ---
commit 14621d6221b003eab582f677ca7cbfeb78dd54ba
Author: FAN YE <[email protected]>

PCI/VGA: Fix lost wakeup when waiting for a VGA resource
    
This commit addresses a lost wakeup issue in vga_get() when waiting for a
conflicting device to release its resource. It ensures the task is added to
the wait queue before dropping the vga_lock so that a concurrent wakeup
doesn't slip past.

Cc: [email protected]

> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index 3de05aee78599..51c7d171c9558 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -459,6 +459,23 @@ int vga_get(struct pci_dev *pdev, unsigned int rsrc, int interruptible)
>  			break;
>  		}
>  		conflict = __vga_tryget(vgadev, rsrc);

[Severity: High]
This is a pre-existing issue, but does __vga_tryget() fail to roll back the
state of already-disabled conflicting devices if a subsequent operation
fails?

In drivers/pci/vgaarb.c:__vga_tryget():

    err = pci_set_vga_state(conflict->pdev, false, pci_bits, flags);
    if (err)
        return ERR_PTR(err);

If this fails midway through disabling multiple devices, or fails when
enabling the target device later in the function, it returns ERR_PTR(err)
without reverting the disabled devices. Could this leave those GPUs
permanently disabled with corrupted arbiter state?

> +		/*
> +		 * We have a conflict; we wait until somebody kicks the
> +		 * work queue. Currently we have one work queue that we
> +		 * kick each time some resources are released, but it would
> +		 * be fairly easy to have a per-device one so that we only
> +		 * need to attach to the conflicting device.
> +		 *
> +		 * Queue up before dropping vga_lock: __vga_put() wakes the
> +		 * queue while holding it, so a wakeup cannot slip past.
> +		 */
> +		if (!IS_ERR_OR_NULL(conflict)) {
> +			init_waitqueue_entry(&wait, current);
> +			add_wait_queue(&vga_wait_queue, &wait);
> +			set_current_state(interruptible ?
> +					  TASK_INTERRUPTIBLE :
> +					  TASK_UNINTERRUPTIBLE);
> +		}
>  		spin_unlock_irqrestore(&vga_lock, flags);
>  		if (IS_ERR(conflict)) {

[Severity: High]
This isn't a bug introduced by this patch, but since we are handling
__vga_tryget() errors here, does vga_tryget() incorrectly mask actual error
returns from __vga_tryget() into -EBUSY?

In drivers/pci/vgaarb.c:vga_tryget():

    if (__vga_tryget(vgadev, rsrc))
        rc = -EBUSY;

And does the caller vgaarb_write() then treat this error as a success?

In drivers/pci/vgaarb.c:vgaarb_write():

    if (vga_tryget(pdev, io_state)) {
        /* Update the client's locks lists... */

Since vga_tryget() returns -EBUSY on failure, the non-zero return causes
vgaarb_write() to evaluate the condition as true. Can this false success
lead to userspace incorrectly acting as if the lock was acquired, causing
dangerous concurrent VGA access?

>  			rc = PTR_ERR(conflict);
>  			break;
>  		}

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