[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

RE: [PATCH] mbuf: fix packet copy

[Konstantin Ananyev]

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

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

Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>

> > 2.43.0

FW: [PATCH] mbuf: fix packet copy

[Morten Brørup]

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;
 
 	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]

Forwarding an email is apparently a bad idea; patchwork picked it up as a new patch.

Venlig hilsen / Kind regards,
-Morten Brørup


> -----Original Message-----
> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Thursday, 15 January 2026 11.53
> To: Stephen Hemminger
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: FW: [PATCH] mbuf: fix packet copy
> 
> 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;
> 
>  	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

[Stephen Hemminger]

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.

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


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

RE: [PATCH] mbuf: fix packet copy

[Morten Brørup]

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