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

On Thu, Aug 6, 2026 11:32 PM, Jakub Kicinski wrote:
> 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 non-MSI-X case, the MISC and queue interrupt share the vector 0.

> 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);
> }

The register WX_PX_MISC_IVAR is only valid in the MSI-X case.

> 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?

I'll add a log to describe this in passing fix.