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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch incompletely fixes shared SKB data corruption in generic XDP devmap redirection. When broadcasting to multiple destinations, the original `skb` is used for the final destination (`last_dst`). If `last_dst` has an egress XDP program, it runs directly on the original `skb` and modifies its packet data in-place without checking if it is shared. If prior destinations lacked an egress XDP program, their clones were not unshared by the new logic in `dev_map_redirect_clone()`. Consequently, they still share the same packet data buffer as the original `skb`, and the final destination's egress program will silently corrupt the packet data of the earlier destinations' clones.
--

commit 280ec1e15889a77555ac8877632a39ede4e76d91
Author: Sun Jian <sun.jian.kdev@gmail.com>

bpf: Unshare cloned skb before devmap egress XDP program

This commit modifies dev_map_redirect_clone() to unshare cloned skbs if the
destination devmap entry has an egress XDP program. This prevents modifications
by the egress XDP program from being observed by other clones sharing the same
packet data.

> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> index cc0a43ebab6b9..4ae65d44f9d6e 100644
> --- a/kernel/bpf/devmap.c
> +++ b/kernel/bpf/devmap.c
> @@ -730,6 +730,12 @@ static int dev_map_redirect_clone(struct bpf_dtab_netdev *dst,
>  	if (!nskb)
>  		return -ENOMEM;
>  
> +	if (dst->xdp_prog) {
> +		nskb = skb_unshare(nskb, GFP_ATOMIC);
> +		if (!nskb)
> +			return -ENOMEM;
> +	}
> +

[Severity: High]
This is a pre-existing issue, but does this logic still leave the original skb
vulnerable to data corruption during broadcast?

In dev_map_redirect_multi(), the loop clones the skb for N-1 destinations,
and then calls dev_map_generic_redirect() on the original skb for the final
destination:

kernel/bpf/devmap.c:dev_map_redirect_multi() {
    ...
    /* consume the first skb and return */
    if (last_dst)
        return dev_map_generic_redirect(last_dst, skb, xdp_prog);
    ...
}

If the earlier destinations lack an egress XDP program, dev_map_redirect_clone()
will not unshare their clones. If the final destination (last_dst) does have an
egress XDP program, dev_map_bpf_prog_run_skb() will run directly on the original
skb and modify its packet data in-place without ensuring the buffer is unshared:

kernel/bpf/devmap.c:dev_map_bpf_prog_run_skb() {
    ...
    act = bpf_prog_run_generic_xdp(skb, &xdp, dst->xdp_prog);
    ...
}

Could this silently corrupt or truncate the packet data for the earlier
clones that still share the same buffer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609100214.337538-1-sun.jian.kdev@gmail.com?part=1