> -----Original Message----- > From: Stephen Hemminger <[email protected]> > Sent: Tuesday, 24 March 2026 00:33 > To: Sriram Yagnaraman <[email protected]> > Cc: [email protected]; [email protected]; [email protected]; [email protected] > Subject: Re: [PATCH] net/memif: fix multi-segment RX data corruption and > truncation > > On Mon, 16 Mar 2026 16:59:18 +0100 > Sriram Yagnaraman <[email protected]> wrote: > > > Fix dst_off being reset per-descriptor instead of per-packet in the RX > > slow path. When processing chained descriptors > (MEMIF_DESC_FLAG_NEXT), > > goto next_slot2 reset dst_off to 0, overwriting the beginning of the > > current mbuf with data from subsequent descriptors. Move dst_off > > initialization before the next_slot2 label so it is only reset once > > per packet. > > > > Add boundary check in both RX paths before processing next segment. > > If MEMIF_DESC_FLAG_NEXT is set but n_slots is 0, free the incomplete > > mbuf chain and exit gracefully to prevent reading beyond available > > descriptors. > > > > Bugzilla ID: 1609 > > Fixes: aa17df860891 ("net/memif: add a Rx fast path") > > Cc: [email protected] > > > > Reported-by: Mike Bly <[email protected]> > > Signed-off-by: Sriram Yagnaraman <[email protected]> > > It looked good to me, but AI review spotted an issue. > > > --- > > OK, so my error finding stands but with refined reasoning. Here's the > corrected summary: > > **Error: Double free of `mbuf_head` in the fast-path truncation error path.** > > In the new truncation check added to the fast path: > ```c > if (unlikely(n_slots == 0)) { > MIF_LOG(ERR, "Truncated packet: ..."); > rte_pktmbuf_free(mbuf_head); > rte_pktmbuf_free_bulk(mbufs + rx_pkts, > MAX_PKT_BURST - rx_pkts); > goto no_free_bufs; > } > ``` > > `mbuf_head` is `mbufs[rx_pkts]` (assigned at line 359). > `rte_pktmbuf_free(mbuf_head)` frees the head and its entire chain. Then > `rte_pktmbuf_free_bulk(mbufs + rx_pkts, ...)` processes the same array > starting at `mbufs[rx_pkts]`, which still holds the now-freed pointer. Since > `rte_pktmbuf_free_bulk` walks `m->next` chains (confirmed in > `lib/mbuf/rte_mbuf.c:574`), this frees `mbuf_head` a second time. > > The existing error paths at lines 383-386 and 391-394 do NOT have this bug — > they don't call `rte_pktmbuf_free(mbuf_head)` first; they let > `rte_pktmbuf_free_bulk` handle the entire chain. The fix is to remove the > separate `rte_pktmbuf_free(mbuf_head)` and just do: > ```c > rte_pktmbuf_free_bulk(mbufs + rx_pkts, > MAX_PKT_BURST - rx_pkts); > ``` > > This works because `rte_pktmbuf_free_bulk` already walks the chain on > `mbufs[rx_pkts]`, freeing `mbuf_head` and all segments chained onto it, plus > the remaining unused mbufs. >
Thanks Stephen, it is a good catch, I corrected this and posted a v2. > No other issues found. The `dst_off` fix and the slow-path bounds check are > both correct.
