FW: [PATCH] mbuf: fix packet copy

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;