Re: [PATCH net] octeontx2-pf: fix null RX segment address free

[Simon Horman <horms@kernel.org>]

On Wed, Sep 09, 2026 at 02:36:52AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 2 · Medium: 0 · Low: 0
> 
> - [High] Incomplete fix: the zero-`seg_addr` guard is added only to
>   `otx2_free_rcv_seg()`, leaving the bug class the commit message claims…
> - [High] The new guard in `otx2_free_rcv_seg()` (otx2_txrx.c:260-261)
>   uses `return`, which aborts the inner per-segment loop *and* the outer…

Hi Nitin,

I think that these days it's best practice for authors to respond to reviews
from netdev-bot+sashiko. But let me give it a shot.

> 
> --
> 
> octeontx2-pf: fix null RX segment address free
> 
> The patch adds a check in otx2_free_rcv_seg() so that a segment address
> which reads back as zero is not handed to aura_freeptr().  It targets the
> multi-segment reclaim path introduced by b1bc8457e9d0.
> 
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> > index 8d2d607bc92f..a9aefd14ae52 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> > @@ -256,9 +256,12 @@ static void otx2_free_rcv_seg(struct otx2_nic *pfvf, struct nix_cqe_rx_s *cqe,
> >  	while (start < end) {
> >  		sg = (struct nix_rx_sg_s *)start;
> >  		seg_addr = &sg->seg_addr;
> > -		for (seg = 0; seg < sg->segs; seg++, seg_addr++)
> > +		for (seg = 0; seg < sg->segs; seg++, seg_addr++) {
> > +			if (unlikely(!*seg_addr))
> > +				return;
> 
> [Severity: High]
> Should this be continue instead of return?
> 
> The return leaves both the inner per-segment loop and the outer
> while (start < end) walk over the remaining SG sub-descriptors, so if
> slot N reads zero, slots N+1..segs-1 and every later SG word inside
> (desc_sizem1 + 1) * 16 are never returned to the aura.

Looking over the implementation of otx2_rx_napi_handler(),
which only zero's the first segment address, it seems to
me that return is the right approach. As subsequent seg_addrs will
be neither zero nor value if processed by otx2_rx_napi_handler().

However, if this is the case, I wonder if it would be somehow
nicer to only apply the zero condition outside the loops.

Something like this (completely untested!).

@@ -251,6 +251,10 @@ static void otx2_free_rcv_seg(struct otx2_nic *pfvf, struct nix_cqe_rx_s *cqe,
 	u64 *seg_addr;
 	int seg;

+	/* If the first seg_addr is 0, then there is no data to free */
+	if (unlikely(!sg->seg_addr))
+		return;
+
 	start = (void *)sg;
 	end = start + ((cqe->parse.desc_sizem1 + 1) * 16);
 	while (start < end) {

> 
> struct nix_rx_sg_s in otx2_struct.h describes segs as a count, not a
> null-terminated list:
> 
> 	u64 segs       : 2;
> 	...
> 	u64 seg_addr;
> 	u64 seg2_addr;
> 	u64 seg3_addr;
> 
> and the peer walk in otx2_rcv_pkt_handler() processes every advertised
> slot of every SG word, so the two readers of the same descriptor now
> disagree about its meaning.
> 
> Neither caller re-scans the CQE afterwards.  otx2_check_rcv_errors()
> does:
> 
> 	/* Free buffer back to pool */
> 	if (cqe->sg.segs)
> 		otx2_free_rcv_seg(pfvf, cqe, qidx);
> 	return true;
> 
> and otx2_cleanup_rx_cqes() bumps processed_cqe, continues, and later
> rings the CQ doorbell.  Can the skipped pointers leak the RX buffers
> here, i.e. lose them from the NPA aura and, on the teardown path, leave
> pages charged to the page_pool with their DMA mappings still installed
> so page_pool_destroy() sees inflight pages?
> 
> The commit message describes a per-segment condition ("when the segment
> address is null"), which reads more like a skip of the single bad slot
> than an abort of the whole reclaim loop.
> 
> >  			pfvf->hw_ops->aura_freeptr(pfvf, qidx,
> >  						   *seg_addr & ~0x07ULL);
> > +		}
> >  		start += sizeof(*sg);
> >  	}
> >  }
> 
> [Severity: High]
> If a segment address in an RX CQE can be observed as zero, does the fix
> cover the other readers of the same descriptor words?
> 
> otx2_rcv_pkt_handler() performs the identical walk with no zero test at
> all:
> 
> 	start = (void *)sg;
> 	end = start + ((cqe->parse.desc_sizem1 + 1) * 16);
> 	while (start < end) {
> 		sg = (struct nix_rx_sg_s *)start;
> 		seg_addr = &sg->seg_addr;
> 		seg_size = (void *)sg;
> 		for (seg = 0; seg < sg->segs; seg++, seg_addr++) {
> 			if (otx2_skb_add_frag(pfvf, skb, *seg_addr,
> 					      seg_size[seg], parse, cq->cq_idx))
> 				cq->pool_ptrs++;
> 		}
> 		start += sizeof(*sg);
> 	}
> 
> The pre-check in otx2_rx_napi_handler() only looks at the first word:
> 
> 		if (cqe->hdr.cqe_type == NIX_XQE_TYPE_INVALID ||
> 		    !cqe->sg.seg_addr) {
> 
> so seg2_addr, seg3_addr and any later SG sub-descriptor reach
> otx2_skb_add_frag() unchecked.  With a zero iova that function does:
> 
> 	va = phys_to_virt(otx2_iova_to_phys(pfvf->iommu_domain, iova));
> 	...
> 	page = virt_to_page(va);
> 
> and publishes a page the driver never allocated into the GRO skb, with
> skb_mark_for_recycle() run on it later.  When nr_frags has reached
> MAX_SKB_FRAGS it also does:
> 
> 	pfvf->hw_ops->aura_freeptr(pfvf, qidx, iova & ~0x07ULL);
> 
> which is the same invalid free the commit message says it is preventing,
> only on the fast path.  Can this happen?
> 
> The same question applies to the caller that motivates this change.  In
> otx2_cleanup_rx_cqes() only the multi-segment case goes through the
> patched helper:
> 
> 		if (cqe->sg.segs > 1) {
> 			otx2_free_rcv_seg(pfvf, cqe, cq->cq_idx);
> 			continue;
> 		}
> 		iova = cqe->sg.seg_addr - OTX2_HEAD_ROOM;
> 
> 		otx2_free_bufs(pfvf, pool, iova, pfvf->rbsize);
> 
> With seg_addr == 0 the subtraction underflows, and otx2_free_bufs() in
> otx2_common.c then does virt_to_head_page(phys_to_virt(pa)) followed by
> page_pool_put_full_page() / dma_unmap / put_page() on a page derived from
> that address.  Does this branch need the same guard?

OTOH, this concern does seem to warrant addressing.