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

[Jani Nikula <jani.nikula@linux.intel.com>]

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