* [PATCH net v2] bpf: Fix skb_pull() BUG in __bpf_redirect_neigh()
@ 2026-10-29 9:36 Jamal Hadi Salim
2026-10-30 21:47 ` Alexei Starovoitov
2026-11-02 9:52 ` netdev-bot+sashiko
0 siblings, 2 replies; 5+ messages in thread
From: Jamal Hadi Salim @ 2026-10-29 9:36 UTC (permalink / raw)
To: netdev
Cc: Jamal Hadi Salim, Eric Dumazet, bpf, bot+bpf-ci, Daniel Borkmann,
Alexei Starovoitov, David Ahern, Martin KaFai Lau,
Martin KaFai Lau, John Fastabend, Stanislav Fomichev,
Andrii Nakryiko, Eduard Zingerman, Kumar Kartikeya Dwivedi,
Song Liu, Yonghong Song, Jiri Olsa, Emil Tsalapatis,
Ihor Solodrai, David S . Miller, Jakub Kicinski, Paolo Abeni,
Simon Horman, Jiri Pirko, Victor Nogueira, linux-kselftest,
Shuah Khan, stable
The issue:
This is a follow-up to the short link-layer header fixes from Eric.
After vlan pop_eth, a privileged SCHED_ACT BPF program can call
bpf_skb_change_head(skb, 1, 0) and then bpf_redirect_neigh().
__bpf_redirect_neigh() calls skb_pull(skb, sizeof(*ethh)) after checking
only that mac_header < network_header. It does not check that the MAC
header is long enough or that it is linear.
A SCHED_ACT BPF program can do:
bpf_skb_change_head(skb, 1, 0);
bpf_redirect_neigh(lo, NULL, 0, 0);
after vlan pop_eth. The skb then has mac_len == 1 and only one byte of
linear data; skb_pull(ETH_HLEN) trips the skb->len < skb->data_len BUG in
__skb_pull():
kernel BUG at include/linux/skbuff.h:2839
RIP: skb_pull+0xc5/0x100
Call Trace: skb_do_redirect -> ...
The Fix:
Use skb_mac_header_len(), i.e. network_header - mac_header, rather than
skb->mac_len. mac_len is zero for locally generated tc egress and netkit
packets, while skb_mac_header_len() still rejects the one-byte header
produced by pop_eth + bpf_skb_change_head(skb, 1, 0).
Also use pskb_may_pull() to ensure the Ethernet header is linear before
reading or pulling it, and re-read ethh afterwards since pskb_may_pull()
may reallocate skb->head.
Add egress coverage to tc_redirect_neigh: the existing test only attaches
redirect_neigh at ingress, where mac_len is populated.
Conditions to recreate the bug:
On x86-64 with 4 KiB pages, enable CONFIG_PACKET, CONFIG_BPF_SYSCALL,
CONFIG_NET_SCH_INGRESS, CONFIG_NET_CLS_MATCHALL, CONFIG_NET_ACT_VLAN and
CONFIG_NET_ACT_BPF. Run as root with BPF loading, network administration
and raw-packet privileges. Bring lo up and attach a SCHED_ACT program
that calls bpf_skb_change_head(skb, 1, 0), then returns
bpf_redirect_neigh(skb->ifindex, NULL, 0, 0):
ip link set lo up
tc qdisc add dev lo clsact
tc filter add dev lo egress matchall action vlan pop_eth pipe \
action bpf object-file neigh_chghead.o section action
Send an untagged ETH_P_IP frame of ETH_HLEN + 8192 bytes through an
AF_PACKET/SOCK_RAW socket bound to lo, without PACKET_VNET_HDR or
PACKET_QDISC_BYPASS. Use a unicast destination MAC. The sender's skb
has a 14-byte linear Ethernet prefix and a fragmented payload; pop_eth
removes that prefix and change_head restores only one linear byte.
The unfixed neighbor redirect then tries to pull 14 bytes and BUGs.
Fixes: b4ab31414970 ("bpf: Add redirect_neigh helper as redirect drop-in")
Reported-by: BPF CI <bot+bpf-ci@kernel.org>
Link: https://lore.kernel.org/all/2a0f5ff1d8db81b07fc4b344b8e68e3505fcbf36f72a471d1678b2f1cecd881b@mail.kernel.org/
Link: https://lore.kernel.org/netdev/20261027082604.1485903-1-edumazet@kernel.org/
Cc: stable@vger.kernel.org
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
v2:
- use skb_mac_header_len() instead of mac_len; mac_len is zero for
locally generated tc egress and netkit packets (BPF CI)
- add tc-egress coverage to tc_redirect_neigh
- document the nonlinear AF_PACKET reproduction conditions
net/core/filter.c | 6 ++++-
.../selftests/bpf/prog_tests/tc_redirect.c | 26 +++++++++++++++++++
.../selftests/bpf/progs/test_tc_neigh.c | 10 +++++++
3 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index 70dc621672f2..4925f3e355d4 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2432,10 +2432,14 @@ static int __bpf_redirect_neigh_v4(struct sk_buff *skb, struct net_device *dev,
static int __bpf_redirect_neigh(struct sk_buff *skb, struct net_device *dev,
struct bpf_nh_params *nh)
{
- struct ethhdr *ethh = eth_hdr(skb);
+ struct ethhdr *ethh;
if (unlikely(skb->mac_header >= skb->network_header))
goto out;
+ if (unlikely(skb_mac_header_len(skb) < ETH_HLEN ||
+ !pskb_may_pull(skb, ETH_HLEN)))
+ goto out;
+ ethh = eth_hdr(skb);
bpf_push_mac_rcsum(skb);
if (is_multicast_ether_addr(ethh->h_dest))
goto out;
diff --git a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
index af8968b89ad7..829f84fb64a3 100644
--- a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
+++ b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
@@ -407,6 +407,26 @@ static int netns_load_bpf(const struct bpf_program *src_prog,
return -1;
}
+static int netns_load_egress_bpf(const struct bpf_program *prog,
+ const char *ns, int ifindex)
+{
+ LIBBPF_OPTS(bpf_tc_hook, qdisc);
+ struct nstoken *nstoken;
+ int err;
+
+ nstoken = open_netns(ns);
+ if (!ASSERT_OK_PTR(nstoken, "setns egress"))
+ return -1;
+
+ QDISC_CLSACT_CREATE(&qdisc, ifindex);
+ XGRESS_FILTER_ADD(&qdisc, BPF_TC_EGRESS, prog, 0);
+ close_netns(nstoken);
+ return 0;
+fail:
+ close_netns(nstoken);
+ return -1;
+}
+
static struct bpf_link *netns_attach_nk(const char *ns, int ifindex,
struct bpf_program *prog)
{
@@ -1055,6 +1075,12 @@ static void test_tc_redirect_neigh(struct netns_setup_result *setup_result)
skel->progs.tc_chk, setup_result))
goto done;
+ if (netns_load_egress_bpf(skel->progs.tc_egress, NS_SRC,
+ setup_result->ifindex_src) ||
+ netns_load_egress_bpf(skel->progs.tc_egress, NS_DST,
+ setup_result->ifindex_dst))
+ goto done;
+
if (!ASSERT_OK(set_forwarding(false), "disable forwarding"))
goto done;
diff --git a/tools/testing/selftests/bpf/progs/test_tc_neigh.c b/tools/testing/selftests/bpf/progs/test_tc_neigh.c
index de15155f2609..e0b6c14a3935 100644
--- a/tools/testing/selftests/bpf/progs/test_tc_neigh.c
+++ b/tools/testing/selftests/bpf/progs/test_tc_neigh.c
@@ -133,4 +133,14 @@ int tc_src(struct __sk_buff *skb)
return bpf_redirect_neigh(IFINDEX_DST, NULL, 0, 0);
}
+SEC("tc")
+int tc_egress(struct __sk_buff *skb)
+{
+ if (skb->mark)
+ return TC_ACT_OK;
+
+ skb->mark = 1;
+ return bpf_redirect_neigh(skb->ifindex, NULL, 0, 0);
+}
+
char __license[] SEC("license") = "GPL";
--
2.43.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net v2] bpf: Fix skb_pull() BUG in __bpf_redirect_neigh()
2026-10-29 9:36 [PATCH net v2] bpf: Fix skb_pull() BUG in __bpf_redirect_neigh() Jamal Hadi Salim
@ 2026-10-30 21:47 ` Alexei Starovoitov
2026-10-31 13:32 ` Jamal Hadi Salim
2026-11-02 9:52 ` netdev-bot+sashiko
1 sibling, 1 reply; 5+ messages in thread
From: Alexei Starovoitov @ 2026-10-30 21:47 UTC (permalink / raw)
To: Jamal Hadi Salim, netdev
Cc: Eric Dumazet, bpf, bot+bpf-ci, Daniel Borkmann, David Ahern,
Martin KaFai Lau, Martin KaFai Lau, John Fastabend,
Stanislav Fomichev, Andrii Nakryiko, Eduard Zingerman,
Kumar Kartikeya Dwivedi, Song Liu, Yonghong Song, Jiri Olsa,
Emil Tsalapatis, Ihor Solodrai, David S . Miller, Jakub Kicinski,
Paolo Abeni, Simon Horman, Jiri Pirko, Victor Nogueira,
linux-kselftest, Shuah Khan, stable
On Thu, Oct 29, 2026 at 02:36 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> Add egress coverage to tc_redirect_neigh: the existing test only attaches
> redirect_neigh at ingress, where mac_len is populated.
This test cannot hit the BUG.
Just drop it and respin as [PATCH bpf v3].
> static int __bpf_redirect_neigh(struct sk_buff *skb, struct net_device *dev,
> struct bpf_nh_params *nh)
> {
> - struct ethhdr *ethh = eth_hdr(skb);
> + struct ethhdr *ethh;
>
> if (unlikely(skb->mac_header >= skb->network_header))
> goto out;
> + if (unlikely(skb_mac_header_len(skb) < ETH_HLEN ||
> + !pskb_may_pull(skb, ETH_HLEN)))
> + goto out;
> + ethh = eth_hdr(skb);
ETH_HLEN is not enough.
__bpf_redirect_neigh_v4() and __bpf_redirect_neigh_v6() read ip_hdr()
and ipv6_hdr() right after the pull. Nothing makes these headers linear.
Take the AF_PACKET frame from the commit log without pop_eth and
bpf_skb_change_head():
packet_snd() headlen == 14, IP header in frags
skb_pull(skb, ETH_HLEN) headlen == 0
skb_reset_network_header(skb) network header == skb->tail
ip4h->daddr, ip4h->saddr read past skb->tail
The route lookup runs on garbage.
Eric's skb_network_offset() check also covers
mac_header >= network_header. No need for two checks.
Something like this:
diff --git a/net/core/filter.c b/net/core/filter.c
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2290,10 +2290,14 @@ static int bpf_out_neigh_v6(struct net *net, struct sk_buff *skb,
static int __bpf_redirect_neigh_v6(struct sk_buff *skb, struct net_device *dev,
struct bpf_nh_params *nh)
{
- const struct ipv6hdr *ip6h = ipv6_hdr(skb);
struct net *net = dev_net(dev);
int err, ret = NET_XMIT_DROP;
+ const struct ipv6hdr *ip6h;
+
+ if (unlikely(!pskb_may_pull(skb, sizeof(*ip6h))))
+ goto out_drop;
+ ip6h = ipv6_hdr(skb);
if (!nh) {
struct dst_entry *dst;
struct flowi6 fl6 = {
@@ -2399,10 +2403,14 @@ static int bpf_out_neigh_v4(struct net *net, struct sk_buff *skb,
static int __bpf_redirect_neigh_v4(struct sk_buff *skb, struct net_device *dev,
struct bpf_nh_params *nh)
{
- const struct iphdr *ip4h = ip_hdr(skb);
struct net *net = dev_net(dev);
int err, ret = NET_XMIT_DROP;
+ const struct iphdr *ip4h;
+
+ if (unlikely(!pskb_may_pull(skb, sizeof(*ip4h))))
+ goto out_drop;
+ ip4h = ip_hdr(skb);
if (!nh) {
struct flowi4 fl4 = {
.flowi4_flags = FLOWI_FLAG_ANYSRC,
@@ -2451,15 +2459,14 @@ static int __bpf_redirect_neigh_v4(struct sk_buff *skb, struct net_device *dev,
static int __bpf_redirect_neigh(struct sk_buff *skb, struct net_device *dev,
struct bpf_nh_params *nh)
{
- struct ethhdr *ethh = eth_hdr(skb);
-
- if (unlikely(skb->mac_header >= skb->network_header))
+ if (unlikely(skb_network_offset(skb) < ETH_HLEN ||
+ !pskb_may_pull(skb, ETH_HLEN)))
goto out;
bpf_push_mac_rcsum(skb);
- if (is_multicast_ether_addr(ethh->h_dest))
+ if (is_multicast_ether_addr(eth_hdr(skb)->h_dest))
goto out;
- skb_pull(skb, sizeof(*ethh));
+ skb_pull(skb, ETH_HLEN);
skb_unset_mac_header(skb);
skb_reset_network_header(skb);
pw-bot: cr
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net v2] bpf: Fix skb_pull() BUG in __bpf_redirect_neigh()
2026-10-30 21:47 ` Alexei Starovoitov
@ 2026-10-31 13:32 ` Jamal Hadi Salim
0 siblings, 0 replies; 5+ messages in thread
From: Jamal Hadi Salim @ 2026-10-31 13:32 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: netdev, Eric Dumazet, bpf, bot+bpf-ci, Daniel Borkmann,
David Ahern, Martin KaFai Lau, Martin KaFai Lau, John Fastabend,
Stanislav Fomichev, Andrii Nakryiko, Eduard Zingerman,
Kumar Kartikeya Dwivedi, Song Liu, Yonghong Song, Jiri Olsa,
Emil Tsalapatis, Ihor Solodrai, David S . Miller, Jakub Kicinski,
Paolo Abeni, Simon Horman, Jiri Pirko, Victor Nogueira,
linux-kselftest, Shuah Khan, stable
On Fri, Oct 30, 2026 at 5:47 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Oct 29, 2026 at 02:36 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > Add egress coverage to tc_redirect_neigh: the existing test only attaches
> > redirect_neigh at ingress, where mac_len is populated.
>
> This test cannot hit the BUG.
It was reachable _before_ the fix, normally useful to include such tests.
I will drop it, but if you think it is still useful and you catch me
before i send v3 i can include it (or can be sent afterwards).
> Just drop it and respin as [PATCH bpf v3].
>
Will do a v3 sometimes today for below...
cheers,
jamal
> > static int __bpf_redirect_neigh(struct sk_buff *skb, struct net_device *dev,
> > struct bpf_nh_params *nh)
> > {
> > - struct ethhdr *ethh = eth_hdr(skb);
> > + struct ethhdr *ethh;
> >
> > if (unlikely(skb->mac_header >= skb->network_header))
> > goto out;
> > + if (unlikely(skb_mac_header_len(skb) < ETH_HLEN ||
> > + !pskb_may_pull(skb, ETH_HLEN)))
> > + goto out;
> > + ethh = eth_hdr(skb);
>
> ETH_HLEN is not enough.
> __bpf_redirect_neigh_v4() and __bpf_redirect_neigh_v6() read ip_hdr()
> and ipv6_hdr() right after the pull. Nothing makes these headers linear.
> Take the AF_PACKET frame from the commit log without pop_eth and
> bpf_skb_change_head():
>
> packet_snd() headlen == 14, IP header in frags
> skb_pull(skb, ETH_HLEN) headlen == 0
> skb_reset_network_header(skb) network header == skb->tail
> ip4h->daddr, ip4h->saddr read past skb->tail
>
> The route lookup runs on garbage.
>
> Eric's skb_network_offset() check also covers
> mac_header >= network_header. No need for two checks.
>
> Something like this:
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2290,10 +2290,14 @@ static int bpf_out_neigh_v6(struct net *net, struct sk_buff *skb,
> static int __bpf_redirect_neigh_v6(struct sk_buff *skb, struct net_device *dev,
> struct bpf_nh_params *nh)
> {
> - const struct ipv6hdr *ip6h = ipv6_hdr(skb);
> struct net *net = dev_net(dev);
> int err, ret = NET_XMIT_DROP;
> + const struct ipv6hdr *ip6h;
> +
> + if (unlikely(!pskb_may_pull(skb, sizeof(*ip6h))))
> + goto out_drop;
> + ip6h = ipv6_hdr(skb);
>
> if (!nh) {
> struct dst_entry *dst;
> struct flowi6 fl6 = {
> @@ -2399,10 +2403,14 @@ static int bpf_out_neigh_v4(struct net *net, struct sk_buff *skb,
> static int __bpf_redirect_neigh_v4(struct sk_buff *skb, struct net_device *dev,
> struct bpf_nh_params *nh)
> {
> - const struct iphdr *ip4h = ip_hdr(skb);
> struct net *net = dev_net(dev);
> int err, ret = NET_XMIT_DROP;
> + const struct iphdr *ip4h;
> +
> + if (unlikely(!pskb_may_pull(skb, sizeof(*ip4h))))
> + goto out_drop;
> + ip4h = ip_hdr(skb);
>
> if (!nh) {
> struct flowi4 fl4 = {
> .flowi4_flags = FLOWI_FLAG_ANYSRC,
> @@ -2451,15 +2459,14 @@ static int __bpf_redirect_neigh_v4(struct sk_buff *skb, struct net_device *dev,
> static int __bpf_redirect_neigh(struct sk_buff *skb, struct net_device *dev,
> struct bpf_nh_params *nh)
> {
> - struct ethhdr *ethh = eth_hdr(skb);
> -
> - if (unlikely(skb->mac_header >= skb->network_header))
> + if (unlikely(skb_network_offset(skb) < ETH_HLEN ||
> + !pskb_may_pull(skb, ETH_HLEN)))
> goto out;
> bpf_push_mac_rcsum(skb);
> - if (is_multicast_ether_addr(ethh->h_dest))
> + if (is_multicast_ether_addr(eth_hdr(skb)->h_dest))
> goto out;
>
> - skb_pull(skb, sizeof(*ethh));
> + skb_pull(skb, ETH_HLEN);
> skb_unset_mac_header(skb);
> skb_reset_network_header(skb);
>
>
> pw-bot: cr
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net v2] bpf: Fix skb_pull() BUG in __bpf_redirect_neigh()
2026-10-29 9:36 [PATCH net v2] bpf: Fix skb_pull() BUG in __bpf_redirect_neigh() Jamal Hadi Salim
2026-10-30 21:47 ` Alexei Starovoitov
@ 2026-11-02 9:52 ` netdev-bot+sashiko
2026-11-03 11:00 ` Jamal Hadi Salim
1 sibling, 1 reply; 5+ messages in thread
From: netdev-bot+sashiko @ 2026-11-02 9:52 UTC (permalink / raw)
To: jhs
Cc: netdev, edumazet, bpf, bot+bpf-ci, daniel, ast, dsahern, kafai,
martin.lau, john.fastabend, sdf, andrii, eddyz87, memxor, song,
yonghong.song, jolsa, emil, ihor.solodrai, davem, kuba, pabeni,
horms, jiri, victor, linux-kselftest, shuah, stable
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 0 (1) · Medium: 1 (1) · Low: 1
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] The new netns_load_egress_bpf() calls inside
test_tc_redirect_neigh() leave the test thread in the test runner's…
- [Low] No selftest runs the new rejection branch in
__bpf_redirect_neigh() (skb_mac_header_len(skb) < ETH_HLEN ||…
Pre-existing issues:
- [High] The patch closes the ETH_HLEN pull BUG only in
__bpf_redirect_neigh().
- [Medium] __bpf_redirect_neigh() now linearizes only ETH_HLEN bytes,
then does skb_pull(ETH_HLEN) and skb_reset_network_header(), and…
--
bpf: Fix skb_pull() BUG in __bpf_redirect_neigh()
This commit hardens __bpf_redirect_neigh() before it reads and pulls the
Ethernet header. The MAC header length (network_header - mac_header) must
now be at least ETH_HLEN, and pskb_may_pull() must make the header linear.
It also adds tc egress coverage to the tc_redirect_neigh selftest.
Link: https://lore.kernel.org/all/2a0f5ff1d8db81b07fc4b344b8e68e3505fcbf36f72a471d1678b2f1cecd881b@mail.kernel.org/
Link: https://lore.kernel.org/netdev/20261027082604.1485903-1-edumazet@kernel.org/
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 70dc621672f2e..4925f3e355d46 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2432,10 +2432,14 @@ static int __bpf_redirect_neigh_v4(struct sk_buff *skb, struct net_device *dev,
> static int __bpf_redirect_neigh(struct sk_buff *skb, struct net_device *dev,
> struct bpf_nh_params *nh)
> {
> - struct ethhdr *ethh = eth_hdr(skb);
> + struct ethhdr *ethh;
>
> if (unlikely(skb->mac_header >= skb->network_header))
> goto out;
> + if (unlikely(skb_mac_header_len(skb) < ETH_HLEN ||
> + !pskb_may_pull(skb, ETH_HLEN)))
> + goto out;
[Severity: High]
This is a pre-existing issue and was not introduced by this patch. Still,
the skb from the commit message's own reproducer looks like it can BUG
through the sibling redirect paths. Is that expected?
After vlan pop_eth and bpf_skb_change_head(skb, 1, 0) on the nonlinear
AF_PACKET skb, len is 8193, data_len is 8192, and one L2 byte is linear.
__bpf_redirect_common() only checks:
net/core/filter.c:__bpf_redirect_common() {
...
if (unlikely(skb->mac_header >= skb->network_header || skb->len == 0)) {
...
}
so the 1-byte MAC header gets through there. With BPF_F_INGRESS:
bpf_redirect(BPF_F_INGRESS)
__bpf_rx_skb()
dev_forward_skb_nomtu()
__dev_forward_skb2()
eth_type_trans()
eth_skb_pull_mac()
skb_pull_inline(skb, ETH_HLEN)
__skb_pull() -> BUG()
Without BPF_F_INGRESS the path is:
__bpf_tx_skb()
dev_queue_xmit(lo)
loopback_xmit()
eth_type_trans()
veth goes through __dev_forward_skb() and ends the same way.
bpf_clone_redirect() takes the same __bpf_redirect() path. On lo, a plain
TC_ACT_OK instead of bpf_redirect_neigh() also reaches
loopback_xmit()->eth_type_trans().
In each case the 14 <= 8193 length check passes. __skb_pull() then leaves
len at 8179, which is below data_len (8192), and hits BUG().
The commit message calls this a follow-up to Eric's short link-layer
header fixes. This tree has no pskb_may_pull() in eth_type_trans() or
eth_skb_pull_mac(), and bpf_skb_change_head() still accepts a header
shorter than ETH_HLEN.
Should one of those two places be hardened as well? Otherwise stable
backporters may assume this change fully closes the reproducer.
> + ethh = eth_hdr(skb);
> bpf_push_mac_rcsum(skb);
> if (is_multicast_ether_addr(ethh->h_dest))
> goto out;
[Severity: Medium]
This isn't a bug introduced by this patch, but only ETH_HLEN bytes are
made linear here. After skb_pull(skb, sizeof(*ethh)) and
skb_reset_network_header(), can the L3 header sit entirely in frags?
__bpf_redirect_neigh_v4() reads the header directly from skb->data:
net/core/filter.c:__bpf_redirect_neigh_v4() {
const struct iphdr *ip4h = ip_hdr(skb);
...
.flowi4_dscp = ip4h_dscp(ip4h),
.flowi4_oif = dev->ifindex,
.flowi4_proto = ip4h->protocol,
.daddr = ip4h->daddr,
.saddr = ip4h->saddr,
...
}
__bpf_redirect_neigh_v6() does the same with ipv6_hdr(skb) for flowinfo,
nexthdr, saddr and daddr. When the route has no gateway,
bpf_out_neigh_v4()->ip_neigh_for_gw() reads ip_hdr(skb)->daddr again.
Take an AF_PACKET SOCK_RAW frame where prepad + len >= PAGE_SIZE.
packet_snd() and packet_alloc_skb() make only hard_header_len (14) bytes
linear. The frame passes both new checks, and skb_pull(14) leaves
skb_headlen() == 0.
Wouldn't the 20 or 40 byte L3 reads then come from past skb->tail, from
the tailroom or skb_shared_info? Those bytes would become the flowi4 or
flowi6 keys and drive the neighbour lookup. No pop_eth or change_head is
needed, just bpf_redirect_neigh() on such a packet.
Would a pskb_may_pull() of sizeof(struct iphdr) or sizeof(struct ipv6hdr)
before calling the v4/v6 helpers be appropriate?
> diff --git a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
> index af8968b89ad7c..829f84fb64a30 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
> @@ -407,6 +407,26 @@ static int netns_load_bpf(const struct bpf_program *src_prog,
> return -1;
> }
>
> +static int netns_load_egress_bpf(const struct bpf_program *prog,
> + const char *ns, int ifindex)
> +{
> + LIBBPF_OPTS(bpf_tc_hook, qdisc);
> + struct nstoken *nstoken;
> + int err;
> +
> + nstoken = open_netns(ns);
> + if (!ASSERT_OK_PTR(nstoken, "setns egress"))
> + return -1;
> +
> + QDISC_CLSACT_CREATE(&qdisc, ifindex);
> + XGRESS_FILTER_ADD(&qdisc, BPF_TC_EGRESS, prog, 0);
> + close_netns(nstoken);
> + return 0;
[ ... ]
> @@ -1055,6 +1075,12 @@ static void test_tc_redirect_neigh(struct netns_setup_result *setup_result)
> skel->progs.tc_chk, setup_result))
> goto done;
>
> + if (netns_load_egress_bpf(skel->progs.tc_egress, NS_SRC,
> + setup_result->ifindex_src) ||
> + netns_load_egress_bpf(skel->progs.tc_egress, NS_DST,
> + setup_result->ifindex_dst))
> + goto done;
> +
> if (!ASSERT_OK(set_forwarding(false), "disable forwarding"))
> goto done;
[Severity: Medium]
Is the test thread still in NS_FWD when set_forwarding(false) runs here?
test_tc_redirect() runs the tests in a separate pthread. open_netns()
saves the original namespace with:
tools/testing/selftests/bpf/network_helpers.c:open_netns() {
...
token->orig_netns_fd = open("/proc/self/ns/net", O_RDONLY);
...
}
/proc/self resolves to the thread-group leader. So the saved fd points to
the runner's root netns, not NS_FWD, which the worker entered at the top
of test_tc_redirect_neigh().
The close_netns() calls in netns_load_egress_bpf() therefore setns() the
worker back into the root netns. set_forwarding(false) then writes the
ipv4 and ipv6 forwarding sysctls, and those resolve through
current->nsproxy->net_ns.
Would this turn off forwarding in the host namespace and leave NS_FWD
unchanged? Nothing restores the setting afterwards. The test would also no
longer check redirect_neigh with forwarding disabled in NS_FWD.
test_tc_redirect_peer_ing() already has the same pattern via
netns_attach_nk(). Any of these would avoid it:
- attach the egress programs before entering NS_FWD
- re-enter NS_FWD before calling set_forwarding()
- have open_netns() save /proc/thread-self/ns/net
> diff --git a/tools/testing/selftests/bpf/progs/test_tc_neigh.c b/tools/testing/selftests/bpf/progs/test_tc_neigh.c
> index de15155f2609f..e0b6c14a3935e 100644
> --- a/tools/testing/selftests/bpf/progs/test_tc_neigh.c
> +++ b/tools/testing/selftests/bpf/progs/test_tc_neigh.c
> @@ -133,4 +133,14 @@ int tc_src(struct __sk_buff *skb)
> return bpf_redirect_neigh(IFINDEX_DST, NULL, 0, 0);
> }
>
> +SEC("tc")
> +int tc_egress(struct __sk_buff *skb)
> +{
> + if (skb->mark)
> + return TC_ACT_OK;
> +
> + skb->mark = 1;
> + return bpf_redirect_neigh(skb->ifindex, NULL, 0, 0);
> +}
[Severity: Low]
This isn't a bug, but does any packet in this test reach the new rejection
branch in __bpf_redirect_neigh()?
This program sees ordinary locally generated TCP/ICMP packets at veth
egress. __dev_queue_xmit() has already reset mac_header there, so
skb_mac_header_len() is 14 and the header is linear. These packets pass
both the old and the new checks, so removing the new check would go
unnoticed.
tc_redirect_neigh is also registered only for veth:
RUN_TEST(tc_redirect_neigh, MODE_VETH);
so the netkit case mentioned in the changelog is not exercised.
Would a negative test be worth adding? For example,
bpf_skb_change_head(skb, 1, 0) on a nonlinear skb followed by
bpf_redirect_neigh(), expecting a drop.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/bpf_redirect_neigh_short_linear.v2.r0.20261028080316-1%40mojatatu.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net v2] bpf: Fix skb_pull() BUG in __bpf_redirect_neigh()
2026-11-02 9:52 ` netdev-bot+sashiko
@ 2026-11-03 11:00 ` Jamal Hadi Salim
0 siblings, 0 replies; 5+ messages in thread
From: Jamal Hadi Salim @ 2026-11-03 11:00 UTC (permalink / raw)
To: netdev-bot+sashiko
Cc: netdev, edumazet, bpf, bot+bpf-ci, daniel, ast, dsahern, kafai,
martin.lau, john.fastabend, sdf, andrii, eddyz87, memxor, song,
yonghong.song, jolsa, emil, ihor.solodrai, davem, kuba, pabeni,
horms, jiri, victor, linux-kselftest, shuah, stable
On Mon, Nov 2, 2026 at 4:52 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 (1) · Medium: 1 (1) · Low: 1
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Medium] The new netns_load_egress_bpf() calls inside
> test_tc_redirect_neigh() leave the test thread in the test runner's…
> - [Low] No selftest runs the new rejection branch in
> __bpf_redirect_neigh() (skb_mac_header_len(skb) < ETH_HLEN ||…
>
> Pre-existing issues:
> - [High] The patch closes the ETH_HLEN pull BUG only in
> __bpf_redirect_neigh().
> - [Medium] __bpf_redirect_neigh() now linearizes only ETH_HLEN bytes,
> then does skb_pull(ETH_HLEN) and skb_reset_network_header(), and…
>
Most of this is stale material that the BPF Sashiko review already
exposed and we fixed it in a patch that has already been merged in the
bpf tree.
One of the follow-ups hadn't been captured yet until you pointed it
out; hank you for your service
cheers,
jamal
> --
>
> bpf: Fix skb_pull() BUG in __bpf_redirect_neigh()
>
> This commit hardens __bpf_redirect_neigh() before it reads and pulls the
> Ethernet header. The MAC header length (network_header - mac_header) must
> now be at least ETH_HLEN, and pskb_may_pull() must make the header linear.
> It also adds tc egress coverage to the tc_redirect_neigh selftest.
>
> Link: https://lore.kernel.org/all/2a0f5ff1d8db81b07fc4b344b8e68e3505fcbf36f72a471d1678b2f1cecd881b@mail.kernel.org/
> Link: https://lore.kernel.org/netdev/20261027082604.1485903-1-edumazet@kernel.org/
>
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 70dc621672f2e..4925f3e355d46 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -2432,10 +2432,14 @@ static int __bpf_redirect_neigh_v4(struct sk_buff *skb, struct net_device *dev,
> > static int __bpf_redirect_neigh(struct sk_buff *skb, struct net_device *dev,
> > struct bpf_nh_params *nh)
> > {
> > - struct ethhdr *ethh = eth_hdr(skb);
> > + struct ethhdr *ethh;
> >
> > if (unlikely(skb->mac_header >= skb->network_header))
> > goto out;
> > + if (unlikely(skb_mac_header_len(skb) < ETH_HLEN ||
> > + !pskb_may_pull(skb, ETH_HLEN)))
> > + goto out;
>
> [Severity: High]
> This is a pre-existing issue and was not introduced by this patch. Still,
> the skb from the commit message's own reproducer looks like it can BUG
> through the sibling redirect paths. Is that expected?
>
> After vlan pop_eth and bpf_skb_change_head(skb, 1, 0) on the nonlinear
> AF_PACKET skb, len is 8193, data_len is 8192, and one L2 byte is linear.
> __bpf_redirect_common() only checks:
>
> net/core/filter.c:__bpf_redirect_common() {
> ...
> if (unlikely(skb->mac_header >= skb->network_header || skb->len == 0)) {
> ...
> }
>
> so the 1-byte MAC header gets through there. With BPF_F_INGRESS:
>
> bpf_redirect(BPF_F_INGRESS)
> __bpf_rx_skb()
> dev_forward_skb_nomtu()
> __dev_forward_skb2()
> eth_type_trans()
> eth_skb_pull_mac()
> skb_pull_inline(skb, ETH_HLEN)
> __skb_pull() -> BUG()
>
> Without BPF_F_INGRESS the path is:
>
> __bpf_tx_skb()
> dev_queue_xmit(lo)
> loopback_xmit()
> eth_type_trans()
>
> veth goes through __dev_forward_skb() and ends the same way.
> bpf_clone_redirect() takes the same __bpf_redirect() path. On lo, a plain
> TC_ACT_OK instead of bpf_redirect_neigh() also reaches
> loopback_xmit()->eth_type_trans().
>
> In each case the 14 <= 8193 length check passes. __skb_pull() then leaves
> len at 8179, which is below data_len (8192), and hits BUG().
>
> The commit message calls this a follow-up to Eric's short link-layer
> header fixes. This tree has no pskb_may_pull() in eth_type_trans() or
> eth_skb_pull_mac(), and bpf_skb_change_head() still accepts a header
> shorter than ETH_HLEN.
>
> Should one of those two places be hardened as well? Otherwise stable
> backporters may assume this change fully closes the reproducer.
>
> > + ethh = eth_hdr(skb);
> > bpf_push_mac_rcsum(skb);
> > if (is_multicast_ether_addr(ethh->h_dest))
> > goto out;
>
> [Severity: Medium]
> This isn't a bug introduced by this patch, but only ETH_HLEN bytes are
> made linear here. After skb_pull(skb, sizeof(*ethh)) and
> skb_reset_network_header(), can the L3 header sit entirely in frags?
>
> __bpf_redirect_neigh_v4() reads the header directly from skb->data:
>
> net/core/filter.c:__bpf_redirect_neigh_v4() {
> const struct iphdr *ip4h = ip_hdr(skb);
> ...
> .flowi4_dscp = ip4h_dscp(ip4h),
> .flowi4_oif = dev->ifindex,
> .flowi4_proto = ip4h->protocol,
> .daddr = ip4h->daddr,
> .saddr = ip4h->saddr,
> ...
> }
>
> __bpf_redirect_neigh_v6() does the same with ipv6_hdr(skb) for flowinfo,
> nexthdr, saddr and daddr. When the route has no gateway,
> bpf_out_neigh_v4()->ip_neigh_for_gw() reads ip_hdr(skb)->daddr again.
>
> Take an AF_PACKET SOCK_RAW frame where prepad + len >= PAGE_SIZE.
> packet_snd() and packet_alloc_skb() make only hard_header_len (14) bytes
> linear. The frame passes both new checks, and skb_pull(14) leaves
> skb_headlen() == 0.
>
> Wouldn't the 20 or 40 byte L3 reads then come from past skb->tail, from
> the tailroom or skb_shared_info? Those bytes would become the flowi4 or
> flowi6 keys and drive the neighbour lookup. No pop_eth or change_head is
> needed, just bpf_redirect_neigh() on such a packet.
>
> Would a pskb_may_pull() of sizeof(struct iphdr) or sizeof(struct ipv6hdr)
> before calling the v4/v6 helpers be appropriate?
>
> > diff --git a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
> > index af8968b89ad7c..829f84fb64a30 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c
> > @@ -407,6 +407,26 @@ static int netns_load_bpf(const struct bpf_program *src_prog,
> > return -1;
> > }
> >
> > +static int netns_load_egress_bpf(const struct bpf_program *prog,
> > + const char *ns, int ifindex)
> > +{
> > + LIBBPF_OPTS(bpf_tc_hook, qdisc);
> > + struct nstoken *nstoken;
> > + int err;
> > +
> > + nstoken = open_netns(ns);
> > + if (!ASSERT_OK_PTR(nstoken, "setns egress"))
> > + return -1;
> > +
> > + QDISC_CLSACT_CREATE(&qdisc, ifindex);
> > + XGRESS_FILTER_ADD(&qdisc, BPF_TC_EGRESS, prog, 0);
> > + close_netns(nstoken);
> > + return 0;
>
> [ ... ]
>
> > @@ -1055,6 +1075,12 @@ static void test_tc_redirect_neigh(struct netns_setup_result *setup_result)
> > skel->progs.tc_chk, setup_result))
> > goto done;
> >
> > + if (netns_load_egress_bpf(skel->progs.tc_egress, NS_SRC,
> > + setup_result->ifindex_src) ||
> > + netns_load_egress_bpf(skel->progs.tc_egress, NS_DST,
> > + setup_result->ifindex_dst))
> > + goto done;
> > +
> > if (!ASSERT_OK(set_forwarding(false), "disable forwarding"))
> > goto done;
>
> [Severity: Medium]
> Is the test thread still in NS_FWD when set_forwarding(false) runs here?
>
> test_tc_redirect() runs the tests in a separate pthread. open_netns()
> saves the original namespace with:
>
> tools/testing/selftests/bpf/network_helpers.c:open_netns() {
> ...
> token->orig_netns_fd = open("/proc/self/ns/net", O_RDONLY);
> ...
> }
>
> /proc/self resolves to the thread-group leader. So the saved fd points to
> the runner's root netns, not NS_FWD, which the worker entered at the top
> of test_tc_redirect_neigh().
>
> The close_netns() calls in netns_load_egress_bpf() therefore setns() the
> worker back into the root netns. set_forwarding(false) then writes the
> ipv4 and ipv6 forwarding sysctls, and those resolve through
> current->nsproxy->net_ns.
>
> Would this turn off forwarding in the host namespace and leave NS_FWD
> unchanged? Nothing restores the setting afterwards. The test would also no
> longer check redirect_neigh with forwarding disabled in NS_FWD.
>
> test_tc_redirect_peer_ing() already has the same pattern via
> netns_attach_nk(). Any of these would avoid it:
>
> - attach the egress programs before entering NS_FWD
> - re-enter NS_FWD before calling set_forwarding()
> - have open_netns() save /proc/thread-self/ns/net
>
> > diff --git a/tools/testing/selftests/bpf/progs/test_tc_neigh.c b/tools/testing/selftests/bpf/progs/test_tc_neigh.c
> > index de15155f2609f..e0b6c14a3935e 100644
> > --- a/tools/testing/selftests/bpf/progs/test_tc_neigh.c
> > +++ b/tools/testing/selftests/bpf/progs/test_tc_neigh.c
> > @@ -133,4 +133,14 @@ int tc_src(struct __sk_buff *skb)
> > return bpf_redirect_neigh(IFINDEX_DST, NULL, 0, 0);
> > }
> >
> > +SEC("tc")
> > +int tc_egress(struct __sk_buff *skb)
> > +{
> > + if (skb->mark)
> > + return TC_ACT_OK;
> > +
> > + skb->mark = 1;
> > + return bpf_redirect_neigh(skb->ifindex, NULL, 0, 0);
> > +}
>
> [Severity: Low]
> This isn't a bug, but does any packet in this test reach the new rejection
> branch in __bpf_redirect_neigh()?
>
> This program sees ordinary locally generated TCP/ICMP packets at veth
> egress. __dev_queue_xmit() has already reset mac_header there, so
> skb_mac_header_len() is 14 and the header is linear. These packets pass
> both the old and the new checks, so removing the new check would go
> unnoticed.
>
> tc_redirect_neigh is also registered only for veth:
>
> RUN_TEST(tc_redirect_neigh, MODE_VETH);
>
> so the netkit case mentioned in the changelog is not exercised.
>
> Would a negative test be worth adding? For example,
> bpf_skb_change_head(skb, 1, 0) on a nonlinear skb followed by
> bpf_redirect_neigh(), expecting a drop.
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/bpf_redirect_neigh_short_linear.v2.r0.20261028080316-1%40mojatatu.com
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-11-03 11:01 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-29 9:36 [PATCH net v2] bpf: Fix skb_pull() BUG in __bpf_redirect_neigh() Jamal Hadi Salim
2026-10-30 21:47 ` Alexei Starovoitov
2026-10-31 13:32 ` Jamal Hadi Salim
2026-11-02 9:52 ` netdev-bot+sashiko
2026-11-03 11:00 ` Jamal Hadi Salim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox