[PATCH] PCI/VGA: fix inverted vga_tryget() return check and clear removed pci_dev

[PATCH] PCI/VGA: fix inverted vga_tryget() return check and clear removed pci_dev

[Hui Peng]

Fix two issues in drivers/pci/vgaarb.c:

1. In vga_arb_device_init() / vga_arbiter_From_pci_dev(), vga_tryget()
   returns 0 on success and a negative errno on failure, so checking if
   (!vga_tryget(...)) is inverted and skips unlocking on success.
2. In vga_arbiter_del_pci_device(), clear any open vga_arb_user_init
   priv->target references pointing to the removed pci_dev so subsequent
   writes or close() on /dev/vga_arbiter do not dereference a freed
   pci_dev.

Fixes: 1d38fe6ee6a8 ("PCI/VGA: Move vgaarb to drivers/pci")
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 3de05aee7859..4f13e542505a 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -818,11 +818,33 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
 	return false;
 }
 
+#define MAX_USER_CARDS         CONFIG_VGA_ARB_MAX_GPUS
+#define PCI_INVALID_CARD       ((struct pci_dev *)-1UL)
+
+/* Each user has an array of these, tracking which cards have locks */
+struct vga_arb_user_card {
+	struct pci_dev *pdev;
+	unsigned int mem_cnt;
+	unsigned int io_cnt;
+};
+
+struct vga_arb_private {
+	struct list_head list;
+	struct pci_dev *target;
+	struct vga_arb_user_card cards[MAX_USER_CARDS];
+	spinlock_t lock;
+};
+
+static LIST_HEAD(vga_user_list);
+static DEFINE_SPINLOCK(vga_user_lock);
+
 static bool vga_arbiter_del_pci_device(struct pci_dev *pdev)
 {
 	struct vga_device *vgadev;
+	struct vga_arb_private *priv;
 	unsigned long flags;
 	bool ret = true;
+	int i;
 
 	spin_lock_irqsave(&vga_lock, flags);
 	vgadev = vgadev_find(pdev);
@@ -845,6 +867,21 @@ static bool vga_arbiter_del_pci_device(struct pci_dev *pdev)
 	wake_up_all(&vga_wait_queue);
 bail:
 	spin_unlock_irqrestore(&vga_lock, flags);
+	if (ret) {
+		spin_lock_irqsave(&vga_user_lock, flags);
+		list_for_each_entry(priv, &vga_user_list, list) {
+			if (priv->target == pdev)
+				priv->target = PCI_INVALID_CARD;
+			for (i = 0; i < MAX_USER_CARDS; i++) {
+				if (priv->cards[i].pdev == pdev) {
+					priv->cards[i].pdev = PCI_INVALID_CARD;
+					priv->cards[i].io_cnt = 0;
+					priv->cards[i].mem_cnt = 0;
+				}
+			}
+		}
+		spin_unlock_irqrestore(&vga_user_lock, flags);
+	}
 	kfree(vgadev);
 	return ret;
 }
@@ -1025,27 +1062,6 @@ EXPORT_SYMBOL(vga_client_register);
  * the arbiter.
  */
 
-#define MAX_USER_CARDS         CONFIG_VGA_ARB_MAX_GPUS
-#define PCI_INVALID_CARD       ((struct pci_dev *)-1UL)
-
-/* Each user has an array of these, tracking which cards have locks */
-struct vga_arb_user_card {
-	struct pci_dev *pdev;
-	unsigned int mem_cnt;
-	unsigned int io_cnt;
-};
-
-struct vga_arb_private {
-	struct list_head list;
-	struct pci_dev *target;
-	struct vga_arb_user_card cards[MAX_USER_CARDS];
-	spinlock_t lock;
-};
-
-static LIST_HEAD(vga_user_list);
-static DEFINE_SPINLOCK(vga_user_lock);
-
-
 /*
  * Take a string in the format: "PCI:domain:bus:dev.fn" and return the
  * respective values. If the string is not in this format, return 0.
@@ -1168,7 +1184,7 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
 		}
 
 		pdev = priv->target;
-		if (priv->target == NULL) {
+		if (priv->target == NULL || priv->target == PCI_INVALID_CARD) {
 			ret_val = -ENODEV;
 			goto done;
 		}
@@ -1215,7 +1231,7 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
 		}
 
 		pdev = priv->target;
-		if (priv->target == NULL) {
+		if (priv->target == NULL || priv->target == PCI_INVALID_CARD) {
 			ret_val = -ENODEV;
 			goto done;
 		}
@@ -1266,12 +1282,12 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
 		 */
 
 		pdev = priv->target;
-		if (priv->target == NULL) {
+		if (priv->target == NULL || priv->target == PCI_INVALID_CARD) {
 			ret_val = -ENODEV;
 			goto done;
 		}
 
-		if (vga_tryget(pdev, io_state)) {
+		if (vga_tryget(pdev, io_state) == 0) {
 			/* Update the client's locks lists... */
 			for (i = 0; i < MAX_USER_CARDS; i++) {
 				if (priv->cards[i].pdev == pdev) {
@@ -1335,7 +1351,8 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
 		for (i = 0; i < MAX_USER_CARDS; i++) {
 			if (priv->cards[i].pdev == pdev)
 				break;
-			if (priv->cards[i].pdev == NULL) {
+			if (priv->cards[i].pdev == NULL ||
+			    priv->cards[i].pdev == PCI_INVALID_CARD) {
 				priv->cards[i].pdev = pdev;
 				priv->cards[i].io_cnt = 0;
 				priv->cards[i].mem_cnt = 0;
@@ -1366,7 +1383,7 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
 			goto done;
 		}
 		pdev = priv->target;
-		if (priv->target == NULL) {
+		if (priv->target == NULL || priv->target == PCI_INVALID_CARD) {
 			ret_val = -ENODEV;
 			goto done;
 		}
@@ -1429,7 +1446,7 @@ static int vga_arb_release(struct inode *inode, struct file *file)
 	list_del(&priv->list);
 	for (i = 0; i < MAX_USER_CARDS; i++) {
 		uc = &priv->cards[i];
-		if (uc->pdev == NULL)
+		if (uc->pdev == NULL || uc->pdev == PCI_INVALID_CARD)
 			continue;
 		vgaarb_dbg(&uc->pdev->dev, "uc->io_cnt == %d, uc->mem_cnt == %d\n",
 			uc->io_cnt, uc->mem_cnt);

Re: [PATCH] PCI/VGA: fix inverted vga_tryget() return check and clear removed pci_dev

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] commit message references incorrect functions for vga_tryget() fix
--

commit 341d48fbd447d6890560b76963b8d28bd58bbe29
Author: Hui Peng <benquike@gmail.com>

PCI/VGA: fix inverted vga_tryget() return check and clear removed pci_dev

This commit fixes an inverted return check for vga_tryget() and clears open
references pointing to a removed pci_dev in vga_arbiter_del_pci_device()
to prevent dereferencing a freed pci_dev.

>     1. In vga_arb_device_init() / vga_arbiter_From_pci_dev(), vga_tryget()
>        returns 0 on success and a negative errno on failure, so checking if
>        (!vga_tryget(...)) is inverted and skips unlocking on success.

[Severity: Medium]
The commit message references incorrect functions for the vga_tryget() fix.

Reading the commit message suggests looking at vga_arb_device_init() or
vga_arbiter_From_pci_dev() for the vga_tryget() fix. However,
vga_arb_device_init() does not call vga_tryget(), and
vga_arbiter_From_pci_dev() does not exist in the codebase.

The actual code modified to fix the inverted return check is in
vga_arb_write().

Could the commit message be updated to accurately reflect the function
where the fix is applied?

> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index 3de05aee78599..4f13e542505ad 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
[ ... ]
> @@ -1266,12 +1282,12 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
>  		 */
>  
>  		pdev = priv->target;
> -		if (priv->target == NULL) {
> +		if (priv->target == NULL || priv->target == PCI_INVALID_CARD) {
>  			ret_val = -ENODEV;
>  			goto done;
>  		}
>  
> -		if (vga_tryget(pdev, io_state)) {
> +		if (vga_tryget(pdev, io_state) == 0) {
>  			/* Update the client's locks lists... */

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

Re: [PATCH] PCI/VGA: fix inverted vga_tryget() return check and clear removed pci_dev

[Jani Nikula]

On Sat, 19 Sep 2026, Hui Peng <benquike@gmail.com> wrote:
> Fix two issues in drivers/pci/vgaarb.c:

One change per patch, please.

>
> 1. In vga_arb_device_init() / vga_arbiter_From_pci_dev(), vga_tryget()
>    returns 0 on success and a negative errno on failure, so checking if
>    (!vga_tryget(...)) is inverted and skips unlocking on success.
> 2. In vga_arbiter_del_pci_device(), clear any open vga_arb_user_init
>    priv->target references pointing to the removed pci_dev so subsequent
>    writes or close() on /dev/vga_arbiter do not dereference a freed
>    pci_dev.
>
> Fixes: 1d38fe6ee6a8 ("PCI/VGA: Move vgaarb to drivers/pci")
> Assisted-by: LLM
> Signed-off-by: Hui Peng <benquike@gmail.com>
> ---
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index 3de05aee7859..4f13e542505a 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -818,11 +818,33 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
>  	return false;
>  }
>  
> +#define MAX_USER_CARDS         CONFIG_VGA_ARB_MAX_GPUS
> +#define PCI_INVALID_CARD       ((struct pci_dev *)-1UL)
> +
> +/* Each user has an array of these, tracking which cards have locks */
> +struct vga_arb_user_card {
> +	struct pci_dev *pdev;
> +	unsigned int mem_cnt;
> +	unsigned int io_cnt;
> +};
> +
> +struct vga_arb_private {
> +	struct list_head list;
> +	struct pci_dev *target;
> +	struct vga_arb_user_card cards[MAX_USER_CARDS];
> +	spinlock_t lock;
> +};
> +
> +static LIST_HEAD(vga_user_list);
> +static DEFINE_SPINLOCK(vga_user_lock);
> +
>  static bool vga_arbiter_del_pci_device(struct pci_dev *pdev)
>  {
>  	struct vga_device *vgadev;
> +	struct vga_arb_private *priv;
>  	unsigned long flags;
>  	bool ret = true;
> +	int i;
>  
>  	spin_lock_irqsave(&vga_lock, flags);
>  	vgadev = vgadev_find(pdev);
> @@ -845,6 +867,21 @@ static bool vga_arbiter_del_pci_device(struct pci_dev *pdev)
>  	wake_up_all(&vga_wait_queue);
>  bail:
>  	spin_unlock_irqrestore(&vga_lock, flags);
> +	if (ret) {
> +		spin_lock_irqsave(&vga_user_lock, flags);
> +		list_for_each_entry(priv, &vga_user_list, list) {
> +			if (priv->target == pdev)
> +				priv->target = PCI_INVALID_CARD;
> +			for (i = 0; i < MAX_USER_CARDS; i++) {
> +				if (priv->cards[i].pdev == pdev) {
> +					priv->cards[i].pdev = PCI_INVALID_CARD;
> +					priv->cards[i].io_cnt = 0;
> +					priv->cards[i].mem_cnt = 0;
> +				}
> +			}
> +		}
> +		spin_unlock_irqrestore(&vga_user_lock, flags);
> +	}
>  	kfree(vgadev);
>  	return ret;
>  }
> @@ -1025,27 +1062,6 @@ EXPORT_SYMBOL(vga_client_register);
>   * the arbiter.
>   */
>  
> -#define MAX_USER_CARDS         CONFIG_VGA_ARB_MAX_GPUS
> -#define PCI_INVALID_CARD       ((struct pci_dev *)-1UL)
> -
> -/* Each user has an array of these, tracking which cards have locks */
> -struct vga_arb_user_card {
> -	struct pci_dev *pdev;
> -	unsigned int mem_cnt;
> -	unsigned int io_cnt;
> -};
> -
> -struct vga_arb_private {
> -	struct list_head list;
> -	struct pci_dev *target;
> -	struct vga_arb_user_card cards[MAX_USER_CARDS];
> -	spinlock_t lock;
> -};
> -
> -static LIST_HEAD(vga_user_list);
> -static DEFINE_SPINLOCK(vga_user_lock);
> -
> -
>  /*
>   * Take a string in the format: "PCI:domain:bus:dev.fn" and return the
>   * respective values. If the string is not in this format, return 0.
> @@ -1168,7 +1184,7 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
>  		}
>  
>  		pdev = priv->target;
> -		if (priv->target == NULL) {
> +		if (priv->target == NULL || priv->target == PCI_INVALID_CARD) {
>  			ret_val = -ENODEV;
>  			goto done;
>  		}
> @@ -1215,7 +1231,7 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
>  		}
>  
>  		pdev = priv->target;
> -		if (priv->target == NULL) {
> +		if (priv->target == NULL || priv->target == PCI_INVALID_CARD) {
>  			ret_val = -ENODEV;
>  			goto done;
>  		}
> @@ -1266,12 +1282,12 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
>  		 */
>  
>  		pdev = priv->target;
> -		if (priv->target == NULL) {
> +		if (priv->target == NULL || priv->target == PCI_INVALID_CARD) {
>  			ret_val = -ENODEV;
>  			goto done;
>  		}
>  
> -		if (vga_tryget(pdev, io_state)) {
> +		if (vga_tryget(pdev, io_state) == 0) {
>  			/* Update the client's locks lists... */
>  			for (i = 0; i < MAX_USER_CARDS; i++) {
>  				if (priv->cards[i].pdev == pdev) {
> @@ -1335,7 +1351,8 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
>  		for (i = 0; i < MAX_USER_CARDS; i++) {
>  			if (priv->cards[i].pdev == pdev)
>  				break;
> -			if (priv->cards[i].pdev == NULL) {
> +			if (priv->cards[i].pdev == NULL ||
> +			    priv->cards[i].pdev == PCI_INVALID_CARD) {
>  				priv->cards[i].pdev = pdev;
>  				priv->cards[i].io_cnt = 0;
>  				priv->cards[i].mem_cnt = 0;
> @@ -1366,7 +1383,7 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
>  			goto done;
>  		}
>  		pdev = priv->target;
> -		if (priv->target == NULL) {
> +		if (priv->target == NULL || priv->target == PCI_INVALID_CARD) {
>  			ret_val = -ENODEV;
>  			goto done;
>  		}
> @@ -1429,7 +1446,7 @@ static int vga_arb_release(struct inode *inode, struct file *file)
>  	list_del(&priv->list);
>  	for (i = 0; i < MAX_USER_CARDS; i++) {
>  		uc = &priv->cards[i];
> -		if (uc->pdev == NULL)
> +		if (uc->pdev == NULL || uc->pdev == PCI_INVALID_CARD)
>  			continue;
>  		vgaarb_dbg(&uc->pdev->dev, "uc->io_cnt == %d, uc->mem_cnt == %d\n",
>  			uc->io_cnt, uc->mem_cnt);

-- 
Jani Nikula, Intel

Re: [PATCH] PCI/VGA: fix inverted vga_tryget() return check and clear removed pci_dev

[krzk]

On Sat, 19 Sep 2026 22:26:29 +0000, Hui Peng wrote:
> Fix two issues in drivers/pci/vgaarb.c:
> 
> 1. In vga_arb_device_init() / vga_arbiter_From_pci_dev(), vga_tryget()
>    returns 0 on success and a negative errno on failure, so checking if
>    (!vga_tryget(...)) is inverted and skips unlocking on success.
> 2. In vga_arbiter_del_pci_device(), clear any open vga_arb_user_init
>    priv->target references pointing to the removed pci_dev so subsequent
>    writes or close() on /dev/vga_arbiter do not dereference a freed
>    pci_dev.
> 
> Fixes: 1d38fe6ee6a8 ("PCI/VGA: Move vgaarb to drivers/pci")
> Assisted-by: LLM
> Signed-off-by: Hui Peng <benquike@gmail.com>
> ---


You sent multiple independent patches, to multiple independent
subsystems. The amount of these patches clearly suggest this was
AI generated and most likely not tested.

More importantly, you sent all this work without properly organizing
relevant patches into patchsets. This makes reviewing difficult
and might cause multiple reviewers to address the same issue.
Replying to the entire set is impossible and requires handling each
patch independently, instead of applying or discarding the set.
Maintainers also won't see the bigger picture of your work. Quite
worrying.

This is on the verge of hostile patch: bomb us with so many
contributions, we won't be able to handle them in efficient manner,
like responding ONCE to ask you to slow down.  Considering all this
is untested and LLM generated, I have even more doubts whether this
should be considered for review.

Please read kernel documentation BEFORE posting more work. It will
explain you how to identify subsystems, how to organize your work per
subsystem, how to document usage of LLM and how what you should not
do if this was posted in a good faith.

Best regards,
Krzysztof