RE: [PATCH] mbuf: fix packet copy

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Friday, 16 January 2026 06.55
> 
> On Thu, 15 Jan 2026 11:53:19 +0100
> Morten Brørup <mb@smartsharesystems.com> wrote:
> 
> > Stephen,
> >
> > As the author of rte_pktmbuf_copy(), can you please review this
> patch?
> >
> > You might find my answers to Konstantin's review informative:
> > https://patchwork.dpdk.org/project/dpdk/patch/20251119120403.907511-
> 1-mb@smartsharesystems.com/#181914
> >
> >
> > Venlig hilsen / Kind regards,
> > -Morten Brørup
> >
> >
> > From: Morten Brørup [mailto:mb@smartsharesystems.com]
> > Sent: Wednesday, 19 November 2025 13.04
> >
> > Requests for copying the at the end of a packet incorrectly returned
> NULL,
> > as if copying past the end of a packet.
> >
> > When allocating copies from a mempool using pinned external buffers,
> the
> > external flag was not preserved in these mbufs.
> >
> > Fixes: c3a90c381daa ("mbuf: add a copy routine")
> >
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > ---
> >  lib/mbuf/rte_mbuf.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/mbuf/rte_mbuf.c b/lib/mbuf/rte_mbuf.c
> > index 0d931c7a15..e639aff03e 100644
> > --- a/lib/mbuf/rte_mbuf.c
> > +++ b/lib/mbuf/rte_mbuf.c
> > @@ -675,7 +675,7 @@ rte_pktmbuf_copy(const struct rte_mbuf *m, struct
> rte_mempool *mp,
> >  	__rte_mbuf_sanity_check(m, 1);
> >
> >  	/* check for request to copy at offset past end of mbuf */
> > -	if (unlikely(off >= m->pkt_len))
> > +	if (unlikely(off > m->pkt_len))
> >  		return NULL;
> 
> It makes more sense to return NULL (as error) rather than creating a 0
> length mbuf in this corner case.

As replied to Kontantin, 0 length buffers are perfectly valid, so a library should not optimize them away, on an assumption that they are useless.
E.g. consider TCP, which carries many empty packets, adding feedback information (ACK, SACK) to the TCP header. I know it's a very theoretical example stripping all headers and then adding headers again; but I can creatively imagine something like that.
If a normal application don't want to deal with 0 length buffers, it can optimize them away.
But if an exotic application does want to deal with 0 length buffers, it would be a bug if the library optimized them away.

> 
> >  	mc = rte_pktmbuf_alloc(mp);
> > @@ -688,8 +688,8 @@ rte_pktmbuf_copy(const struct rte_mbuf *m, struct
> rte_mempool *mp,
> >
> >  	__rte_pktmbuf_copy_hdr(mc, m);
> >
> > -	/* copied mbuf is not indirect or external */
> > -	mc->ol_flags = m->ol_flags &
> ~(RTE_MBUF_F_INDIRECT|RTE_MBUF_F_EXTERNAL);
> > +	/* copy flags except indirect and external, and preserve flags of
> newly allocated mbuf */
> > +	mc->ol_flags |= m->ol_flags &
> ~(RTE_MBUF_F_INDIRECT|RTE_MBUF_F_EXTERNAL);
> 
> Should have space in expression.

Yes. Copy-paste bug. :-)

> At that point it is a new mbuf (the copy) so offload flags should be
> clear, not sure
> what the issue is here.
> But hadn't expected usage of this function with an external mbuf pool.

I consider pinned external buffers exotic too, but we need to support them throughout DPDK.
That's the downside of exotic features in core libraries.

> 
> 
> >
> >  	prev = &mc->next;
> >  	m_last = mc;