Re: [PATCH v4 06/10] net/mlx5: support selective Rx

[Thomas Monjalon <thomas@monjalon.net>]

02/06/2026 15:53, Stephen Hemminger:
> On Fri, 29 May 2026 15:34:00 +0200
> Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> > From: Gregory Etelson <getelson@nvidia.com>
> > 
> > Selective Rx may save some PCI bandwidth.
> > Implement selective Rx in the (quite slow) scalar SPRQ Rx path
> > mlx5_rx_burst() where the performance impact
> > of the added condition branches is acceptable.
> > Other Rx functions do not support this feature.
> > When using selective Rx, mlx5_rx_burst will be selected.
> > 
> > A null Memory Region (MR) is always allocated
> > at shared device context initialization.
> > The selective Rx capability is not advertised
> > if this special MR allocation fails.
> > 
> > For each Rx segment configured with a NULL mempool,
> > a "null mbuf" is created.
> > It is a fake mbuf allocated outside any mempool,
> > used as a placeholder in the Rx ring.
> > The null MR lkey is used in the WQE for these segments
> > so the NIC writes received data to a discard buffer.
> > The mbuf data room size is resolved from the first segment having a pool.
> > For null segments, the buffer length is from the last seen pool,
> > so that the WQE stride size remains consistent.
> > 
> > In mlx5_rx_burst, discarded segments are not chained
> > into the packet mbuf list, NB_SEGS is decremented accordingly,
> > and no replacement buffer is allocated.
> > A separate data_seg_len accumulator tracks the total length
> > of delivered segments only.
> > The packet length is adjusted to reflect only the data
> > actually delivered to the application.
> > 
> > Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > ---
> 
> AI review with Opus 4.8 and High setting found one issue:
> 
> Patch 6: net/mlx5: support selective Rx
> 
> Error: NULL pointer dereference when the first configured Rx segment is a
> discard segment (mp == NULL).
> 
> In mlx5_rx_burst() the head mbuf and the chain tail are tracked like this:
> 
> if (pkt) {
>     if (rep->pool)
>         NEXT(tail) = rep;
>     else
>         --NB_SEGS(pkt);
> }
> ...
> if (seg->pool) {
>     tail = seg;
>     ...
> }
> 
> tail is only ever assigned inside "if (seg->pool)", and pkt is set to the
> first processed segment unconditionally (pkt = seg in the !pkt block, no
> pool guard). So if the first segment of a packet is a discard segment:
> 
>     pkt becomes the null_mbuf (pool == NULL), tail stays NULL;
>     on the next (real) segment, rep->pool is set, so NEXT(tail) = rep executes with tail == NULL -> write through NULL.
> 
> Even without the crash, returning the pool-less null_mbuf as the packet
> head is wrong: the application later frees it back to a NULL pool.
> 
> This is reachable, not theoretical. testpmd (patch 3) inserts a leading
> mp==NULL segment whenever the first offset is > 0 (seg_offset > next_offset
> with next_offset starting at 0), ethdev check_split (patch 2) now permits a
> leading NULL mp, and mlx5_rxq_new() accepts it (first_mp is just the first
> non-NULL pool; there is no requirement that rxseg[0].mp != NULL). The DTS
> cases selective_rx_payload_only (rxoffs=[34]) and selective_rx_two_segments
> (rxoffs=[14,...]) in patch 10 configure exactly this layout, and
> mlx5_selective_rx_enabled() forces the scalar mlx5_rx_burst path, so the
> buggy path is the one that runs.
> 
> Trace for rxoffs=34 / rxpkts=payload (segments: discard[0,34) real[34,290)
> discard[290,max)):
> 
> iter0 (discard head): pkt == NULL, seg->pool == NULL -> pkt = null_mbuf,
> tail not set; len(290) > DATA_LEN(34) -> ++NB_SEGS, continue.
> iter1 (real seg):      pkt set, rep->pool != NULL -> NEXT(tail==NULL)=rep.
> 
> Suggested fix: a discard segment must not become the packet head/tail.
> Either reject rxseg[0].mp == NULL in mlx5_rxq_new() (cleanest, matches the
> "deliver last N bytes" case being unsupported here), or make the data path
> skip leading discard segments without assigning them to pkt and only set
> pkt/tail on the first segment with a pool. If leading discard is intended
> to be supported, the head selection and NEXT(tail) linking both need to
> account for tail == NULL.
> 
> The same head/tail assumption also means a packet that falls entirely
> within a leading discard segment would be returned with a NULL-pool head;
> fixing the above covers that too.

Yes sorry for missing that.
I fix it by assigning packet after the first real segment.
I take this opportunity to add few comments on the branching conditions.