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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] net: mpnic: unbounded NAPI budget in mpnic_poll causes fairness violation
- [High] net: mpnic: missing descriptor sequence validation causes NULL pointer dereference
- [Medium] net: mpnic: direct DMA sync on page pool buffers breaks memory providers
--

commit a0a26aee27284ff3d767cd871801499330442f84
Author: Daniel Zahka <daniel.zahka@gmail.com>

eth: mpnic: add basic Rx handling

This commit introduces basic receive handling for the mpnic driver. It 
assembles frames using completion descriptors, merging contiguous payload
fragments, and utilizes a batched page reference strategy to manage 
xdp_buff allocations.

> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
[ ... ]
> +static void
> +mpnic_pkt_prepare(struct mpnic_napi_vector *nv, u64 rcd,
> +		  struct mpnic_rcq_state *state, struct mpnic_q_triad *qt)
> +{
[ ... ]
> +	/* 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;
> +
> +	dma_sync_single_range_for_cpu(nv->dev, page_pool_get_dma_addr(page),
> +				      pg_start, frame_sz, DMA_FROM_DEVICE);

[Severity: Medium]
Does this break the abstraction for memory providers like dmabuf? 
Using dma_sync_single_range_for_cpu() directly on page pool buffers bypasses 
the standard page pool synchronization hooks like page_pool_dma_sync_for_cpu() 
or page_pool_dma_sync_netmem_for_cpu().

> +
> +	xdp_init_buff(buff, frame_sz, &qt->xdp_rxq);
> +	xdp_prepare_buff(buff, page_address(page) + pg_start,
> +			 pg_off - pg_start, len, true);
> +	net_prefetch(buff->data);
> +
> +	state->add_frag_failed = false;
> +}
> +
> +static void
> +mpnic_add_rx_frag(struct mpnic_napi_vector *nv, u64 rcd,
> +		  struct mpnic_rcq_state *state, struct mpnic_q_triad *qt)
> +{
[ ... ]
> +	truesz = (fin ? page_size(page) : ALIGN(pg_off + len, 128)) - pg_off;
> +
> +	dma_sync_single_range_for_cpu(nv->dev, page_pool_get_dma_addr(page),
> +				      pg_off, truesz, DMA_FROM_DEVICE);

[Severity: Medium]
Similarly here, does direct use of dma_sync_single_range_for_cpu() on the 
payload fragments circumvent the required page pool wrappers?

> +
> +	if (!xdp_buff_add_frag(buff, page_to_netmem(page), pg_off, len,
> +			       truesz)) {

[Severity: High]
Can a malformed descriptor sequence crash the kernel here?

If the hardware delivers a payload (PAY_AL) descriptor before a header 
(HDR_AL) descriptor, mpnic_pkt_prepare() won't be called, so the buffer 
won't be properly initialized. The data_hard_start field would remain NULL, 
causing xdp_buff_add_frag() to compute an invalid pointer for the 
skb_shared_info.

> +		state->payld.pagecnt_bias++;
> +		state->add_frag_failed = true;
> +	}
> +}
[ ... ]
> +static int mpnic_clean_rcq(struct mpnic_napi_vector *nv,
> +			   struct mpnic_q_triad *qt, int budget)
> +{
[ ... ]
> +	while (packets < budget) {
> +		u64 rcd;
> +
> +		if ((*raw_rcd & cpu_to_le64(MPNIC_RCD_DONE)) != done)
> +			break;
> +
> +		dma_rmb();
> +
> +		rcd = le64_to_cpu(*raw_rcd);
> +
> +		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(nv, rcd, state, qt);
> +			break;
> +		case MPNIC_RCD_TYPE_PAY_AL:
> +			mpnic_add_rx_frag(nv, rcd, state, qt);
> +			break;
> +		case MPNIC_RCD_TYPE_META: {
> +			struct sk_buff *skb = NULL;
> +
> +			if (likely(!(rcd &
> +				     MPNIC_RCD_META_UNCORRECTABLE_ERR_MASK) &&
> +				   !state->add_frag_failed))
> +				skb = xdp_build_skb_from_buff(&state->pkt);

[Severity: High]
If a metadata (META) descriptor is received before any header descriptor 
(HDR_AL), could xdp_build_skb_from_buff() also attempt to operate on an 
uninitialized state->pkt and dereference a NULL data_hard_start?

> +
> +			if (likely(skb))
> +				napi_gro_receive(&nv->napi, skb);
> +			else
> +				mpnic_put_pkt_buff(&state->pkt, true);
> +
> +			state->pkt.data_hard_start = NULL;
> +			packets++;
> +			break;
> +		}
> +		}
[ ... ]
>  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: High]
Does this allow the total work done to dramatically exceed the NAPI limit?

The full budget is unconditionally passed to every RX queue without dividing
or subtracting the work already done. A NAPI vector with multiple RX queues
handling a burst could process nv->rxt_count * budget packets, potentially
defeating softirq fairness mechanisms and causing high system latency.

> +
>  	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);
>  
> -	return 0;
> +	return work_done;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922-linux-mpnic-v1-0-236844f53072@gmail.com?part=8