Skip to content

bpf: Run generic devmap egress prog on private skb#12428

Open
kernel-patches-daemon-bpf[bot] wants to merge 1 commit into
bpf_basefrom
series/1109185=>bpf
Open

bpf: Run generic devmap egress prog on private skb#12428
kernel-patches-daemon-bpf[bot] wants to merge 1 commit into
bpf_basefrom
series/1109185=>bpf

Conversation

@kernel-patches-daemon-bpf

Copy link
Copy Markdown

Pull request for series with
subject: bpf: Run generic devmap egress prog on private skb
version: 2
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1109185

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: e7ae89a
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1109185
version: 2

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: e7ae89a
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1109185
version: 2

Generic XDP devmap multi redirect uses skb_clone() for the
intermediate destinations and sends the last destination with the
original skb. This can leave multiple destinations sharing the same
packet data.

This becomes visible when a devmap egress program mutates packet data.
One destination can observe changes made for another destination. The
last-destination path has the same problem: the last destination runs on
the original skb, so its egress program can modify packet data still
shared with earlier cloned skbs.

Native XDP broadcast redirect does not have this issue because
xdpf_clone() copies the frame data for each destination. Generic XDP
should provide the same per-destination isolation before running a
devmap egress program.

Fix this by making cloned skbs private in dev_map_generic_redirect()
before running the devmap egress program. Use skb_copy() instead of
skb_unshare() so that allocation failure does not consume the skb and
the existing caller error paths keep their ownership semantics.

Add a selftest that covers the last-destination case where earlier
destinations do not have a devmap egress program, while the final
destination does.

Tested with:
  ./test_progs -t xdp_veth_egress
  ./test_progs -t xdp_veth
  ./test_progs -t xdp

Fixes: e624d4e ("xdp: Extend xdp_redirect_map with broadcast support")
Suggested-by: Jiayuan Chen <jiayuan.chen@linux.dev>
Signed-off-by: Sun Jian <sun.jian.kdev@gmail.com>
Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
@kernel-patches-review-bot

Copy link
Copy Markdown
commit eb18dbeefdd70b12114025e87857918f03ff3162
Author: Sun Jian <sun.jian.kdev@gmail.com>

bpf: Run generic devmap egress prog on private skb

This makes cloned skbs private in dev_map_generic_redirect() before
running a devmap egress program, so one destination's egress program
cannot observe or mutate packet data still shared with other cloned
destinations. It uses skb_copy() rather than skb_unshare() so an
allocation failure does not consume the skb, and adds a selftest for the
last-destination case.

>     Generic XDP devmap multi redirect uses skb_clone() for the
>     intermediate destinations and sends the last destination with the
>     original skb. This can leave multiple destinations sharing the same
>     packet data.
>     This becomes visible when a devmap egress program mutates packet data.
>     One destination can observe changes made for another destination. The
>     last-destination path has the same problem: the last destination runs on
>     the original skb, so its egress program can modify packet data still
>     shared with earlier cloned skbs.
>     Native XDP broadcast redirect does not have this issue because
>     xdpf_clone() copies the frame data for each destination. Generic XDP
>     should provide the same per-destination isolation before running a
>     devmap egress program.
>     Fix this by making cloned skbs private in dev_map_generic_redirect()
>     before running the devmap egress program. Use skb_copy() instead of
>     skb_unshare() so that allocation failure does not consume the skb and
>     the existing caller error paths keep their ownership semantics.
>     Add a selftest that covers the last-destination case where earlier
>     destinations do not have a devmap egress program, while the final
>     destination does.
>     Fixes: e624d4ed4aa8 ("xdp: Extend xdp_redirect_map with broadcast support")
>     Suggested-by: Jiayuan Chen <jiayuan.chen@linux.dev>
>     Signed-off-by: Sun Jian <sun.jian.kdev@gmail.com>

Is the Fixes: tag pointing at the right commit?

The tag references e624d4ed4aa8 ("xdp: Extend xdp_redirect_map with
broadcast support"), which introduced the generic-XDP multi/broadcast
redirect that clones skbs and sends the last destination on the original
skb. At that commit, though, dev_map_generic_redirect() only called
generic_xdp_tx() and ran no per-destination program, so sharing skb data
between clones was harmless.

The corruption being fixed only appears once a devmap egress program
runs on the shared skb data. That egress-program execution in the
generic path (dev_map_bpf_prog_run_skb(), gated on dst->xdp_prog) was
added later by 2ea5eabaf04a ("bpf: devmap: Implement devmap prog
execution for generic XDP"). The fix here is itself gated on
dst->xdp_prog && skb_cloned(skb), matching the commit message statement
that the problem "becomes visible when a devmap egress program mutates
packet data".

Would this be more accurate?

  Fixes: 2ea5eabaf04a ("bpf: devmap: Implement devmap prog execution for generic XDP")

e624d4ed4aa8 provides the clone-sharing infrastructure and could be kept
as an additional reference, but 2ea5eabaf04a is the commit that
introduced the writer into the shared-data path.


AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

In-Reply-To-Subject: bpf: Run generic devmap egress prog on private skb
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/27270693851

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant