[PATCH] PCI: vgaarb: Remove stale "not implemented" notes from docs

[PATCH] PCI: vgaarb: Remove stale "not implemented" notes from docs

[Christian Melendez Nuñez]

The vgaarb kerneldoc and Documentation/gpu/vgaarbiter.rst both
describe "unlock all" and "target default" as "(TODO: not
implemented yet)". Both are actually implemented already:

- "unlock all" is handled in vga_arb_write() via the "all"
  strncmp branch, which releases both VGA_RSRC_LEGACY_IO and
  VGA_RSRC_LEGACY_MEM.
- "target default" is handled in the same function via the
  "default" strncmp branch, which resolves the target to
  vga_default_device().

Drop the stale disclaimers so the documentation matches actual
behavior.

This is my first submission to the kernel; happy to take any
feedback on the patch itself or how I should be doing this.

Signed-off-by: Christian Melendez Nuñez <chrismelnu@gmail.com>
---
 Documentation/gpu/vgaarbiter.rst | 9 ++++-----
 drivers/pci/vgaarb.c             | 7 +++----
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/Documentation/gpu/vgaarbiter.rst b/Documentation/gpu/vgaarbiter.rst
index d1e953712cc2..c3a728130b54 100644
--- a/Documentation/gpu/vgaarbiter.rst
+++ b/Documentation/gpu/vgaarbiter.rst
@@ -65,8 +65,7 @@ write
         unlock <io_state>
                 release locks on target
         unlock all
-                release all locks on target held by this user (not implemented
-                yet)
+                release all locks on target held by this user
         decodes <io_state>
                 set the legacy decoding attributes for the card
 
@@ -74,9 +73,9 @@ write
                 event if something changes on any card (not just the target)
 
         card_ID is of the form "PCI:domain:bus:dev.fn". It can be set to "default"
-        to go back to the system default card (TODO: not implemented yet). Currently,
-        only PCI is supported as a prefix, but the userland API may support other bus
-        types in the future, even if the current kernel implementation doesn't.
+        to go back to the system default card. Currently, only PCI is supported
+        as a prefix, but the userland API may support other bus types in the
+        future, even if the current kernel implementation doesn't.
 
 Note about locks:
 
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index c360eee11dd9..4ae96ae60d8b 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -1008,10 +1008,9 @@ EXPORT_SYMBOL(vga_client_register);
  * poll         : event if something change on any card (not just the target)
  *
  * card_ID is of the form "PCI:domain:bus:dev.fn". It can be set to "default"
- * to go back to the system default card (TODO: not implemented yet).
- * Currently, only PCI is supported as a prefix, but the userland API may
- * support other bus types in the future, even if the current kernel
- * implementation doesn't.
+ * to go back to the system default card. Currently, only PCI is supported
+ * as a prefix, but the userland API may support other bus types in the
+ * future, even if the current kernel implementation doesn't.
  *
  * Note about locks:
  *
-- 
2.34.1

Re: [PATCH] PCI: vgaarb: Remove stale "not implemented" notes from docs

[sashiko-bot]

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

New issues:
- [Medium] The commit incorrectly removes the 'not implemented' TODO for the 'unlock all' command, even though its implementation remains fundamentally broken.

Pre-existing issues:
- [High] Use-After-Free in vga_arb_release due to unreferenced pci_dev pointers.
- [High] Permanent global VGA lock resource leak when MAX_USER_CARDS is exceeded.
- [High] Data race and Use-After-Free during unprotected vga_list iteration and concurrent FD accesses.
--

--- Patch [1]: [PATCH] PCI: vgaarb: Remove stale "not implemented" notes from docs ---
commit bbd78d4ec38a7ceee963b0e74ebab5896fceb191
Author: Christian Melendez Nuñez <chrismelnu@gmail.com>

PCI: vgaarb: Remove stale "not implemented" notes from docs

This commit updates the vgaarb documentation and kerneldoc to remove
"TODO: not implemented yet" notes for the "unlock all" and "target
default" commands. It states that these commands are already fully handled
in vga_arb_write().

> - "unlock all" is handled in vga_arb_write() via the "all"
>   strncmp branch, which releases both VGA_RSRC_LEGACY_IO and
>   VGA_RSRC_LEGACY_MEM.

[Severity: Medium]
Is the "unlock all" command fully implemented and working as expected?

Looking at vga_arb_write(), when the "all" branch is taken, it forces both
flags:

drivers/pci/vgaarb.c:vga_arb_write() {
    ...
    if (strncmp(curr_pos, "all", 3) == 0)
        io_state = VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM;
    ...
}

But later in the same function, it checks if the user actually holds these
locks:

drivers/pci/vgaarb.c:vga_arb_write() {
    ...
    if (io_state & VGA_RSRC_LEGACY_MEM && uc->mem_cnt == 0) {
        ret_val = -EINVAL;
        goto done;
    }
    ...
}

If the user holds only one type of lock, wouldn't this fail with -EINVAL
instead of unlocking the resources? Should the TODO disclaimer remain until
this logic is fixed?


> diff --git a/Documentation/gpu/vgaarbiter.rst b/Documentation/gpu/vgaarbiter.rst
> index d1e953712cc29..c3a728130b545 100644
> --- a/Documentation/gpu/vgaarbiter.rst
> +++ b/Documentation/gpu/vgaarbiter.rst
> @@ -65,8 +65,7 @@ write
>          unlock <io_state>
>                  release locks on target
>          unlock all
> -                release all locks on target held by this user (not implemented
> -                yet)
> +                release all locks on target held by this user

[Severity: High]
This is a pre-existing issue, but does closing the file descriptor lead to a
use-after-free here?

In vga_arb_write(), when a "target" command is processed, the code drops the
reference to the PCI device:

drivers/pci/vgaarb.c:vga_arb_write() {
    ...
    pci_dev_put(pdev);
    ...
}

If the target PCI device is subsequently hot-unplugged and freed by the PCI
subsystem, the pointers in priv->target and priv->cards[i].pdev are left
dangling. 

When the user later closes the file descriptor, vga_arb_release() accesses
the device unconditionally:

drivers/pci/vgaarb.c:vga_arb_release() {
    ...
    vgaarb_dbg(&uc->pdev->dev, "uc->io_cnt == %d, uc->mem_cnt == %d\n",
               uc->io_cnt, uc->mem_cnt);
    ...
}

Could this dereference the freed pointer if the device was removed?


[Severity: High]
This is a pre-existing issue, but can we permanently leak a global VGA lock
if MAX_USER_CARDS is exceeded?

When vga_arb_write() processes the "target" command, it sets priv->target
before checking if the tracking array is full:

drivers/pci/vgaarb.c:vga_arb_write() {
    ...
    priv->target = pdev;
    for (i = 0; i < MAX_USER_CARDS; i++) {
        ...
    if (i == MAX_USER_CARDS) {
        ...
        ret_val =  -ENOMEM;
        goto done;
    }
    ...
}

If the user has 16 devices and targets a 17th, the function returns -ENOMEM
but leaves priv->target set to the new device.

If the user then sends a "lock io" command, it will acquire the global lock
for this new device. Because the device isn't tracked in priv->cards, does
vga_arb_release() fail to release this lock on close, leaking it permanently?


[Severity: High]
This is a pre-existing issue, but is it safe to traverse the global vga_list
without holding vga_lock?

In vga_arb_write(), when resolving a target device, it calls vgadev_find():

drivers/pci/vgaarb.c:vga_arb_write() {
    ...
    vgadev = vgadev_find(pdev);
    ...
}

Which traverses the global list:

drivers/pci/vgaarb.c:vgadev_find() {
    ...
    list_for_each_entry(vgadev, &vga_list, list) {
        ...
}

If a VGA device is concurrently hot-plugged or unplugged, could this
concurrent modification cause the iteration to dereference freed list nodes?

Additionally, should concurrent accesses to the user's private struct,
such as priv->target and priv->cards in vga_arb_write(), be protected by
priv->lock to prevent data corruption when accessed by multiple threads?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807200639.172487-1-chrismelnu@gmail.com?part=1

Re: [PATCH] PCI: vgaarb: Remove stale "not implemented" notes from docs

[Christian Melendez Nuñez]

> [Severity: Medium]
> Is the "unlock all" command fully implemented and working as expected?
>
> If the user holds only one type of lock, wouldn't this fail with -EINVAL
> instead of unlocking the resources? Should the TODO disclaimer remain
> until this logic is fixed?

This is correct.  "unlock all" sets both VGA_RSRC_LEGACY_IO and
VGA_RSRC_LEGACY_MEM unconditionally:

    if (strncmp(curr_pos, "all", 3) == 0)
        io_state = VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM;

but "lock" allows acquiring them independently via vga_str_to_iostate(),
so a caller holding only one of them hits

    if (io_state & VGA_RSRC_LEGACY_MEM && uc->mem_cnt == 0) {
        ret_val = -EINVAL;
        goto done;
    }

and returns -EINVAL without releasing the lock it does hold. So "release
all locks on target held by this user" is not accurate unless both are
held, and the caveat should stay.

I'll wait for further review before sending a v2.

Thanks,
Christian