Re: [PATCH] bpf: Unshare cloned skb before devmap egress XDP program

[Menglong Dong]

On 2026/6/10 08:06 Emil Tsalapatis <emil@xxxxxxxxxxxxxxx> write:
> On Tue Jun 9, 2026 at 7:06 AM EDT, Menglong Dong wrote:
> > On 2026/6/9 18:02 Sun Jian <sun.jian.kdev@xxxxxxxxx> write:
> >> dev_map_redirect_clone() uses skb_clone() when redirecting a generic XDP
> >> skb to multiple devmap destinations. The cloned skb can share packet data
> >> with other clones.
> >> 
> >> If the destination devmap entry has an egress XDP program, that program
> >> can modify packet data. Such modifications can then be observed by other
> >> clones sharing the same packet data.
> >> 
> >> This can be reproduced by strengthening xdp_veth_egress to configure a
> >> different source MAC for each egress device and checking that store_mac_1/2
> >> observe the MAC configured for their own egress devices. Without the fix,
> >> the SKB_MODE subtest observes store_mac_1 receiving the MAC configured for
> >> the next egress device.
> >> 
> >> Fix this by unsharing the cloned skb before running the devmap egress XDP
> >> program. Limit the extra copy to destinations with an attached egress
> >> program.
> >
> > Hi, Jian.
> >
> > This sounds like a good idea in this case. When I have a look at bpf_clone_redirect(),
> > I found that it use skb_clone() too, which means it has the same problem. The
> > data can be modified by other xdp prog in the destination NIC if we use
> > bpf_clone_redirect().
> >
> > So maybe this is the default logic, and I'm not sure if this patch can break the
> > existing users :/
> 
> I think for use cases where we are using bpf_clone_redirect() to use
> one clone for inspection this would add an unnecessary copy. Maybe
> adding *_copy() variants instead of changing the *_clone() would be
> better? That way we wouldn't be changing the behavior for existing
> consumers and the naming would be consistent with the skb_* methods.

Agree. It's not a good idea to change the logic of the existing API. Or
maybe we can add a BPF_F_CLONE flag for the existing API.

> 
> But more importantly, is there an actual use case for the kind of API
> that the modified selftest requires? Nobody until now has considered
> the existing behavior to be a problem.

Agree too. Obviously, this is not a bug. For the use case in the commit
log, it's something that can be fixed by the user themself. If we need
modify the MAC, we'd better attach a BPF program for all the egress
device in the devmap.

> 
> >
> > Thanks!
> > Menglong Dong
> >
> >> 
> >> Tested with:
> >>   ./test_progs -t xdp_veth_egress
> >>   ./test_progs -t xdp_veth
> >>   ./test_progs -t xdp
> > [...]
> >>  
> >>  destroy_xdp_redirect_map:
> >> -- 
> >> 2.43.0
> >> 
> >> 
> >> 
>