RE: [PATCH] mbuf: fix packet copy

Konstantin Ananyev
2026-01-12T10:56:40Z

> > > > 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.