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 <[email protected]> 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/[email protected]?part=1
