[PATCH bpf-next v1 0/2] ktls, sockmap: Fix missing uncharge operation and add selfttest

[PATCH bpf-next v1 0/2] ktls, sockmap: Fix missing uncharge operation and add selfttest

[Jiayuan Chen]

Cong reported a warning when running ./test_sockmp:
https://lore.kernel.org/bpf/aAmIi0vlycHtbXeb@pop-os.localdomain/T/#t

------------[ cut here ]------------
WARNING: CPU: 1 PID: 40 at net/ipv4/af_inet.c inet_sock_destruct+0x173/0x1d5
Tainted: [W]=WARN
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014
Workqueue: events sk_psock_destroy
RIP: 0010:inet_sock_destruct+0x173/0x1d5
RSP: 0018:ffff8880085cfc18 EFLAGS: 00010202
RAX: 1ffff11003dbfc00 RBX: ffff88801edfe3e8 RCX: ffffffff822f5af4
RDX: 0000000000000007 RSI: dffffc0000000000 RDI: ffff88801edfe16c
RBP: ffff88801edfe184 R08: ffffed1003dbfc31 R09: 0000000000000000
R10: ffffffff822f5ab7 R11: ffff88801edfe187 R12: ffff88801edfdec0
R13: ffff888020376ac0 R14: ffff888020376ac0 R15: ffff888020376a60
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000556365155830 CR3: 000000001d6aa000 CR4: 0000000000350ef0
Call Trace:
 <TASK>
 __sk_destruct+0x46/0x222
 sk_psock_destroy+0x22f/0x242
 process_one_work+0x504/0x8a8
 ? process_one_work+0x39d/0x8a8
 ? __pfx_process_one_work+0x10/0x10
 ? worker_thread+0x44/0x2ae
 ? __list_add_valid_or_report+0x83/0xea
 ? srso_return_thunk+0x5/0x5f
 ? __list_add+0x45/0x52
 process_scheduled_works+0x73/0x82
 worker_thread+0x1ce/0x2ae


When we specify apply_bytes, we divide the msg into multiple segments,
each with a length of 'send', and every time we send this part of the data
using tcp_bpf_sendmsg_redir(), we use sk_msg_return_zero() to uncharge the
memory of the specified 'send' size.

However, if the first segment of data fails to send, for example, the
peer's buffer is full, we need to release all of the msg. When releasing
the msg, we haven't uncharged the memory of the subsequent segments.

This modification does not make significant logical changes, but only
fills in the missing uncharge places.

This issue has existed all along, until it was exposed after we added the
apply test in test_sockmap:

commit 3448ad23b34e ("selftests/bpf: Add apply_bytes test to test_txmsg_redir_wait_sndmem in test_sockmap")


Jiayuan Chen (2):
  ktls, sockmap: Fix missing uncharge operation
  selftests/bpf: Add test to cover sockmap with ktls

 net/tls/tls_sw.c                              |  7 ++
 .../selftests/bpf/prog_tests/sockmap_ktls.c   | 76 +++++++++++++++++++
 .../selftests/bpf/progs/test_sockmap_ktls.c   | 10 +++
 3 files changed, 93 insertions(+)

-- 
2.47.1

[PATCH bpf-next v1 1/2] ktls, sockmap: Fix missing uncharge operation

[Jiayuan Chen]

When we specify apply_bytes, we divide the msg into multiple segments,
each with a length of 'send', and every time we send this part of the data
using tcp_bpf_sendmsg_redir(), we use sk_msg_return_zero() to uncharge the
memory of the specified 'send' size.

However, if the first segment of data fails to send, for example, the
peer's buffer is full, we need to release all of the msg. When releasing
the msg, we haven't uncharged the memory of the subsequent segments.

This modification does not make significant logical changes, but only
fills in the missing uncharge places.

This issue has existed all along, until it was exposed after we added the
apply test in test_sockmap:
commit 3448ad23b34e ("selftests/bpf: Add apply_bytes test to test_txmsg_redir_wait_sndmem in test_sockmap")

Reported-by: Cong Wang <xiyou.wangcong@gmail.com>
Closes: https://lore.kernel.org/bpf/aAmIi0vlycHtbXeb@pop-os.localdomain/T/#t
Fixes: d3b18ad31f93 ("tls: add bpf support to sk_msg handling")
Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 net/tls/tls_sw.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index f3d7d19482da..fc88e34b7f33 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -908,6 +908,13 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg, struct sock *sk,
 					    &msg_redir, send, flags);
 		lock_sock(sk);
 		if (err < 0) {
+			/* Regardless of whether the data represented by
+			 * msg_redir is sent successfully, we have already
+			 * uncharged it via sk_msg_return_zero(). The
+			 * msg->sg.size represents the remaining unprocessed
+			 * data, which needs to be uncharged here.
+			 */
+			sk_mem_uncharge(sk, msg->sg.size);
 			*copied -= sk_msg_free_nocharge(sk, &msg_redir);
 			msg->sg.size = 0;
 		}
-- 
2.47.1

[PATCH bpf-next v1 2/2] selftests/bpf: Add test to cover sockmap with ktls

[Jiayuan Chen]

The selftest can reproduce an issue where we miss the uncharge operation
when freeing msg, which will cause the following warning. We fixed the
issue and added this reproducer to selftest to ensure it will not happen
again.

------------[ cut here ]------------
WARNING: CPU: 1 PID: 40 at net/ipv4/af_inet.c inet_sock_destruct+0x173/0x1d5
Tainted: [W]=WARN
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014
Workqueue: events sk_psock_destroy
RIP: 0010:inet_sock_destruct+0x173/0x1d5
RSP: 0018:ffff8880085cfc18 EFLAGS: 00010202
RAX: 1ffff11003dbfc00 RBX: ffff88801edfe3e8 RCX: ffffffff822f5af4
RDX: 0000000000000007 RSI: dffffc0000000000 RDI: ffff88801edfe16c
RBP: ffff88801edfe184 R08: ffffed1003dbfc31 R09: 0000000000000000
R10: ffffffff822f5ab7 R11: ffff88801edfe187 R12: ffff88801edfdec0
R13: ffff888020376ac0 R14: ffff888020376ac0 R15: ffff888020376a60
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000556365155830 CR3: 000000001d6aa000 CR4: 0000000000350ef0
Call Trace:
 <TASK>
 __sk_destruct+0x46/0x222
 sk_psock_destroy+0x22f/0x242
 process_one_work+0x504/0x8a8
 ? process_one_work+0x39d/0x8a8
 ? __pfx_process_one_work+0x10/0x10
 ? worker_thread+0x44/0x2ae
 ? __list_add_valid_or_report+0x83/0xea
 ? srso_return_thunk+0x5/0x5f
 ? __list_add+0x45/0x52
 process_scheduled_works+0x73/0x82
 worker_thread+0x1ce/0x2ae

Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 .../selftests/bpf/prog_tests/sockmap_ktls.c   | 76 +++++++++++++++++++
 .../selftests/bpf/progs/test_sockmap_ktls.c   | 10 +++
 2 files changed, 86 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c b/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c
index 49b85c1c7552..8f07168c9de6 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c
@@ -306,6 +306,80 @@ static void test_sockmap_ktls_tx_cork(int family, int sotype, bool push)
 	test_sockmap_ktls__destroy(skel);
 }
 
+static void test_sockmap_ktls_tx_no_buf(int family, int sotype, bool push)
+{
+	int c = 0, p = 0, one = 1, two = 2;
+	struct test_sockmap_ktls *skel;
+	unsigned char *data = NULL;
+	struct msghdr msg = {0};
+	struct iovec iov[2];
+	int prog_fd, map_fd;
+	int txrx_buf = 1024;
+	int iov_length = 8192;
+	int err;
+
+	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;
+
+	err = setsockopt(c, SOL_SOCKET, SO_RCVBUFFORCE, &txrx_buf, sizeof(int));
+	err |= setsockopt(p, SOL_SOCKET, SO_SNDBUFFORCE, &txrx_buf, sizeof(int));
+	if (!ASSERT_OK(err, "set buf limit"))
+		goto out;
+
+	prog_fd = bpf_program__fd(skel->progs.prog_sk_policy_redir);
+	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 = bpf_map_update_elem(map_fd, &two, &p, BPF_NOEXIST);
+	if (!ASSERT_OK(err, "bpf_map_update_elem(p)"))
+		goto out;
+
+	skel->bss->apply_bytes = 1024;
+
+	err = init_ktls_pairs(c, p);
+	if (!ASSERT_OK(err, "init_ktls_pairs(c, p)"))
+		goto out;
+
+	data = calloc(iov_length, sizeof(char));
+	if (!data)
+		goto out;
+
+	iov[0].iov_base = data;
+	iov[0].iov_len = iov_length;
+	iov[1].iov_base = data;
+	iov[1].iov_len = iov_length;
+	msg.msg_iov = iov;
+	msg.msg_iovlen = 2;
+
+	for (;;) {
+		err = sendmsg(c, &msg, MSG_DONTWAIT);
+		if (err <= 0)
+			break;
+	}
+
+out:
+	if (data)
+		free(data);
+	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;
@@ -330,6 +404,8 @@ static void run_ktls_test(int family, int sotype)
 		test_sockmap_ktls_tx_cork(family, sotype, false);
 	if (test__start_subtest("tls tx cork with push"))
 		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);
 }
 
 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 e0f757929ef4..8bdb9987c0c7 100644
--- a/tools/testing/selftests/bpf/progs/test_sockmap_ktls.c
+++ b/tools/testing/selftests/bpf/progs/test_sockmap_ktls.c
@@ -6,6 +6,7 @@
 int cork_byte;
 int push_start;
 int push_end;
+int apply_bytes;
 
 struct {
 	__uint(type, BPF_MAP_TYPE_SOCKMAP);
@@ -24,3 +25,12 @@ int prog_sk_policy(struct sk_msg_md *msg)
 
 	return SK_PASS;
 }
+
+SEC("sk_msg")
+int prog_sk_policy_redir(struct sk_msg_md *msg)
+{
+	int two = 2;
+
+	bpf_msg_apply_bytes(msg, apply_bytes);
+	return bpf_msg_redirect_map(msg, &sock_map, two, 0);
+}
-- 
2.47.1

Re: [PATCH bpf-next v1 1/2] ktls, sockmap: Fix missing uncharge operation

[Cong Wang]

On Fri, Apr 25, 2025 at 01:59:57PM +0800, Jiayuan Chen wrote:
>  net/tls/tls_sw.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index f3d7d19482da..fc88e34b7f33 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -908,6 +908,13 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg, struct sock *sk,
>  					    &msg_redir, send, flags);
>  		lock_sock(sk);
>  		if (err < 0) {
> +			/* Regardless of whether the data represented by
> +			 * msg_redir is sent successfully, we have already
> +			 * uncharged it via sk_msg_return_zero(). The
> +			 * msg->sg.size represents the remaining unprocessed
> +			 * data, which needs to be uncharged here.
> +			 */
> +			sk_mem_uncharge(sk, msg->sg.size);
>  			*copied -= sk_msg_free_nocharge(sk, &msg_redir);

Equivalent to sk_msg_free() ?

Thanks.

Re: [PATCH bpf-next v1 1/2] ktls, sockmap: Fix missing uncharge operation

[Jiayuan Chen]

2025/4/29 08:14, "Cong Wang" <xiyou.wangcong@gmail.com> wrote:

> 
> On Fri, Apr 25, 2025 at 01:59:57PM +0800, Jiayuan Chen wrote:
> 
> > 
> > net/tls/tls_sw.c | 7 +++++++
> > 
> >  1 file changed, 7 insertions(+)
> > 
> >  
> > 
> >  diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> > 
> >  index f3d7d19482da..fc88e34b7f33 100644
> > 
> >  --- a/net/tls/tls_sw.c
> > 
> >  +++ b/net/tls/tls_sw.c
> > 
> >  @@ -908,6 +908,13 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg, struct sock *sk,
> > 
> >  &msg_redir, send, flags);
> > 
> >  lock_sock(sk);
> > 
> >  if (err < 0) {
> > 
> >  + /* Regardless of whether the data represented by
> > 
> >  + * msg_redir is sent successfully, we have already
> > 
> >  + * uncharged it via sk_msg_return_zero(). The
> > 
> >  + * msg->sg.size represents the remaining unprocessed
> > 
> >  + * data, which needs to be uncharged here.
> > 
> >  + */
> > 
> >  + sk_mem_uncharge(sk, msg->sg.size);
> > 
> >  *copied -= sk_msg_free_nocharge(sk, &msg_redir);
> > 
> 
> Equivalent to sk_msg_free() ?
> 
> Thanks.
>

Before calling tcp_bpf_sendmsg_redir(), we have already uncharged some
memory using sk_msg_return_zero(). If we perform sk_msg_free(msg_redir),
it will cause the duplicate uncharge of this part of data. If we perform
sk_msg_free(msg), since tcp_bpf_sendmsg_redir() may not have sent any data
and msg->sg.start no longer points to this part of data, it will lead to
memoryleak.

So, directly calling sk_msg_free is not a good idea.

Re: [PATCH bpf-next v1 0/2] ktls, sockmap: Fix missing uncharge operation and add selfttest

[John Fastabend]

On 2025-04-25 13:59:56, Jiayuan Chen wrote:
> Cong reported a warning when running ./test_sockmp:
> https://lore.kernel.org/bpf/aAmIi0vlycHtbXeb@pop-os.localdomain/T/#t
> 
> ------------[ cut here ]------------
> WARNING: CPU: 1 PID: 40 at net/ipv4/af_inet.c inet_sock_destruct+0x173/0x1d5
> Tainted: [W]=WARN
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014
> Workqueue: events sk_psock_destroy
> RIP: 0010:inet_sock_destruct+0x173/0x1d5
> RSP: 0018:ffff8880085cfc18 EFLAGS: 00010202
> RAX: 1ffff11003dbfc00 RBX: ffff88801edfe3e8 RCX: ffffffff822f5af4
> RDX: 0000000000000007 RSI: dffffc0000000000 RDI: ffff88801edfe16c
> RBP: ffff88801edfe184 R08: ffffed1003dbfc31 R09: 0000000000000000
> R10: ffffffff822f5ab7 R11: ffff88801edfe187 R12: ffff88801edfdec0
> R13: ffff888020376ac0 R14: ffff888020376ac0 R15: ffff888020376a60
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000556365155830 CR3: 000000001d6aa000 CR4: 0000000000350ef0
> Call Trace:
>  <TASK>
>  __sk_destruct+0x46/0x222
>  sk_psock_destroy+0x22f/0x242
>  process_one_work+0x504/0x8a8
>  ? process_one_work+0x39d/0x8a8
>  ? __pfx_process_one_work+0x10/0x10
>  ? worker_thread+0x44/0x2ae
>  ? __list_add_valid_or_report+0x83/0xea
>  ? srso_return_thunk+0x5/0x5f
>  ? __list_add+0x45/0x52
>  process_scheduled_works+0x73/0x82
>  worker_thread+0x1ce/0x2ae
> 
> 
> When we specify apply_bytes, we divide the msg into multiple segments,
> each with a length of 'send', and every time we send this part of the data
> using tcp_bpf_sendmsg_redir(), we use sk_msg_return_zero() to uncharge the
> memory of the specified 'send' size.
> 
> However, if the first segment of data fails to send, for example, the
> peer's buffer is full, we need to release all of the msg. When releasing
> the msg, we haven't uncharged the memory of the subsequent segments.
> 
> This modification does not make significant logical changes, but only
> fills in the missing uncharge places.
> 
> This issue has existed all along, until it was exposed after we added the
> apply test in test_sockmap:
> 
> commit 3448ad23b34e ("selftests/bpf: Add apply_bytes test to test_txmsg_redir_wait_sndmem in test_sockmap")
> 
> 
> Jiayuan Chen (2):
>   ktls, sockmap: Fix missing uncharge operation
>   selftests/bpf: Add test to cover sockmap with ktls

For the series thanks Jiayuan for poking into the kTLS side. I doubt
anyone has used kTLS+apply_bytes yet. Thanks.

For me.

Acked-by: John Fastabend <john.fastabend@gmail.com>

Re: [PATCH bpf-next v1 1/2] ktls, sockmap: Fix missing uncharge operation

[Cong Wang]

On Fri, Apr 25, 2025 at 01:59:57PM +0800, Jiayuan Chen wrote:
> When we specify apply_bytes, we divide the msg into multiple segments,
> each with a length of 'send', and every time we send this part of the data
> using tcp_bpf_sendmsg_redir(), we use sk_msg_return_zero() to uncharge the
> memory of the specified 'send' size.
> 
> However, if the first segment of data fails to send, for example, the
> peer's buffer is full, we need to release all of the msg. When releasing
> the msg, we haven't uncharged the memory of the subsequent segments.
> 
> This modification does not make significant logical changes, but only
> fills in the missing uncharge places.
> 
> This issue has existed all along, until it was exposed after we added the
> apply test in test_sockmap:
> commit 3448ad23b34e ("selftests/bpf: Add apply_bytes test to test_txmsg_redir_wait_sndmem in test_sockmap")
> 
> Reported-by: Cong Wang <xiyou.wangcong@gmail.com>

Reviewed-by: Cong Wang <xiyou.wangcong@gmail.com>

Thanks!