Re: [PATCH net] net: vmxnet3: unwind partial IRQ setup

[Simon Horman <horms@kernel.org>]

On Tue, Sep 22, 2026 at 07:07:10PM +0800, Runyu Xiao wrote:
> Track which interrupt vectors successfully acquired an IRQ handler and
> release that subset when a later request fails. This prevents activation
> error paths from leaving handlers registered while queue resources are
> being torn down.
> 
> Reproducer:
> 
>   Build an x86_64 kernel with CONFIG_PCI=y, CONFIG_PCI_MSI=y,
>   CONFIG_NET=y, CONFIG_NETDEVICES=y, and CONFIG_VMXNET3=m. For testing,
>   add a test-only wrapper around vmxnet3_request_irqs() that lets the
>   first request_irq() succeed and returns -EBUSY for the second request.
>   Boot QEMU with a vmxnet3 device, for example:
> 
>     qemu-system-x86_64 -machine pc -m 1G -smp 2 -nodefaults \
>       -no-reboot -display none -serial file:console.log \
>       -kernel arch/x86/boot/bzImage -initrd test.cpio.gz \
>       -append 'console=ttyS0 rdinit=/init loglevel=7 panic=1' \
>       -netdev user,id=n0 -device vmxnet3,netdev=n0
> 
>   In the guest, load the driver and open the interface:
> 
>     insmod vmxnet3.ko
>     ip link set dev eth0 up
>     cat /proc/interrupts
> 
>   The injected second request makes the open fail with -EBUSY. On the
>   unfixed kernel, the first handler remains registered and
>   /proc/interrupts contains an eth0-rxtx-0 entry. On the fixed kernel,
>   the same failure leaves no vmxnet3 IRQ entry. The injection is
>   deliberate to exercise the partial-registration path and is not a
>   claim that ordinary interface activation fails this way.
> 
> Fixes: d1a890fa37f2 ("net: VMware virtual Ethernet NIC driver: vmxnet3")
> Cc: stable@vger.kernel.org

I am not at all sure that failures can occur in practice.
And while I'm happy to be convinced otherwise, if this
is a theoretical bug, then I think it is best handled
as a patch for net-next (without a Fixes tag or CC to stable).

> Assisted-by: LLM
> Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
> ---
>  drivers/net/vmxnet3/vmxnet3_drv.c | 38 ++++++++++++++++++++++++-------
>  drivers/net/vmxnet3/vmxnet3_int.h |  1 +
>  2 files changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c b/drivers/net/vmxnet3/vmxnet3_drv.c
> index f8df83f99..05373ba4d 100644
> --- a/drivers/net/vmxnet3/vmxnet3_drv.c
> +++ b/drivers/net/vmxnet3/vmxnet3_drv.c
> @@ -53,6 +53,9 @@ static int enable_mq = 1;
>  static void
>  vmxnet3_write_mac_addr(struct vmxnet3_adapter *adapter, const u8 *mac);
>  
> +static void
> +vmxnet3_free_irqs(struct vmxnet3_adapter *adapter);
> +
>  /*
>   *    Enable/Disable the given intr
>   */
> @@ -2585,8 +2588,11 @@ vmxnet3_request_irqs(struct vmxnet3_adapter *adapter)
>  					"Failed to request irq for MSIX, %s, "
>  					"error %d\n",
>  					adapter->tx_queue[i].name, err);
> +				vmxnet3_free_irqs(adapter);
>  				return err;
>  			}
> +			if (adapter->share_intr != VMXNET3_INTR_BUDDYSHARE)
> +				intr->irq_requested[vector] = true;
>  
>  			/* Handle the case where only 1 MSIx was allocated for
>  			 * all tx queues */
> @@ -2620,8 +2626,10 @@ vmxnet3_request_irqs(struct vmxnet3_adapter *adapter)
>  					   "Failed to request irq for MSIX, "
>  					   "%s, error %d\n",
>  					   adapter->rx_queue[i].name, err);
> +				vmxnet3_free_irqs(adapter);
>  				return err;
>  			}
> +			intr->irq_requested[vector] = true;
>  
>  			adapter->rx_queue[i].comp_ring.intr_idx = vector++;
>  		}
> @@ -2631,6 +2639,8 @@ vmxnet3_request_irqs(struct vmxnet3_adapter *adapter)
>  		err = request_irq(intr->msix_entries[vector].vector,
>  				  vmxnet3_msix_event, 0,
>  				  intr->event_msi_vector_name, adapter->netdev);
> +		if (!err)
> +			intr->irq_requested[vector] = true;
>  		intr->event_intr_idx = vector;
>  
>  	} else if (intr->type == VMXNET3_IT_MSI) {
> @@ -2646,11 +2656,14 @@ vmxnet3_request_irqs(struct vmxnet3_adapter *adapter)
>  #ifdef CONFIG_PCI_MSI
>  	}
>  #endif
> +	if (intr->type != VMXNET3_IT_MSIX && !err)
> +		intr->irq_requested[0] = true;
>  	intr->num_intrs = vector + 1;
>  	if (err) {
>  		netdev_err(adapter->netdev,
>  			   "Failed to request irq (intr type:%d), error %d\n",
>  			   intr->type, err);
> +		vmxnet3_free_irqs(adapter);
>  	} else {
>  		/* Number of rx queues will not change after this */
>  		for (i = 0; i < adapter->num_rx_queues; i++) {
> @@ -2693,29 +2706,38 @@ vmxnet3_free_irqs(struct vmxnet3_adapter *adapter)
>  
>  		if (adapter->share_intr != VMXNET3_INTR_BUDDYSHARE) {
>  			for (i = 0; i < adapter->num_tx_queues; i++) {
> -				free_irq(intr->msix_entries[vector++].vector,
> -					 &(adapter->tx_queue[i]));
> +				if (intr->irq_requested[vector])
> +					free_irq(intr->msix_entries[vector].vector,
> +						 &adapter->tx_queue[i]);
> +				intr->irq_requested[vector++] = false;
>  				if (adapter->share_intr == VMXNET3_INTR_TXSHARE)
>  					break;
>  			}
>  		}
>  
>  		for (i = 0; i < adapter->num_rx_queues; i++) {
> -			free_irq(intr->msix_entries[vector++].vector,
> -				 &(adapter->rx_queue[i]));
> +			if (intr->irq_requested[vector])
> +				free_irq(intr->msix_entries[vector].vector,
> +					 &adapter->rx_queue[i]);
> +			intr->irq_requested[vector++] = false;
>  		}
>  
> -		free_irq(intr->msix_entries[vector].vector,
> -			 adapter->netdev);
> +		if (intr->irq_requested[vector])
> +			free_irq(intr->msix_entries[vector].vector, adapter->netdev);
> +		intr->irq_requested[vector] = false;
>  		BUG_ON(vector >= intr->num_intrs);
>  		break;
>  	}
>  #endif
>  	case VMXNET3_IT_MSI:
> -		free_irq(adapter->pdev->irq, adapter->netdev);
> +		if (intr->irq_requested[0])
> +			free_irq(adapter->pdev->irq, adapter->netdev);
> +		intr->irq_requested[0] = false;
>  		break;
>  	case VMXNET3_IT_INTX:
> -		free_irq(adapter->pdev->irq, adapter->netdev);
> +		if (intr->irq_requested[0])
> +			free_irq(adapter->pdev->irq, adapter->netdev);
> +		intr->irq_requested[0] = false;
>  		break;
>  	default:
>  		BUG();

> diff --git a/drivers/net/vmxnet3/vmxnet3_int.h b/drivers/net/vmxnet3/vmxnet3_int.h
> index 9f24d66db..93c545eb6 100644
> --- a/drivers/net/vmxnet3/vmxnet3_int.h
> +++ b/drivers/net/vmxnet3/vmxnet3_int.h
> @@ -362,6 +362,7 @@ struct vmxnet3_intr {
>  	enum vmxnet3_intr_type       type;	/* MSI-X, MSI, or INTx? */
>  	u8  num_intrs;			/* # of intr vectors */
>  	u8  event_intr_idx;		/* idx of the intr vector for event */
> +	bool irq_requested[VMXNET3_LINUX_MAX_MSIX_VECT];

This isn't the central point I want to make, but if you take
this approach then perhaps a bitmap could be considered
rather than an array of bool.

>  	u8  mod_levels[VMXNET3_LINUX_MAX_MSIX_VECT]; /* moderation level */
>  	char	event_msi_vector_name[IFNAMSIZ+17];
>  #ifdef CONFIG_PCI_MSI
> -- 
> 2.34.1

The first main point I want to make is this: vmxnet3_free_irqs() tries to
do too much.  It is is a long function with nested if conditions. And it
doesn't follow the basic flow of keeping error paths in conditions and the
main thread of execution outside of those conditions:

	err = do_something_awesome();
	if (err)
		/* Oops, it wasn't so awesome after all */

	/* Main thread of execution continues here */

Probably a lot of clarity would be achieved by cleaning-up the above.
But not without the risk of adding bugs :(

The second point I want to make is that, as a wise person once told me,
relying on generic clean-up-everything functions leads to errors. And while
it feels like duplication, it's usually cleaner to simply unwind using a
goto ladder.

I think this is the case here. And I'd suggest an approach similar
to the following (compile tested only; please check for out-by-one mistakes).

diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c b/drivers/net/vmxnet3/vmxnet3_drv.c
index f8df83f9965d..618850ba7918 100644
--- a/drivers/net/vmxnet3/vmxnet3_drv.c
+++ b/drivers/net/vmxnet3/vmxnet3_drv.c
@@ -2562,6 +2562,8 @@ static int
 vmxnet3_request_irqs(struct vmxnet3_adapter *adapter)
 {
 	struct vmxnet3_intr *intr = &adapter->intr;
+	int num_msix_tx = 0;
+	int num_msix_rx = 0;
 	int err = 0, i;
 	int vector = 0;
 
@@ -2576,17 +2578,17 @@ vmxnet3_request_irqs(struct vmxnet3_adapter *adapter)
 					      vmxnet3_msix_tx, 0,
 					      adapter->tx_queue[i].name,
 					      &adapter->tx_queue[i]);
+				if (err) {
+					dev_err(&adapter->netdev->dev,
+						"Failed to request irq for MSIX, %s, error %d\n",
+						adapter->tx_queue[i].name, err);
+					goto err_free_msix_tx;
+				}
+				num_msix_tx++;
 			} else {
 				sprintf(adapter->tx_queue[i].name, "%s-rxtx-%d",
 					adapter->netdev->name, vector);
 			}
-			if (err) {
-				dev_err(&adapter->netdev->dev,
-					"Failed to request irq for MSIX, %s, "
-					"error %d\n",
-					adapter->tx_queue[i].name, err);
-				return err;
-			}
 
 			/* Handle the case where only 1 MSIx was allocated for
 			 * all tx queues */
@@ -2620,8 +2622,9 @@ vmxnet3_request_irqs(struct vmxnet3_adapter *adapter)
 					   "Failed to request irq for MSIX, "
 					   "%s, error %d\n",
 					   adapter->rx_queue[i].name, err);
-				return err;
+				goto err_free_msix_rx;
 			}
+			num_msix_rx++;
 
 			adapter->rx_queue[i].comp_ring.intr_idx = vector++;
 		}
@@ -2676,6 +2679,16 @@ vmxnet3_request_irqs(struct vmxnet3_adapter *adapter)
 	}
 
 	return err;
+
+err_free_msix_rx:
+	for (i = 0; i < num_msix_rx; i++)
+		free_irq(intr->msix_entries[vector++].vector,
+			 &(adapter->rx_queue[i]));
+err_free_msix_tx:
+	for (i = 0; i < num_msix_tx; i++)
+		free_irq(intr->msix_entries[vector++].vector,
+			 &(adapter->tx_queue[i]));
+	return err;
 }