[PATCH] mbuf: fix packet copy

[PATCH] mbuf: fix packet copy

[Morten Brørup]

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;
 
 	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);
 
 	prev = &mc->next;
 	m_last = mc;
-- 
2.43.0

RE: [PATCH] mbuf: fix packet copy

[Morten Brørup]

Recheck-request: iol-compile-amd64-testing

RE: [PATCH] mbuf: fix packet copy

[Morten Brørup]

Recheck-request: iol-compile-amd64-testing

RE: [PATCH] mbuf: fix packet copy

[Morten Brørup]

Recheck-request: iol-compile-amd64-testing

RE: [PATCH] mbuf: fix packet copy

[Morten Brørup]

PING for review.

> 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;
> 
>  	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);
> 
>  	prev = &mc->next;
>  	m_last = mc;
> --
> 2.43.0

RE: [PATCH] mbuf: fix packet copy

[Konstantin Ananyev]

> 
> PING for review.
> 
> > 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? 

> >  	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.
BTW, if there are some flags that we would like to preserve,
wouldn't that be a  change in public API behavior?

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

RE: [PATCH] mbuf: fix packet copy

[Morten Brørup]

> From: Konstantin Ananyev [mailto:konstantin.ananyev@huawei.com]
> 
> >
> > PING for review.
> >
> > > 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.

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