RE: [PATCH] mbuf: fix packet copy

[Konstantin Ananyev <konstantin.ananyev@huawei.com>]

> > > > 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;
> >
> > So, when off= m->pkt_len, what do we want it to return?
> > New mbuf with pkt_len == 0?
> > Any point of such copying then?
> 
> Empty packets are perfectly valid.
> A library should not optimize them away; it is the application's choice how to handle
> empty packets.
> 
> >
> > > >  	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);
> >
> > Which flags in the new mbuf we want to preserve?
> > AFAIK mbuf_alloc() doesn't set any flags.
> 
> Correct: the newly allocated mbuf returned by mbuf_alloc() normally has no flags
> set.
> However, if it is allocated from an mbuf pool holding mbufs using pinned external
> buffers, the RTE_MBUF_F_EXTERNAL is already set in the mbufs held in that pool,
> and will remain set when returning the mbuf by mbuf_alloc(). We want to preserve
> that flag.
> 
> > BTW, if there are some flags that we would like to preserve,
> > wouldn't that be a  change in public API behavior?
> 
> This patch does not change which flags to preserve from the source mbuf.
> The patch only adds that the flags of the newly allocated mbuf are not cleared when
> copying the flags from the source mbuf.

Makes sense, thanks for clarifying.