Re: [PATCH net v2] net: ngbe: fix NULL pointer dereference in non-MSI-X interrupt enabling

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: ngbe: fix NULL pointer dereference in non-MSI-X interrupt enabling

This patch avoids dereferencing wx->msix_entry in NGBE_INTR_MISC() when the
device is not running in MSI-X mode, by testing pdev->msix_enabled in
ngbe_irq_enable() and falling back to a hard-coded mask value otherwise.

> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> index a16221995909..cb653861aad8 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> @@ -180,8 +180,10 @@ static void ngbe_irq_enable(struct wx *wx, bool queues)
>  	/* mask interrupt */
>  	if (queues)
>  		wx_intr_enable(wx, NGBE_INTR_ALL);
> -	else
> +	else if (wx->pdev->msix_enabled)
>  		wx_intr_enable(wx, NGBE_INTR_MISC(wx));
> +	else
> +		wx_intr_enable(wx, BIT(0));
>  }

[Severity: Medium]
Is BIT(0) the right mask bit for the misc cause in the non-MSI-X case?

In the MSI/INTx fallback, wx_set_interrupt_capability() sets
wx->num_q_vectors = 1, and ngbe_up() -> wx_configure_vectors() still
programs the misc IVAR:

drivers/net/ethernet/wangxun/libwx/wx_lib.c:wx_configure_vectors() {
	...
	wx_set_ivar(wx, -1, 0, v_idx);
}

with v_idx == wx->num_q_vectors == 1.  wx_set_ivar() only forces the
other-causes vector to 0 when WX_FLAG_IRQ_VECTOR_SHARED is set, which
happens for wx_mac_em with num_vfs == 7 in __wx_enable_sriov():

	if (test_bit(WX_FLAG_IRQ_VECTOR_SHARED, wx->flags))
		msix_vector = 0;

So in plain MSI/INTx the misc cause sits at bit 1, and bit 0 is queue
vector 0.  Does this branch then clear the wrong bit in PX_IMC, leaving
the misc cause masked while unmasking the queue cause?

The queue cause looks like it is meant to stay masked here, per
ngbe_intr():

	/* would disable interrupts here but it is auto disabled */
	napi_schedule_irqoff(&q_vector->napi);

	if (!test_bit(WX_STATE_DOWN, wx->state))
		ngbe_irq_enable(wx, false);

and wx_poll() is the place that re-arms it after NAPI completes:

	wx_intr_enable(wx, WX_INTR_Q(q_vector->v_idx));

Can unmasking BIT(0) from the hard IRQ handler let the device re-assert
the (possibly shared) INTx/MSI line while NAPI is still scheduled,
leading to repeated hard-IRQ entries and, on the eicr == 0 path, to
IRQ_NONE returns followed by a full NGBE_INTR_ALL unmask?

Would BIT(wx->num_q_vectors) be the value that preserves the behaviour
prior to commit 4174c0c331a2, where NGBE_INTR_MISC(A) expanded to
BIT((A)->num_q_vectors)?  The sibling driver still uses that form
unconditionally, including on its non-MSI-X path:

drivers/net/ethernet/wangxun/txgbe/txgbe_type.h:
	#define TXGBE_INTR_MISC(A)	BIT((A)->num_q_vectors)

One more note on the changelog: it says the fix is to check
pdev->msix_enabled "to determine the correct vector index", but it does
not mention that the mask value written in non-MSI-X mode changes from
BIT(1) to BIT(0).  Could the changelog spell out the intended vector
index for the MSI/INTx case?
-- 
pw-bot: cr