From 76be5fae32febb1fdb848ba09f78c4b2c76cb337 Mon Sep 17 00:00:00 2001 From: Jiayuan Chen Date: Mon, 9 Jun 2025 10:59:08 +0800 Subject: [PATCH 1/6] bpf, sockmap: Fix psock incorrectly pointing to sk We observed an issue from the latest selftest: sockmap_redir where sk_psock(psock->sk) != psock in the backlog. The root cause is the special behavior in sockmap_redir - it frequently performs map_update() and map_delete() on the same socket. During map_update(), we create a new psock and during map_delete(), we eventually free the psock via rcu_work in sk_psock_drop(). However, pending workqueues might still exist and not be processed yet. If users immediately perform another map_update(), a new psock will be allocated for the same sk, resulting in two psocks pointing to the same sk. When the pending workqueue is later triggered, it uses the old psock to access sk for I/O operations, which is incorrect. Timing Diagram: cpu0 cpu1 map_update(sk): sk->psock = psock1 psock1->sk = sk map_delete(sk): rcu_work_free(psock1) map_update(sk): sk->psock = psock2 psock2->sk = sk workqueue: wakeup with psock1, but the sk of psock1 doesn't belong to psock1 rcu_handler: clean psock1 free(psock1) Previously, we used reference counting to address the concurrency issue between backlog and sock_map_close(). This logic remains necessary as it prevents the sk from being freed while processing the backlog. But this patch prevents pending backlogs from using a psock after it has been stopped. Note: We cannot call cancel_delayed_work_sync() in map_delete() since this might be invoked in BPF context by BPF helper, and the function may sleep. Fixes: 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface") Signed-off-by: Jiayuan Chen Signed-off-by: Daniel Borkmann Reviewed-by: John Fastabend Link: https://lore.kernel.org/bpf/20250609025908.79331-1-jiayuan.chen@linux.dev --- net/core/skmsg.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/net/core/skmsg.c b/net/core/skmsg.c index 34c51eb1a14f..83c78379932e 100644 --- a/net/core/skmsg.c +++ b/net/core/skmsg.c @@ -656,6 +656,13 @@ static void sk_psock_backlog(struct work_struct *work) bool ingress; int ret; + /* If sk is quickly removed from the map and then added back, the old + * psock should not be scheduled, because there are now two psocks + * pointing to the same sk. + */ + if (!sk_psock_test_state(psock, SK_PSOCK_TX_ENABLED)) + return; + /* Increment the psock refcnt to synchronize with close(fd) path in * sock_map_close(), ensuring we wait for backlog thread completion * before sk_socket freed. If refcnt increment fails, it indicates From 178f6a5c8cb3b6be1602de0964cd440243f493c9 Mon Sep 17 00:00:00 2001 From: Jiayuan Chen Date: Mon, 9 Jun 2025 10:08:52 +0800 Subject: [PATCH 2/6] bpf, ktls: Fix data corruption when using bpf_msg_pop_data() in ktls When sending plaintext data, we initially calculated the corresponding ciphertext length. However, if we later reduced the plaintext data length via socket policy, we failed to recalculate the ciphertext length. This results in transmitting buffers containing uninitialized data during ciphertext transmission. This causes uninitialized bytes to be appended after a complete "Application Data" packet, leading to errors on the receiving end when parsing TLS record. Fixes: d3b18ad31f93 ("tls: add bpf support to sk_msg handling") Reported-by: Cong Wang Signed-off-by: Jiayuan Chen Signed-off-by: Daniel Borkmann Reviewed-by: John Fastabend Acked-by: Jakub Kicinski Link: https://lore.kernel.org/bpf/20250609020910.397930-2-jiayuan.chen@linux.dev --- net/tls/tls_sw.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index fc88e34b7f33..549d1ea01a72 100644 --- a/net/tls/tls_sw.c +++ b/net/tls/tls_sw.c @@ -872,6 +872,19 @@ more_data: delta = msg->sg.size; psock->eval = sk_psock_msg_verdict(sk, psock, msg); delta -= msg->sg.size; + + if ((s32)delta > 0) { + /* It indicates that we executed bpf_msg_pop_data(), + * causing the plaintext data size to decrease. + * Therefore the encrypted data size also needs to + * correspondingly decrease. We only need to subtract + * delta to calculate the new ciphertext length since + * ktls does not support block encryption. + */ + struct sk_msg *enc = &ctx->open_rec->msg_encrypted; + + sk_msg_trim(sk, enc, enc->sg.size - delta); + } } if (msg->cork_bytes && msg->cork_bytes > msg->sg.size && !enospc && !full_record) { From f1c025773f257b534f6fa77dca29b0967dfed459 Mon Sep 17 00:00:00 2001 From: Jiayuan Chen Date: Mon, 9 Jun 2025 10:08:53 +0800 Subject: [PATCH 3/6] selftests/bpf: Add test to cover ktls with bpf_msg_pop_data The selftest can reproduce an issue where using bpf_msg_pop_data() in ktls causes errors on the receiving end. Signed-off-by: Jiayuan Chen Signed-off-by: Daniel Borkmann Reviewed-by: John Fastabend Link: https://lore.kernel.org/bpf/20250609020910.397930-3-jiayuan.chen@linux.dev --- .../selftests/bpf/prog_tests/sockmap_ktls.c | 91 +++++++++++++++++++ .../selftests/bpf/progs/test_sockmap_ktls.c | 4 + 2 files changed, 95 insertions(+) diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c b/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c index b6c471da5c28..b87e7f39e15a 100644 --- a/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c @@ -314,6 +314,95 @@ out: test_sockmap_ktls__destroy(skel); } +static void test_sockmap_ktls_tx_pop(int family, int sotype) +{ + char msg[37] = "0123456789abcdefghijklmnopqrstuvwxyz\0"; + int c = 0, p = 0, one = 1, sent, recvd; + struct test_sockmap_ktls *skel; + int prog_fd, map_fd; + char rcv[50] = {0}; + int err; + int i, m, r; + + skel = test_sockmap_ktls__open_and_load(); + if (!ASSERT_TRUE(skel, "open ktls skel")) + return; + + err = create_pair(family, sotype, &c, &p); + if (!ASSERT_OK(err, "create_pair()")) + goto out; + + prog_fd = bpf_program__fd(skel->progs.prog_sk_policy); + map_fd = bpf_map__fd(skel->maps.sock_map); + + err = bpf_prog_attach(prog_fd, map_fd, BPF_SK_MSG_VERDICT, 0); + if (!ASSERT_OK(err, "bpf_prog_attach sk msg")) + goto out; + + err = bpf_map_update_elem(map_fd, &one, &c, BPF_NOEXIST); + if (!ASSERT_OK(err, "bpf_map_update_elem(c)")) + goto out; + + err = init_ktls_pairs(c, p); + if (!ASSERT_OK(err, "init_ktls_pairs(c, p)")) + goto out; + + struct { + int pop_start; + int pop_len; + } pop_policy[] = { + /* trim the start */ + {0, 2}, + {0, 10}, + {1, 2}, + {1, 10}, + /* trim the end */ + {35, 2}, + /* New entries should be added before this line */ + {-1, -1}, + }; + + i = 0; + while (pop_policy[i].pop_start >= 0) { + skel->bss->pop_start = pop_policy[i].pop_start; + skel->bss->pop_end = pop_policy[i].pop_len; + + sent = send(c, msg, sizeof(msg), 0); + if (!ASSERT_EQ(sent, sizeof(msg), "send(msg)")) + goto out; + + recvd = recv_timeout(p, rcv, sizeof(rcv), MSG_DONTWAIT, 1); + if (!ASSERT_EQ(recvd, sizeof(msg) - pop_policy[i].pop_len, "pop len mismatch")) + goto out; + + /* verify the data + * msg: 0123456789a bcdefghij klmnopqrstuvwxyz + * | | + * popped data + */ + for (m = 0, r = 0; m < sizeof(msg);) { + /* skip checking the data that has been popped */ + if (m >= pop_policy[i].pop_start && + m <= pop_policy[i].pop_start + pop_policy[i].pop_len - 1) { + m++; + continue; + } + + if (!ASSERT_EQ(msg[m], rcv[r], "data mismatch")) + goto out; + m++; + r++; + } + i++; + } +out: + if (c) + close(c); + if (p) + close(p); + test_sockmap_ktls__destroy(skel); +} + static void run_tests(int family, enum bpf_map_type map_type) { int map; @@ -338,6 +427,8 @@ static void run_ktls_test(int family, int sotype) test_sockmap_ktls_tx_cork(family, sotype, true); if (test__start_subtest("tls tx egress with no buf")) test_sockmap_ktls_tx_no_buf(family, sotype, true); + if (test__start_subtest("tls tx with pop")) + test_sockmap_ktls_tx_pop(family, sotype); } void test_sockmap_ktls(void) diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_ktls.c b/tools/testing/selftests/bpf/progs/test_sockmap_ktls.c index 8bdb9987c0c7..83df4919c224 100644 --- a/tools/testing/selftests/bpf/progs/test_sockmap_ktls.c +++ b/tools/testing/selftests/bpf/progs/test_sockmap_ktls.c @@ -7,6 +7,8 @@ int cork_byte; int push_start; int push_end; int apply_bytes; +int pop_start; +int pop_end; struct { __uint(type, BPF_MAP_TYPE_SOCKMAP); @@ -22,6 +24,8 @@ int prog_sk_policy(struct sk_msg_md *msg) bpf_msg_cork_bytes(msg, cork_byte); if (push_start > 0 && push_end > 0) bpf_msg_push_data(msg, push_start, push_end, 0); + if (pop_start >= 0 && pop_end > 0) + bpf_msg_pop_data(msg, pop_start, pop_end, 0); return SK_PASS; } From 7f12c33850482521c961c5c15a50ebe9b9a88d1e Mon Sep 17 00:00:00 2001 From: Charalampos Mitrodimas Date: Wed, 11 Jun 2025 17:20:43 +0000 Subject: [PATCH 4/6] net, bpf: Fix RCU usage in task_cls_state() for BPF programs The commit ee971630f20f ("bpf: Allow some trace helpers for all prog types") made bpf_get_cgroup_classid_curr helper available to all BPF program types, not just networking programs. This helper calls __task_get_classid() which internally calls task_cls_state() requiring rcu_read_lock_bh_held(). This works in networking/tc context where RCU BH is held, but triggers an RCU warning when called from other contexts like BPF syscall programs that run under rcu_read_lock_trace(): WARNING: suspicious RCU usage 6.15.0-rc4-syzkaller-g079e5c56a5c4 #0 Not tainted ----------------------------- net/core/netclassid_cgroup.c:24 suspicious rcu_dereference_check() usage! Fix this by also accepting rcu_read_lock_held() and rcu_read_lock_trace_held() as valid RCU contexts in the task_cls_state() function. This ensures the helper works correctly in all needed RCU contexts where it might be called, regular RCU, RCU BH (for networking), and RCU trace (for BPF syscall programs). Fixes: ee971630f20f ("bpf: Allow some trace helpers for all prog types") Reported-by: syzbot+b4169a1cfb945d2ed0ec@syzkaller.appspotmail.com Signed-off-by: Charalampos Mitrodimas Signed-off-by: Daniel Borkmann Acked-by: Daniel Borkmann Link: https://lore.kernel.org/bpf/20250611-rcu-fix-task_cls_state-v3-1-3d30e1de753f@posteo.net Closes: https://syzkaller.appspot.com/bug?extid=b4169a1cfb945d2ed0ec --- net/core/netclassid_cgroup.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/net/core/netclassid_cgroup.c b/net/core/netclassid_cgroup.c index d22f0919821e..dff66d8fb325 100644 --- a/net/core/netclassid_cgroup.c +++ b/net/core/netclassid_cgroup.c @@ -21,7 +21,9 @@ static inline struct cgroup_cls_state *css_cls_state(struct cgroup_subsys_state struct cgroup_cls_state *task_cls_state(struct task_struct *p) { return css_cls_state(task_css_check(p, net_cls_cgrp_id, - rcu_read_lock_bh_held())); + rcu_read_lock_held() || + rcu_read_lock_bh_held() || + rcu_read_lock_trace_held())); } EXPORT_SYMBOL_GPL(task_cls_state); From a9a5f41b04dd137a353d4d1d6fc7d6e80aaad193 Mon Sep 17 00:00:00 2001 From: Steven Rostedt Date: Wed, 11 Jun 2025 15:56:15 -0400 Subject: [PATCH 5/6] xdp: Remove unused events xdp_redirect_map and xdp_redirect_map_err MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each TRACE_EVENT() defined can take up around 5K of text and meta data regardless if they are used or not. New code is being developed that will warn when a tracepoint is defined but not used. The trace events xdp_redirect_map and xdp_redirect_map_err are defined but not used, but there's also a comment that states these are kept around for backward compatibility. Which is interesting because since they are not used, any old BPF program that expects them to exist will get incorrect data (no data) when they use them. It's worse than not working, it's silently failing. Remove them as they will soon cause warnings, or if they really need to stick around, then code needs to be added to use them. Signed-off-by: Steven Rostedt (Google) Reviewed-by: Toke Høiland-Jørgensen Acked-by: Jesper Dangaard Brouer Link: https://lore.kernel.org/r/20250611155615.0c2cf61c@batman.local.home Signed-off-by: Alexei Starovoitov --- include/trace/events/xdp.h | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h index d3ef86c97ae3..0fe0893c2567 100644 --- a/include/trace/events/xdp.h +++ b/include/trace/events/xdp.h @@ -168,25 +168,6 @@ DEFINE_EVENT(xdp_redirect_template, xdp_redirect_err, #define _trace_xdp_redirect_map_err(dev, xdp, to, map_type, map_id, index, err) \ trace_xdp_redirect_err(dev, xdp, to, err, map_type, map_id, index) -/* not used anymore, but kept around so as not to break old programs */ -DEFINE_EVENT(xdp_redirect_template, xdp_redirect_map, - TP_PROTO(const struct net_device *dev, - const struct bpf_prog *xdp, - const void *tgt, int err, - enum bpf_map_type map_type, - u32 map_id, u32 index), - TP_ARGS(dev, xdp, tgt, err, map_type, map_id, index) -); - -DEFINE_EVENT(xdp_redirect_template, xdp_redirect_map_err, - TP_PROTO(const struct net_device *dev, - const struct bpf_prog *xdp, - const void *tgt, int err, - enum bpf_map_type map_type, - u32 map_id, u32 index), - TP_ARGS(dev, xdp, tgt, err, map_type, map_id, index) -); - TRACE_EVENT(xdp_cpumap_kthread, TP_PROTO(int map_id, unsigned int processed, unsigned int drops, From 16f3c7ad887c1f8fd698ab568b5851cadb65b5a8 Mon Sep 17 00:00:00 2001 From: Steven Rostedt Date: Thu, 12 Jun 2025 18:20:23 -0400 Subject: [PATCH 6/6] xdp: tracing: Hide some xdp events under CONFIG_BPF_SYSCALL The events xdp_cpumap_kthread, xdp_cpumap_enqueue and xdp_devmap_xmit are only called when CONFIG_BPF_SYSCALL is defined. As each event can take up to 5K regardless if they are used or not, it's best not to define them when they are not used. Add #ifdef around these events when they are not used. Acked-by: Jesper Dangaard Brouer Signed-off-by: Steven Rostedt (Google) Link: https://lore.kernel.org/r/20250612182023.78397b76@batman.local.home Signed-off-by: Alexei Starovoitov --- include/trace/events/xdp.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h index 0fe0893c2567..18c0ac514fcb 100644 --- a/include/trace/events/xdp.h +++ b/include/trace/events/xdp.h @@ -168,6 +168,7 @@ DEFINE_EVENT(xdp_redirect_template, xdp_redirect_err, #define _trace_xdp_redirect_map_err(dev, xdp, to, map_type, map_id, index, err) \ trace_xdp_redirect_err(dev, xdp, to, err, map_type, map_id, index) +#ifdef CONFIG_BPF_SYSCALL TRACE_EVENT(xdp_cpumap_kthread, TP_PROTO(int map_id, unsigned int processed, unsigned int drops, @@ -281,6 +282,7 @@ TRACE_EVENT(xdp_devmap_xmit, __entry->sent, __entry->drops, __entry->err) ); +#endif /* CONFIG_BPF_SYSCALL */ /* Expect users already include , but not xdp_priv.h */ #include