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

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

[FAN YE via B4 Relay]

From: FAN YE <fy15309206903@gmail.com>

A task waiting in vga_get() can sleep forever even though the conflicting
device has already released the resource.  Once __vga_tryget() reports a
conflict, vga_get() drops vga_lock and only then puts itself on
vga_wait_queue, while __vga_put() wakes that queue while holding vga_lock.
A wakeup landing in between finds the queue empty and is discarded, and as
the conflict is already gone no further wakeup is coming.  Callers passing
interruptible=0, such as the "lock" command of /dev/vga_arbiter, are then
unkillable and keep their lock counts forever.

Queue up before dropping vga_lock, the way prepare_to_wait() publishes a
waiter before the condition is re-tested.  The releasing side needs
vga_lock to reach the wakeup, so it can no longer pass an unqueued waiter.

Fixes: deb2d2ecd43d ("PCI/GPU: implement VGA arbitration on Linux")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5
Signed-off-by: FAN YE <fy15309206903@gmail.com>
---
Reproduced under QEMU on 818bebeb63dd with two VGA devices, the second one
behind a bridge that does not forward VGA so that it owns no legacy resources.
Two /dev/vga_arbiter clients each locking one device wedged the second client
in vga_get(), D state and unkillable, after 8538 rounds, while the conflicting
device already read back locks=none(0:0).  40000 rounds with this patch, none.
---
 drivers/pci/vgaarb.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

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);
+		/*
+		 * 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)) {
 			rc = PTR_ERR(conflict);
@@ -467,18 +484,6 @@ int vga_get(struct pci_dev *pdev, unsigned int rsrc, int interruptible)
 		if (conflict == NULL)
 			break;
 
-		/*
-		 * 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.
-		 */
-		init_waitqueue_entry(&wait, current);
-		add_wait_queue(&vga_wait_queue, &wait);
-		set_current_state(interruptible ?
-				  TASK_INTERRUPTIBLE :
-				  TASK_UNINTERRUPTIBLE);
 		if (interruptible && signal_pending(current)) {
 			__set_current_state(TASK_RUNNING);
 			remove_wait_queue(&vga_wait_queue, &wait);

---
base-commit: 818bebeb63dd6bf5f4e07e145f6cdbace520a34c
change-id: 20260821-pci-vga-lost-wakeup-3b70658424e1

Best regards,
--  
FAN YE <fy15309206903@gmail.com>

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

[sashiko-bot]

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 <fy15309206903@gmail.com>

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: stable@vger.kernel.org

> 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/20260821-pci-vga-lost-wakeup-v1-1-39dc6a2333c6@gmail.com?part=1