bugs in rte_pktmbuf_copy

bugs in rte_pktmbuf_copy

[Morten Brørup]

While working on a rte_pktmbuf_copy_bulk() function, I noticed a couple of bugs in rte_pktmbuf_copy():

1. If the copy is allocated from a mempool using pinned external buffers, the copy's RTE_MBUF_F_EXTERNAL flag is lost.
This is simple to fix:

-	/* 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 */
+	mc->ol_flags |= m->ol_flags & ~(RTE_MBUF_F_INDIRECT|RTE_MBUF_F_EXTERNAL);

2. If the packet is copied with non-zero offset, much of the metadata do not apply to the copy, e.g. many of the offload flags and the packet_type field.
Maybe metadata should be reset when copying with a non-zero offset?
Or maybe rte_pktmbuf_copy() should be considered "payload copy", so the copy should have all metadata reset?


Venlig hilsen / Kind regards,
-Morten Brørup

Re: bugs in rte_pktmbuf_copy

[Stephen Hemminger]

On Mon, 17 Nov 2025 12:51:51 +0100
Morten Brørup <mb@smartsharesystems.com> wrote:

> While working on a rte_pktmbuf_copy_bulk() function, I noticed a couple of bugs in rte_pktmbuf_copy():
> 
> 1. If the copy is allocated from a mempool using pinned external buffers, the copy's RTE_MBUF_F_EXTERNAL flag is lost.
> This is simple to fix:
> 
> -	/* 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 */
> +	mc->ol_flags |= m->ol_flags & ~(RTE_MBUF_F_INDIRECT|RTE_MBUF_F_EXTERNAL);

Good catch, external flag needs to be preserved. 


> 2. If the packet is copied with non-zero offset, much of the metadata do not apply to the copy, e.g. many of the offload flags and the packet_type field.
> Maybe metadata should be reset when copying with a non-zero offset?
> Or maybe rte_pktmbuf_copy() should be considered "payload copy", so the copy should have all metadata reset?

The use case for using offset, is for removing tunnel headers. I expect that the code doing so
would know what manipulation of offloads was required. 

RE: bugs in rte_pktmbuf_copy

[Morten Brørup]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, 18 November 2025 00.43
> 
> On Mon, 17 Nov 2025 12:51:51 +0100
> Morten Brørup <mb@smartsharesystems.com> wrote:
> 
> > While working on a rte_pktmbuf_copy_bulk() function, I noticed a
> couple of bugs in rte_pktmbuf_copy():
> >
> > 1. If the copy is allocated from a mempool using pinned external
> buffers, the copy's RTE_MBUF_F_EXTERNAL flag is lost.
> > This is simple to fix:
> >
> > -	/* 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 */
> > +	mc->ol_flags |= m->ol_flags &
> ~(RTE_MBUF_F_INDIRECT|RTE_MBUF_F_EXTERNAL);
> 
> Good catch, external flag needs to be preserved.
> 
> 
> > 2. If the packet is copied with non-zero offset, much of the metadata
> do not apply to the copy, e.g. many of the offload flags and the
> packet_type field.
> > Maybe metadata should be reset when copying with a non-zero offset?
> > Or maybe rte_pktmbuf_copy() should be considered "payload copy", so
> the copy should have all metadata reset?
> 
> The use case for using offset, is for removing tunnel headers. I expect
> that the code doing so
> would know what manipulation of offloads was required.

IMO, rte_pktmbuf_copy() should behave the same way as rte_pktmbuf_clone(), which doesn't copy any metadata to the clone.
Reusing your argument: the application would know what manipulation of metadata was required.
WDYT?

Having looked deeper into rte_pktmbuf_copy() now, I think the support for partial copy (offset and length parameters) might have been a mistake, for multiple reasons:
- The rte_pktmbuf_clone() doesn't support partial payload cloning.
- The Kernel's skb_copy() doesn't support partial copy. There are other functions for that.
- If used for tunnels, it only supports decapsulation; it lacks support for encapsulation, i.e. a parameter for specifying extra headroom in the allocated "copy" packet mbuf.
On the positive side, partial copy is useful for packet capturing with header stripping and snap length. So not all bad. :-)

Anyway, removing the offset and length parameters would break the API and ABI, so that's too late now.