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

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

On Sun Sep 27, 2026 at 8:01 PM EDT, netdev-bot+sashiko wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 4 · 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).
>
> - [Medium] Rx buffer queues can starve for good after a page allocation
>   failure.
> - [Medium] mpnic_clean_rcq() never checks that a frame is in progress
>   before handling a descriptor.
> - [Medium] mpnic_page_pool_get() indexes ring->rx_buf[idx] with the raw
>   18-bit MPNIC_RCD_AL_BUFF_ID from the completion descriptor (up to…
> - [Medium] The descriptor fields BUFF_OFF (16 bits) and BUFF_LEN (14
>   bits) are used without range checks to build the skb head, frame_sz,…
>
> --
>
> eth: mpnic: add basic Rx handling
>
> This commit adds Rx completion processing to the mpnic driver. Each frame
> is built in an xdp_buff from three kinds of descriptor: a header
> descriptor, one payload descriptor per page and a closing metadata
> descriptor. The frame is then passed to GRO, and pages are shared between
> frames using a batched page_pool reference bias.
>
>> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> index 389a9e5476c6e..edb07b7f334bd 100644
>> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> @@ -405,6 +405,34 @@ static void mpnic_fill_qt_bdqs(struct mpnic_q_triad *qt)
>
> [ ... ]
>
>> +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]
> Here idx is the raw MPNIC_RCD_AL_BUFF_ID field from the completion
> descriptor. mpnic_pkt_prepare() and mpnic_add_rx_frag() pass it through
> unchanged. The field is 18 bits wide (up to 262143), but rx_buf[] only
> has hpq_size/ppq_size entries (256 by default).
>
> Could a bad buffer ID from the device read past the end of rx_buf[]?
> page_pool_fragment_page() would then write pp_ref_count through
> whatever pointer it found there.
>
> An in-range ID for a slot the device already completed seems like it
> would also reset the refcount of a page that in-flight skbs still own.
>
> mpnic_bd_prep() only posts IDs in [0, size_mask], so this needs a faulty
> or misbehaving device. Would it be worth masking idx with
> ring->size_mask, or rejecting IDs that are out of range?
>

Comment addressed in v1.

> [ ... ]
>
>> @@ -417,6 +445,87 @@ static void mpnic_flush_pg_ctxt(struct mpnic_pg_ctxt *ctxt, bool napi)
>>  	}
>>  }
>>  
>> +static unsigned int mpnic_hdr_pg_start(unsigned int pg_off)
>> +{
>
> [ ... ]
>
>> +	if (ALIGN(MPNIC_RX_HROOM, 128) > MPNIC_RX_HROOM &&
>> +	    pg_off == ALIGN(MPNIC_RX_HROOM, 128))
>> +		return 0;
>> +
>> +	return pg_off - MPNIC_RX_HROOM;
>> +}
>
> [Severity: Medium]
> If the device reports a BUFF_OFF smaller than MPNIC_RX_HROOM, does this
> unsigned subtraction wrap around and produce a huge pg_start?
>
> The BUFF_OFF (16 bits) and BUFF_LEN (14 bits) fields are used without
> range checks in a few other places:
>
> - mpnic_hdr_pg_end() can return values well beyond PAGE_SIZE. These
>   feed frame_sz in mpnic_pkt_prepare(), which decides where
>   skb_shared_info is written.
>
> - With fin set and pg_off larger than page_size(page), frame_sz in
>   mpnic_pkt_prepare() and truesz in mpnic_add_rx_frag() both underflow.
>
> - In mpnic_add_rx_frag(), a payload frag with pg_off + len past
>   page_size(page) would expose adjacent memory to the stack.
>
> The same values also set the page_pool_dma_sync_for_cpu() ranges.
>
> A device that follows the configured HROOM/TROOM/MAX_HEADER_BYTES layout
> should not produce these values. Would a sanity check against
> page_size(page) still be worth adding?
>

We trust the offset field written by the device.

> [ ... ]
>
>> @@ -442,23 +551,99 @@ 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)
>> +{
>
> [ ... ]
>
>> +		switch (FIELD_GET(MPNIC_RCD_TYPE, rcd)) {
>> +		case MPNIC_RCD_TYPE_HDR_AL:
>> +			if (FIELD_GET(MPNIC_RCD_HDR_SUBTYPE, rcd) ==
>> +			    MPNIC_RCD_HDR_SUBTYPE_HDR)
>> +				mpnic_pkt_prepare(rcd, state, qt);
>> +			break;
>> +		case MPNIC_RCD_TYPE_PAY_AL:
>> +			mpnic_add_rx_frag(rcd, state, qt);
>> +			break;
>> +		case MPNIC_RCD_TYPE_META: {
>> +			struct sk_buff *skb = NULL;
>> +
>> +			if (likely(!(rcd &
>> +				     MPNIC_RCD_META_UNCORRECTABLE_ERR_MASK) &&
>> +				   !state->pkt.add_frag_failed))
>> +				skb = xdp_build_skb_from_buff(&state->pkt.buff);
>> +
>> +			if (likely(skb))
>> +				napi_gro_receive(&nv->napi, skb);
>> +			else
>> +				mpnic_put_pkt_buff(&state->pkt, true);
>> +
>> +			state->pkt.buff.data_hard_start = NULL;
>
> [Severity: Medium]
> Nothing checks that a frame is in progress before a PAY_AL or META
> descriptor is handled. state->pkt.buff.data_hard_start is zero at
> start and is set back to NULL after every META.
>
> Suppose a PAY_AL arrives without a preceding HDR descriptor.
> mpnic_add_rx_frag() then calls xdp_buff_add_frag(), which writes to
> skb_shared_info at an address based on a NULL data_hard_start and a
> stale frame_sz.
>
> A META in the same state would take the success branch:
>
> mpnic_clean_rcq()
>   xdp_build_skb_from_buff()
>     napi_build_skb(NULL, frame_sz)
>
> Only the error path, mpnic_put_pkt_buff(), checks data_hard_start.
>
> Can either of these cause a NULL pointer dereference in NAPI context?
>
> HDR_AL descriptors whose subtype is not MPNIC_RCD_HDR_SUBTYPE_HDR are
> skipped silently, so a frame that starts that way would hit the same
> path.
>
> The opposite case is also possible. mpnic_pkt_prepare() overwrites
> state->pkt.buff without releasing the page_pool references of an
> unfinished frame. Could that leak pages and stall page_pool_destroy()?
>
> fbnic_pkt_prepare() has a WARN_ON_ONCE(xdp->data_hard_start) at this
> point, and mpnic_pkt_prepare() drops it. Would it make sense to keep it?
>
> All of this needs the device to break the HDR, PAY_AL*, META ordering.
>

Same comment responded to in v1.

> [ ... ]
>
>> +	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)
>>  {
>
> [ ... ]
>
>> -	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]
> mpnic_clean_rcq() is the only runtime caller of mpnic_fill_qt_bdqs().
> __mpnic_fill_bdq() simply stops when the page pool allocation fails:
>
> 		page = page_pool_dev_alloc_pages(bdq->page_pool);
> 		if (!page)
> 			break;
>
> The shortfall is never reported back, because mpnic_clean_rcq() only
> returns the packet count. mpnic_poll() then completes NAPI and re-arms
> the IRQ anyway.
>
> Say the allocation fails while the device has no posted buffers left,
> or fewer than the 16-descriptor HPQ/PPQ drop threshold. The device then
> drops incoming frames without writing any RCQ completions. What would
> schedule NAPI again in that case?
>
> There doesn't seem to be a timer, a service task, a forced IRQ trigger,
> or a return of the full budget to retry the refill. The only
> mpnic_nv_irq_trigger() call is in mpnic_napi_enable(). The only way out
> seems to be a Tx completion on the same vector.
>
> Could a receive-mostly queue stay stuck until the interface is brought
> down and up again?
>
> [ ... ]

Valid. We prefer to address this in a small follow up series.