Re: [PATCH net-next v2 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: rx starvation in mpnic_poll due to sequential queue processing
- [Medium] net: mpnic: NULL pointer dereference on META descriptor without HDR
- [Medium] net: mpnic: NULL pointer dereference on PAY descriptor without HDR
--

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

    eth: mpnic: add basic Rx handling
    
    A frame arrives as a run of completion descriptors: one header
    address/length descriptor, one address/length descriptor per payload
    page, and a metadata descriptor that closes the frame. The frame is
    assembled in an xdp_buff as the descriptors come in and handed to the
    stack when the metadata descriptor arrives.

[ ... ]

> 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

[ ... ]

> +static void
> +mpnic_add_rx_frag(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);
> +	struct xdp_buff *buff = &state->pkt.buff;
> +	unsigned int truesz;
> +	struct page *page;
> +
> +	page = mpnic_page_pool_get(&state->payld, &qt->sub1, pg_idx);
> +	qt->sub1.head = (pg_idx + 1) & qt->sub1.size_mask;
> +
> +	truesz = (fin ? page_size(page) : ALIGN(pg_off + len, 128)) - pg_off;
> +
> +	page_pool_dma_sync_for_cpu(qt->sub1.page_pool, page, pg_off, truesz);
> +
> +	if (!xdp_buff_add_frag(buff, page_to_netmem(page), pg_off, len,
> +			       truesz)) {

[Severity: Medium]
Will this crash if a PAY_AL descriptor arrives before a HDR_AL descriptor?

If the hardware delivers a malformed sequence or drops a descriptor,
state->pkt.buff.data_hard_start might be NULL here. The call to
xdp_buff_add_frag() would then calculate the skb_shared_info pointer relative
to NULL minus SKB_DATA_ALIGN. Writing to sinfo->nr_frags at this invalid
memory address would cause an immediate page fault.

Is there a missing check to verify an active frame was properly started?

> +		state->payld.pagecnt_bias++;
> +		state->pkt.add_frag_failed = true;
> +	}
> +}

[ ... ]

> +static int mpnic_clean_rcq(struct mpnic_napi_vector *nv,
> +			   struct mpnic_q_triad *qt, int budget)
> +{
> +	struct mpnic_ring *rcq = &qt->cmpl;
> +	struct mpnic_rcq_state *state;
> +	unsigned int packets = 0;
> +	__le64 *raw_rcd, done;
> +	u32 head = rcq->head;
> +
> +	done = (head & (rcq->size_mask + 1)) ? 0 : cpu_to_le64(MPNIC_RCD_DONE);
> +	raw_rcd = &rcq->desc[head & rcq->size_mask];
> +	state = rcq->state;
> +
> +	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(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);

[Severity: Medium]
Could this trigger a NULL pointer dereference when accessing unmapped memory?

If no preceding MPNIC_RCD_TYPE_HDR_AL descriptor was received for the frame,
state->pkt.buff.data_hard_start remains NULL. The driver then blindly calls
xdp_build_skb_from_buff(), passing the uninitialized buffer to
napi_build_skb(NULL, 0). The core network stack passes NULL to ksize(), which
returns 0, leading to an integer underflow when subtracting SKB_DATA_ALIGN.
The resulting pointer is memset, causing an immediate crash.

Should we validate that a valid header descriptor was previously received
to initialize the buffer before attempting to build the SKB?

> +
> +			if (likely(skb))
> +				napi_gro_receive(&nv->napi, skb);
> +			else
> +				mpnic_put_pkt_buff(&state->pkt, true);

[ ... ]

> +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);
>  
> +	if (likely(budget))
> +		for (j = 0; j < nv->rxt_count; j++, i++)
> +			work_done += mpnic_clean_rcq(nv, &nv->qt[i],
> +						     budget - work_done);

[Severity: High]
Can this permanently bias NAPI budget allocation and starve higher-indexed Rx
queues?

The loop unconditionally starts at index 0 and passes the remaining NAPI
budget (budget - work_done) to each subsequent queue. If the first queue
continuously exhausts the entire budget under heavy load, budget - work_done
becomes 0 for the remaining queues.

Because the loop does not remember its position across NAPI poll cycles, it
will always restart at queue 0, permanently starving higher-indexed queues.
Could a high rate of traffic to the first queue cause a complete denial of
service for flows hashed to other queues on the same NAPI vector?

> +
>  	for (i = 0; i < nv->txt_count; i++)
>  		mpnic_commit_cq_head(&nv->qt[i].cmpl);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924-linux-mpnic-v2-0-4badc9b58b9e@gmail.com?part=8