Re: [PATCH net-next 8/8] eth: mpnic: add basic Rx handling

["Daniel Zahka" <daniel.zahka@gmail.com>]

On Wed Sep 23, 2026 at 10:05 PM EDT, netdev-bot+sashiko wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 5 · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [High] net: mpnic: missing descriptor sequence validation causes NULL
>   pointer dereference
> - [Medium] mpnic_poll() calls mpnic_clean_rcq() even when budget is 0.
> - [Medium] When page_pool_dev_alloc_pages() returns NULL,
>   __mpnic_fill_bdq() stops refilling and returns the partial tail.
> - [Medium] mpnic_pkt_prepare() re-initialises state->pkt with
>   xdp_init_buff()/xdp_prepare_buff() without checking whether a frame is…
> - [Medium] mpnic_page_pool_get() indexes ring->rx_buf[idx] with idx =
>   FIELD_GET(MPNIC_RCD_AL_BUFF_ID, rcd), an 18-bit device-written field…
> - [Medium] The device-supplied completion fields BUFF_OFF (16 bits) and
>   BUFF_LEN (14 bits) are never checked against page_size(page),…
>
> --
>
> eth: mpnic: add basic Rx handling
>
> This adds Rx completion processing to mpnic. A frame is assembled into an
> xdp_buff from a header descriptor, one descriptor per payload page and a
> closing metadata descriptor, and then handed to the stack. Page references
> are taken in batches per page instead of once per frame.
>
>> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> index 6d2123bee97b4..9878ea5a2f8e1 100644
>> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> @@ -402,6 +402,34 @@ static void mpnic_fill_qt_bdqs(struct mpnic_q_triad *qt)
>>  	__mpnic_bdq_commit_tail(&qt->sub1, ppq_i);
>>  }
>>  
>> +/* Take one of the references batched on the page at @idx. If the device
>> + * has moved on to a new page, first drop the unused references left on
>> + * the previous one.
>> + */
>> +static struct page *
>> +mpnic_page_pool_get(struct mpnic_pg_ctxt *pg_ctxt, struct mpnic_ring *ring,
>> +		    u32 idx)
>> +{
>> +	struct page *page = pg_ctxt->page;
>> +
>> +	if (unlikely(pg_ctxt->idx != idx)) {
>> +		if (pg_ctxt->pagecnt_bias &&
>> +		    !page_pool_unref_page(page, pg_ctxt->pagecnt_bias))
>> +			page_pool_put_unrefed_page(page->pp, page, -1, true);
>> +
>> +		page = ring->rx_buf[idx];
>> +		page_pool_fragment_page(page, MPNIC_PAGECNT_BIAS_MAX);
>
> [Severity: Medium]
> Can idx index past the end of ring->rx_buf[] here?
>

Like I mentioned about the head field in the tx completion descriptor,
we don't harden against device produced completion fields like this.

> idx comes straight from the completion descriptor:
>
>     unsigned int pg_idx = FIELD_GET(MPNIC_RCD_AL_BUFF_ID, rcd);
>
> MPNIC_RCD_AL_BUFF_ID is an 18-bit field (DESC_GENMASK(33, 16)). rx_buf[]
> is sized by hpq_size/ppq_size, which is 256 entries by default.
>
> Both callers mask the value only when they update head:
>
>     qt->sub0.head = (pg_idx + 1) & qt->sub0.size_mask;
>
> If the device reports a BUFF_ID at or above the ring size, the pointer
> read from past the end of rx_buf[] is written through by
> page_pool_fragment_page(). It is then passed to page_address(), used for
> the DMA sync and attached to the skb.
>
> A conforming device only echoes IDs the driver posted, so hitting this
> needs buggy firmware or an untrusted device. Would it make sense to mask
> idx with ring->size_mask before the lookup?
>
> [ ... ]
>
>> @@ -414,9 +442,90 @@ static void mpnic_flush_pg_ctxt(struct mpnic_pg_ctxt *ctxt, bool napi)
>>  	}
>>  }
>>  
>> -static void mpnic_put_pkt_buff(struct mpnic_pkt_ctxt *ctxt, bool napi)
>> +static unsigned int mpnic_hdr_pg_start(unsigned int pg_off)
>> +{
>> +	/* The headroom of the first header may be larger than
>> +	 * MPNIC_RX_HROOM due to alignment. So account for that by just
>> +	 * making the page offset 0 if we are starting at the first header.
>> +	 */
>> +	if (ALIGN(MPNIC_RX_HROOM, 128) > MPNIC_RX_HROOM &&
>> +	    pg_off == ALIGN(MPNIC_RX_HROOM, 128))
>> +		return 0;
>> +
>> +	return pg_off - MPNIC_RX_HROOM;
>> +}
>
> [ ... ]
>
>> +static void
>> +mpnic_pkt_prepare(struct mpnic_napi_vector *nv, u64 rcd,
>> +		  struct mpnic_rcq_state *state, struct mpnic_q_triad *qt)
>> +{
>> +	unsigned int pg_off = FIELD_GET(MPNIC_RCD_AL_BUFF_OFF, rcd);
>> +	unsigned int pg_idx = FIELD_GET(MPNIC_RCD_AL_BUFF_ID, rcd);
>> +	unsigned int len = FIELD_GET(MPNIC_RCD_AL_BUFF_LEN, rcd);
>> +	bool fin = FIELD_GET(MPNIC_RCD_AL_PAGE_FIN, rcd);
>> +	unsigned int frame_sz, pg_start, pg_end;
>> +	struct xdp_buff *buff = &state->pkt;
>> +	struct page *page;
>> +
>> +	pg_start = mpnic_hdr_pg_start(pg_off);
>> +
>> +	page = mpnic_page_pool_get(&state->hdr, &qt->sub0, pg_idx);
>> +	qt->sub0.head = (pg_idx + 1) & qt->sub0.size_mask;
>> +
>> +	/* Short-cut the end calculation if the page is fully consumed */
>> +	pg_end = fin ? page_size(page) : mpnic_hdr_pg_end(pg_off, len);
>> +	frame_sz = pg_end - pg_start;
>
> [Severity: High]
> What happens if a PAY_AL or META descriptor reaches this switch without
> a header descriptor opening the frame first?
>
> In that case state->pkt.data_hard_start is NULL. Either the previous
> META cleared it, or no header has been seen since the queue was
> enabled. Nothing on the PAY_AL or META paths checks for this.
>
> For PAY_AL, mpnic_add_rx_frag() first takes a reference through
> mpnic_page_pool_get(). It then calls xdp_buff_add_frag(), which finds
> skb_shared_info through xdp_get_shared_info_from_buff(), computed from
> data_hard_start + frame_sz. With data_hard_start NULL, and frame_sz and
> flags left over from the previous frame, the frag is written through a
> wild pointer near address zero.
>
> For META, the build path runs unless the error bit or add_frag_failed
> is set. add_frag_failed may also be stale from the previous frame.
> xdp_build_skb_from_buff() then calls napi_build_skb() with a NULL
> data_hard_start. On a queue that has never seen a header, xdp->rxq may
> also be unset when rxq->mem.type is read. mpnic_put_pkt_buff() already
> copes with a NULL data_hard_start, but it only runs after the build
> path has failed.
>
> As with the other descriptor trust issues above, a conforming device
> should always open a frame with a header descriptor, so reaching this
> needs buggy or untrusted firmware. The result is a host crash rather
> than a dropped frame, though.
>
> Would it make sense to check state->pkt.data_hard_start in the PAY_AL
> and META cases and drop the descriptor when no frame is being
> assembled? On the PAY_AL path that would also mean returning the
> reference taken by mpnic_page_pool_get(), the same way the
> xdp_buff_add_frag() failure path does.
>

We trust the descriptor stream to be sequenced correctly.

> [Severity: Medium]
> Should BUFF_OFF and BUFF_LEN from the completion be range checked before
> they are used here?
>

Won't fix.

> Neither value is checked against page_size(page), MPNIC_RX_HROOM or
> MPNIC_RX_TROOM:
>
> - mpnic_hdr_pg_start() returns pg_off - MPNIC_RX_HROOM as an unsigned
>   value, so it wraps if pg_off is below the headroom.
>
> - mpnic_hdr_pg_end() can return an end past the page, and
>   frame_sz = pg_end - pg_start can wrap. After that,
>   xdp_build_skb_from_buff()->napi_build_skb(data_hard_start, frame_sz)
>   would place and zero skb_shared_info outside the page.
>
> - In mpnic_add_rx_frag(), truesz underflows when fin is set and pg_off is
>   larger than the page:
>
>     truesz = (fin ? page_size(page) : ALIGN(pg_off + len, 128)) - pg_off;
>
>   The frag [pg_off, pg_off + len) is also attached with no check that it
>   stays inside the page, which would expose adjacent memory through the
>   skb.
>
> The dma_sync_single_range_for_cpu() calls use the same unchecked ranges.
>
> A conforming device honours the headroom and tailroom programmed in
> mpnic_set_rde_cfg() and stays within the page size, so hitting this needs
> buggy or untrusted hardware.
>
>> +
>> +	dma_sync_single_range_for_cpu(nv->dev, page_pool_get_dma_addr(page),
>> +				      pg_start, frame_sz, DMA_FROM_DEVICE);
>> +
>> +	xdp_init_buff(buff, frame_sz, &qt->xdp_rxq);
>> +	xdp_prepare_buff(buff, page_address(page) + pg_start,
>> +			 pg_off - pg_start, len, true);
>
> [Severity: Medium]
> What happens here to a frame that is still being assembled when a new
> header descriptor arrives?
>
> mpnic_clean_rcq() calls mpnic_pkt_prepare() for every header descriptor.
> It does not check whether state->pkt.data_hard_start is still set by an
> earlier frame that never got its META descriptor:
>
>     case MPNIC_RCD_TYPE_HDR_AL:
>         if (FIELD_GET(MPNIC_RCD_HDR_SUBTYPE, rcd) ==
>             MPNIC_RCD_HDR_SUBTYPE_HDR)
>             mpnic_pkt_prepare(nv, rcd, state, qt);
>
> In that case xdp_init_buff() clears buff->flags and xdp_prepare_buff()
> replaces data_hard_start. The old frame's header page reference from
> mpnic_page_pool_get() and its frag references from xdp_buff_add_frag()
> can no longer be reached.
>
> The META path and mpnic_flush() cannot release them either, because
> mpnic_flush() only calls mpnic_put_pkt_buff() on the current state->pkt.
> Could the page pool pages leak here, leaving page_pool_destroy() waiting
> forever on inflight pages?
>
> The code does not show whether the hardware can produce HDR, [PAY_AL ...],
> HDR without a META in between. fbnic has a

We trust that the hw cannot produce this sequence.

> WARN_ON_ONCE(pkt->buff.data_hard_start) for this case. Would a similar
> check here, plus releasing the old frame, make sense?
>
> [ ... ]
>
>> @@ -439,23 +548,97 @@ static void mpnic_put_pkt_buff(struct mpnic_pkt_ctxt *ctxt, bool napi)
>>  	page_pool_put_full_page(page->pp, page, napi);
>>  }
>>  
>> +static int mpnic_clean_rcq(struct mpnic_napi_vector *nv,
>> +			   struct mpnic_q_triad *qt, int budget)
>> +{
>
> [ ... ]
>
>> +	while (packets < budget) {
>
> [ ... ]
>
>> +	rcq->head = head;
>> +
>> +	/* Allocate buffers, force dma_wmb(), and then start writing tails */
>> +	mpnic_fill_qt_bdqs(qt);
>> +
>> +	return packets;
>> +}
>> +
>>  static int mpnic_poll(struct napi_struct *napi, int budget)
>>  {
>>  	struct mpnic_napi_vector *nv = container_of(napi,
>>  						    struct mpnic_napi_vector,
>>  						    napi);
>> -	int i;
>> +	int i, j, work_done = 0;
>>  
>>  	for (i = 0; i < nv->txt_count; i++)
>>  		mpnic_clean_tcq(nv, &nv->qt[i], budget);
>>  
>> +	for (j = 0; j < nv->rxt_count; j++, i++)
>> +		work_done += mpnic_clean_rcq(nv, &nv->qt[i], budget);
>
> [Severity: Medium]
> Is it safe to do the Rx side of the poll when budget is 0?
>

Will address in v2.

> netpoll calls napi->poll(napi, 0) via
> netpoll_poll_dev()->poll_napi()->poll_one_napi(), and netconsole can reach
> that path from any context. With budget 0 the descriptor loop in
> mpnic_clean_rcq() is skipped, but the refill still runs:
>
> mpnic_clean_rcq()
>   mpnic_fill_qt_bdqs()
>     __mpnic_fill_bdq()
>       page_pool_dev_alloc_pages()
>
> The allocation happens whenever mpnic_bdq_desc_unused() is non-zero, for
> example after an earlier allocation failure.
>
> Documentation/networking/napi.rst says "Rx specific APIs like page pool or
> XDP cannot be used at all when budget is 0".
>
> The page_pool allocation fast path, __page_pool_get_cached(), and
> page_pool_refill_alloc_cache() access pool->alloc.cache and the ptr_ring
> consumer side without taking a lock.
>
> The pool was created with .napi = &nv->napi. That means
> page_pool_napi_local() allows direct recycling into alloc.cache from
> softirq on the NAPI's list_owner CPU, and netpoll's poll_owner does not
> exclude that.
>
> Could this corrupt alloc.cache or the ring and hand out the same page
> twice? Skipping Rx cleaning and refill when budget is 0 would avoid it.
>
>> +
>>  	for (i = 0; i < nv->txt_count; i++)
>>  		mpnic_commit_cq_head(&nv->qt[i].cmpl);
>>  
>> -	if (likely(napi_complete_done(napi, 0)))
>> +	if (work_done >= budget)
>> +		return budget;
>> +
>> +	if (likely(napi_complete_done(napi, work_done)))
>>  		mpnic_nv_irq_rearm(nv);
>
> [Severity: Medium]
> Can Rx stop for good after a run of allocation failures?
>
> When page_pool_dev_alloc_pages() returns NULL, __mpnic_fill_bdq() just
> stops:
>
>     page = page_pool_dev_alloc_pages(bdq->page_pool);
>     if (!page)
>         break;
>
> mpnic_clean_rcq() returns only the packet count. So whenever
> work_done < budget, mpnic_poll() completes NAPI and rearms the IRQ, even
> if the refill did not succeed.
>
> If allocations keep failing, successive polls can drain HPQ/PPQ until no
> buffers are posted. The device then has nothing to DMA into and raises no
> Rx interrupts. The only napi_schedule() in the driver is in
> mpnic_msix_clean_rings().
>
> Rx would then stay stopped after memory is available again, until a Tx
> completion on the same vector triggers a poll or the interface is taken
> down and up. Should the poll stay scheduled, or set up a retry, when the
> BDQs could not be refilled?
>

This will be addressed in v2. napi depletion check was improperly left
out of this posting.

>>  
>> -	return 0;
>> +	return work_done;
>>  }
>
> [ ... ]