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

[Simon Horman <horms@kernel.org>]

On Fri, Aug 07, 2026 at 02:22:14PM +0800, Jiawen Wu wrote:
> In non-MSI-X mode (such as legacy INTx or single MSI), wx->msix_entry is
> not allocated or initialized. Calling NGBE_INTR_MISC(wx) dereferences
> wx->msix_entry->entry, leading to a NULL pointer dereference crash.
> 
> This issue was introduced by fixing the IRQ vector when the number of
> VFs is 7. Fix the issue by explicitly checking `pdev->msix_enabled` to
> determine the correct vector index.
> 
> Additionally, as a side fix, set the interrupt mask to BIT(0) for the
> non-MSI-X fallback. In MSI/INTx mode, the MISC and queue interrupts
> share vector 0, and the WX_PX_MISC_IVAR register is only valid in the
> MSI-X case. Thus, BIT(0) is the correct mask for the miscellaneous cause
> when MSI-X is disabled.
> 
> Fixes: 4174c0c331a2 ("net: ngbe: specify IRQ vector when the number of VFs is 7")
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> Reviewed-by: Breno Leitao <leitao@debian.org>

Hi Jiawen,

I know the topic of BIT(0) came up in v2 - the AI-generated review
forwarded by Jakub. And I appreciate you clarifying things in the commit
message. But the AI generated review on netdev-qi [1] raises concerns which
I'd appreciate you looking over.

[1] https://netdev-ai.bots.linux.dev/sashiko/#/patchset/B2693E9A8BFAD110%2B20260807062214.410838-1-jiawenwu%40trustnetic.com

Although the review rehashes the concerns raised for v2, which you have
addressed.  There are three related areas of concern which I can't resolve in
my mind.

  1) The question of unmasking IRQ 0,
     and the implication of possible interrupt storms.

  2) Compatibility of the BIT(0) change in this patch with the implementation
     of wx_configure_vectors()

  3) Is there a need for any similar changes to txgbe.

Text from the AI-generated review is as follows:

  Is BIT(0) the right mask here, or is it the mask bit of queue vector 0
  rather than of the misc cause?

  In MSI/INTx mode wx_set_interrupt_capability() sets wx->num_q_vectors = 1 and
  q_vector[0]->v_idx == 0, and the queue vector's mask bit is BIT(v_idx), since
  WX_INTR_Q(i) is BIT((i)) and wx_poll() re-arms the queue with:

  drivers/net/ethernet/wangxun/libwx/wx_lib.c:wx_poll() {
	...
	if (likely(napi_complete_done(napi, work_done))) {
		...
		if (!test_bit(WX_STATE_DOWN, wx->state))
			wx_intr_enable(wx, WX_INTR_Q(q_vector->v_idx));
	}
	...
  }

  So does the new fallback unmask the queue vector on a path whose callers
  describe it as "no lsc, no queues"?

  The concrete effect appears in ngbe_intr(), where the queue cause is
  auto-masked by hardware on the cause read:

  drivers/net/ethernet/wangxun/ngbe/ngbe_main.c: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);
	...
  }

  With BIT(0) the queue vector is unmasked again immediately while NAPI is
  still scheduled. Does this defeat the NAPI moderation in MSI/INTx mode, so
  the driver takes an interrupt per completion event instead of one per NAPI
  cycle?

  The peer driver on the same libwx register layout uses the opposite value,
  unconditionally, including on its non-MSI-X paths in txgbe_irq.c:

  drivers/net/ethernet/wangxun/txgbe/txgbe_type.h:

  #define TXGBE_INTR_MISC(A)    BIT((A)->num_q_vectors)
  #define TXGBE_INTR_QALL(A)    (TXGBE_INTR_MISC(A) - 1)

  ngbe also used BIT(wx->num_q_vectors) for this path before 4174c0c331a2.
  Would BIT(wx->num_q_vectors) be the value that matches libwx and txgbe here,
  or alternatively programming WX_PX_MISC_IVAR to vector 0 when
  !pdev->msix_enabled so that routing and mask agree?

  Related to that, the changelog says:

      In MSI/INTx mode, the MISC and queue interrupts
      share vector 0, and the WX_PX_MISC_IVAR register is only valid in the
      MSI-X case. Thus, BIT(0) is the correct mask for the miscellaneous cause
      when MSI-X is disabled.

  Which code establishes that WX_PX_MISC_IVAR is only valid with MSI-X?
  wx_configure_vectors() writes the misc IVAR unconditionally with
  v_idx == wx->num_q_vectors, and only the following ITR write is gated on
  msix_enabled:

  drivers/net/ethernet/wangxun/libwx/wx_lib.c:wx_configure_vectors() {
	...
	wx_set_ivar(wx, -1, 0, v_idx);
	if (pdev->msix_enabled)
		wr32(wx, WX_PX_ITR(v_idx), 1950);
  }

  The only place that forces the misc vector index to 0 is the
  WX_FLAG_IRQ_VECTOR_SHARED path set by __wx_enable_sriov() for
  num_vfs == 7 && mac.type == wx_mac_em, which is itself an MSI-X case. Could
  the changelog be adjusted to match what the code does, or is there hardware
  behaviour behind the claim that is not visible in the driver?