| Message ID | 20260518122842.218522-7-mahe.tardy@gmail.com |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | bpf: add icmp_send kfunc | expand |
> diff --git a/tools/testing/selftests/bpf/prog_tests/icmp_send_kfunc.c b/tools/testing/selftests/bpf/prog_tests/icmp_send_kfunc.c > index d0ac0502f6df..a9e9806877cf 100644 > --- a/tools/testing/selftests/bpf/prog_tests/icmp_send_kfunc.c > +++ b/tools/testing/selftests/bpf/prog_tests/icmp_send_kfunc.c [ ... ] > diff --git a/tools/testing/selftests/bpf/progs/icmp_send.c b/tools/testing/selftests/bpf/progs/icmp_send.c > index 6e1ba539eeb0..7830334b747a 100644 > --- a/tools/testing/selftests/bpf/progs/icmp_send.c > +++ b/tools/testing/selftests/bpf/progs/icmp_send.c > @@ -13,6 +13,9 @@ int unreach_type = 0; > int unreach_code = 0; > int kfunc_ret = -1; > > +unsigned int rec_count = 0; > +int rec_kfunc_rets[] = { -1, -1 }; > + > SEC("cgroup_skb/egress") > int egress(struct __sk_buff *skb) > { > @@ -65,4 +68,32 @@ int egress(struct __sk_buff *skb) > return SK_DROP; > } > > +SEC("cgroup_skb/egress") > +int recursion(struct __sk_buff *skb) > +{ > + void *data = (void *)(long)skb->data; > + void *data_end = (void *)(long)skb->data_end; > + struct iphdr *iph; > + int ret; > + > + iph = data; > + if ((void *)(iph + 1) > data_end || iph->version != 4) > + return SK_PASS; > + > + /* This call will provoke a recursion: the ICMP packet generated by the > + * kfunc will re-trigger this program since we are in the root cgroup in > + * which the kernel ICMP socket belongs. However when re-entering the > + * kfunc, it should return EBUSY. > + */ > + ret = bpf_icmp_send(skb, unreach_type, unreach_code); > + rec_kfunc_rets[rec_count & 1] = ret; Will this program affect background traffic when running on the system? The recursion program is attached to the root cgroup and intercepts all IPv4 traffic. While v5 changed SK_DROP to SK_PASS to avoid blocking traffic, the program still calls bpf_icmp_send() on every IPv4 packet without filtering for test-specific traffic. This means background IPv4 traffic (SSH, DNS, other tests) will trigger ICMP Destination Unreachable messages, potentially causing network disruption and test races on rec_kfunc_rets and rec_count. The egress program demonstrates proper filtering by checking for TCP, SERVER_IP, and server_port: SEC("cgroup_skb/egress") int egress(struct __sk_buff *skb) { void *data = (void *)(long)skb->data; void *data_end = (void *)(long)skb->data_end; struct iphdr *iph; struct ipv6hdr *ip6h; struct tcphdr *tcph; iph = data; if ((void *)(iph + 1) > data_end || iph->version != 4) return SK_PASS; if (iph->protocol != IPPROTO_TCP) return SK_PASS; if (bpf_ntohs(iph->daddr) != SERVER_IP) return SK_PASS; tcph = (struct tcphdr *)((void *)iph + sizeof(*iph)); if ((void *)(tcph + 1) > data_end) return SK_PASS; if (tcph->dest != server_port) return SK_PASS; ... } Could the recursion program use similar filtering to explicitly check for the test's traffic? > + __sync_fetch_and_add(&rec_count, 1); > + > + /* Let the first ICMP error message pass */ > + if (iph->protocol == IPPROTO_ICMP) > + return SK_PASS; > + > + return SK_DROP; > +} > + > char LICENSE[] SEC("license") = "Dual BSD/GPL"; --- 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 CI run summary: https://github.com/kernel-patches/bpf/actions/runs/26034287312
On Mon, May 18, 2026 at 01:07:53PM +0000, bot+bpf-ci@kernel.org wrote: > > diff --git a/tools/testing/selftests/bpf/prog_tests/icmp_send_kfunc.c b/tools/testing/selftests/bpf/prog_tests/icmp_send_kfunc.c > > index d0ac0502f6df..a9e9806877cf 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/icmp_send_kfunc.c > > +++ b/tools/testing/selftests/bpf/prog_tests/icmp_send_kfunc.c > > [ ... ] > > > diff --git a/tools/testing/selftests/bpf/progs/icmp_send.c b/tools/testing/selftests/bpf/progs/icmp_send.c > > index 6e1ba539eeb0..7830334b747a 100644 > > --- a/tools/testing/selftests/bpf/progs/icmp_send.c > > +++ b/tools/testing/selftests/bpf/progs/icmp_send.c > > @@ -13,6 +13,9 @@ int unreach_type = 0; > > int unreach_code = 0; > > int kfunc_ret = -1; > > > > +unsigned int rec_count = 0; > > +int rec_kfunc_rets[] = { -1, -1 }; > > + > > SEC("cgroup_skb/egress") > > int egress(struct __sk_buff *skb) > > { > > @@ -65,4 +68,32 @@ int egress(struct __sk_buff *skb) > > return SK_DROP; > > } > > > > +SEC("cgroup_skb/egress") > > +int recursion(struct __sk_buff *skb) > > +{ > > + void *data = (void *)(long)skb->data; > > + void *data_end = (void *)(long)skb->data_end; > > + struct iphdr *iph; > > + int ret; > > + > > + iph = data; > > + if ((void *)(iph + 1) > data_end || iph->version != 4) > > + return SK_PASS; > > + > > + /* This call will provoke a recursion: the ICMP packet generated by the > > + * kfunc will re-trigger this program since we are in the root cgroup in > > + * which the kernel ICMP socket belongs. However when re-entering the > > + * kfunc, it should return EBUSY. > > + */ > > + ret = bpf_icmp_send(skb, unreach_type, unreach_code); > > + rec_kfunc_rets[rec_count & 1] = ret; > > Will this program affect background traffic when running on the system? > > The recursion program is attached to the root cgroup and intercepts all > IPv4 traffic. While v5 changed SK_DROP to SK_PASS to avoid blocking > traffic, the program still calls bpf_icmp_send() on every IPv4 packet > without filtering for test-specific traffic. > > This means background IPv4 traffic (SSH, DNS, other tests) will trigger > ICMP Destination Unreachable messages, potentially causing network > disruption and test races on rec_kfunc_rets and rec_count. > > The egress program demonstrates proper filtering by checking for TCP, > SERVER_IP, and server_port: > > SEC("cgroup_skb/egress") > int egress(struct __sk_buff *skb) > { > void *data = (void *)(long)skb->data; > void *data_end = (void *)(long)skb->data_end; > struct iphdr *iph; > struct ipv6hdr *ip6h; > struct tcphdr *tcph; > > iph = data; > if ((void *)(iph + 1) > data_end || iph->version != 4) > return SK_PASS; > > if (iph->protocol != IPPROTO_TCP) > return SK_PASS; > > if (bpf_ntohs(iph->daddr) != SERVER_IP) > return SK_PASS; > > tcph = (struct tcphdr *)((void *)iph + sizeof(*iph)); > if ((void *)(tcph + 1) > data_end) > return SK_PASS; > > if (tcph->dest != server_port) > return SK_PASS; > ... > } > > Could the recursion program use similar filtering to explicitly check for > the test's traffic? I could restrict a little bit more by ignoring all traffic that is non-TCP for the dest port or non-ICMP but not sure this is actually needed. E.g. writing something like this before bpf_icmp_send: if (iph->daddr != bpf_htonl(SERVER_IP)) return SK_PASS; if (iph->protocol == IPPROTO_TCP) { tcph = (void *)iph + iph->ihl * 4; if ((void *)(tcph + 1) > data_end || tcph->dest != bpf_htons(server_port)) return SK_PASS; } else if (iph->protocol != IPPROTO_ICMP) { return SK_PASS; } But not sure this is strictly needed. > > > + __sync_fetch_and_add(&rec_count, 1); > > + > > + /* Let the first ICMP error message pass */ > > + if (iph->protocol == IPPROTO_ICMP) > > + return SK_PASS; > > + > > + return SK_DROP; > > +} > > + > > char LICENSE[] SEC("license") = "Dual BSD/GPL"; > > > --- > 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 > > CI run summary: https://github.com/kernel-patches/bpf/actions/runs/26034287312
diff --git a/tools/testing/selftests/bpf/prog_tests/icmp_send_kfunc.c b/tools/testing/selftests/bpf/prog_tests/icmp_send_kfunc.c index d0ac0502f6df..a9e9806877cf 100644 --- a/tools/testing/selftests/bpf/prog_tests/icmp_send_kfunc.c +++ b/tools/testing/selftests/bpf/prog_tests/icmp_send_kfunc.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-2.0 #include <test_progs.h> #include <network_helpers.h> +#include <cgroup_helpers.h> #include <linux/errqueue.h> #include <poll.h> #include "icmp_send.skel.h" @@ -10,6 +11,7 @@ #define ICMP_DEST_UNREACH 3 #define ICMPV6_DEST_UNREACH 1 +#define ICMP_HOST_UNREACH 1 #define ICMP_FRAG_NEEDED 4 #define NR_ICMP_UNREACH 15 #define ICMPV6_REJECT_ROUTE 6 @@ -176,3 +178,41 @@ void test_icmp_send_unreach(void) icmp_send__destroy(skel); close(cgroup_fd); } + +void test_icmp_send_unreach_recursion(void) +{ + struct icmp_send *skel; + int cgroup_fd = -1; + + skel = icmp_send__open_and_load(); + if (!ASSERT_OK_PTR(skel, "skel_open")) + goto cleanup; + + if (setup_cgroup_environment()) { + fprintf(stderr, "Failed to setup cgroup environment\n"); + goto cleanup; + } + + cgroup_fd = get_root_cgroup(); + if (!ASSERT_GE(cgroup_fd, 0, "get_root_cgroup")) + goto cleanup; + + skel->links.recursion = + bpf_program__attach_cgroup(skel->progs.recursion, cgroup_fd); + if (!ASSERT_OK_PTR(skel->links.recursion, "prog_attach_cgroup")) + goto cleanup; + + trigger_prog_read_icmp_errqueue(skel, ICMP_HOST_UNREACH, AF_INET, "127.0.0.1"); + + /* Because there's recursion involved, the first call will return at + * index 1 since it will return the second, and the second call will + * return at index 0 since it will return the first. + */ + ASSERT_EQ(skel->data->rec_kfunc_rets[0], -EBUSY, "kfunc_rets[0]"); + ASSERT_EQ(skel->data->rec_kfunc_rets[1], 0, "kfunc_rets[1]"); + +cleanup: + cleanup_cgroup_environment(); + icmp_send__destroy(skel); + close(cgroup_fd); +} diff --git a/tools/testing/selftests/bpf/progs/icmp_send.c b/tools/testing/selftests/bpf/progs/icmp_send.c index 6e1ba539eeb0..7830334b747a 100644 --- a/tools/testing/selftests/bpf/progs/icmp_send.c +++ b/tools/testing/selftests/bpf/progs/icmp_send.c @@ -13,6 +13,9 @@ int unreach_type = 0; int unreach_code = 0; int kfunc_ret = -1; +unsigned int rec_count = 0; +int rec_kfunc_rets[] = { -1, -1 }; + SEC("cgroup_skb/egress") int egress(struct __sk_buff *skb) { @@ -65,4 +68,32 @@ int egress(struct __sk_buff *skb) return SK_DROP; } +SEC("cgroup_skb/egress") +int recursion(struct __sk_buff *skb) +{ + void *data = (void *)(long)skb->data; + void *data_end = (void *)(long)skb->data_end; + struct iphdr *iph; + int ret; + + iph = data; + if ((void *)(iph + 1) > data_end || iph->version != 4) + return SK_PASS; + + /* This call will provoke a recursion: the ICMP packet generated by the + * kfunc will re-trigger this program since we are in the root cgroup in + * which the kernel ICMP socket belongs. However when re-entering the + * kfunc, it should return EBUSY. + */ + ret = bpf_icmp_send(skb, unreach_type, unreach_code); + rec_kfunc_rets[rec_count & 1] = ret; + __sync_fetch_and_add(&rec_count, 1); + + /* Let the first ICMP error message pass */ + if (iph->protocol == IPPROTO_ICMP) + return SK_PASS; + + return SK_DROP; +} + char LICENSE[] SEC("license") = "Dual BSD/GPL";
This test is similar to test_icmp_send_unreach but checks that, in case of recursion, meaning that the BPF program calling the kfunc was re-triggered by the icmp_send done by the kfunc, the kfunc will stop early and return -EBUSY. The test attaches to the root cgroup to ensure the ICMP packet generated by the kfunc re-triggers the BPF program. Since it's attached only for this recursion test, it should not disrupt the whole network. Signed-off-by: Mahe Tardy <mahe.tardy@gmail.com> --- .../bpf/prog_tests/icmp_send_kfunc.c | 40 +++++++++++++++++++ tools/testing/selftests/bpf/progs/icmp_send.c | 31 ++++++++++++++ 2 files changed, 71 insertions(+) -- 2.34.1