Re: [PATCH net v3] qede: Fix NULL pointer dereference in TPA fragment processing

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 29 Jul 2026 14:57:54 -0700 Jacob Keller wrote:
> Sashiko complains here with the following:
> > Will this check always evaluate to false? Looking at qede_tpa_start(),
> > it appears tpa_info->buffer.data is never assigned:
> > qede_tpa_start() {
> >     ...
> >     tpa_info->buffer.page_offset = sw_rx_data_cons->page_offset;
> >     tpa_info->buffer.mapping = sw_rx_data_cons->mapping;
> >     /* buffer.data is not set here */
> > }
> > If buffer.data is never initialized, will this bypass the error recovery
> > logic for TPA start failures? This could result in a permanent memory leak
> > for the page and DMA mapping, and permanently exhaust the physical ring slots
> > since rxq->filled_buffers is never decremented here to account for the lost
> > slot. Could the original NULL pointer dereference be fixed by adding the
> > missing assignment in qede_tpa_start() instead?  
> 
> 
> It doesn't seem to realize that buffer.data is assigned by
> qede_realloc_rx_buffer or qede_alloc_rx_buffer, so I do not think this
> is legitimate.
> 
> In particular, I think the AI fails to track that tpa_info->buffer
> points to the same memory:
> 
> struct sw_rx_data *current_bd = &rxq->sw_rx_ring[rxq->sw_rx_cons &
> NUM_RX_BDS_MAX]
> 
> vs
> struct qede_agg_info *tpa_info = &rxq->tpa_info[cqe->tpa_agg_index];
> 
> As a result, it thinks the buffer.data is never initialized and relies
> on this conclusion for both this and its next complaint.

I fed this back into AI and it still disagrees (slop at the end).
Could y'all triple check? I'd also look at bnxt for inspiration
Naming of things are suspiciously similar between these drivers.


  current_bd and tpa_info->buffer are distinct objects:

  - current_bd resides in dynamically allocated rxq->sw_rx_ring.
  - tpa_info->buffer is embedded in rxq->tpa_info[] (drivers/net/ethernet/
    qlogic/qede/qede.h:305).

  qede_alloc_rx_buffer() assigns only a ring entry (drivers/net/ethernet/
  qlogic/qede/qede_fp.c:58). Neither it nor qede_realloc_rx_buffer() assigns
  tpa_info->buffer.data.

  At TPA start, only page_offset and mapping are copied (drivers/net/ethernet/
  qlogic/qede/qede_fp.c:865). Because the RX queue is zero-allocated,
  tpa_info->buffer.data remains NULL.

  Consequently, after a TPA-start failure:

  1. The valid ring descriptor is consumed.
  2. buffer.data remains NULL.
  3. The new check at line 1077 skips qede_reuse_page().
  4. The page/mapping are lost and filled_buffers remains overstated,
     preventing normal refill.

  The history confirms this: before commit 8a8633978b84 added build_skb()
  support, the driver used:

  tpa_info->buffer = *sw_rx_data_cons;

  That commit replaced it with copies of only mapping and page_offset,
  apparently dropping data accidentally.

  The proper correction is to preserve data for the failed-start recovery and
  retain tpa_start_fail as the ownership condition. Merely switching to
  buffer.data is also unsafe: after a successful full-page start, a non-NULL
  pointer does not mean the page may be recycled; it belongs to the skb. The
  reviewer’s concern is legitimate and likely identifies the actual root cause
  of the NULL descriptor.