Re: [PATCH] mbuf: fix packet copy

[Stephen Hemminger <stephen@networkplumber.org>]

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;