[PATCH bpf v2 00/15] bpf, sockmap: Fixes for verdicts, redirects, corks and recvmsg()

[PATCH bpf v2 00/15] bpf, sockmap: Fixes for verdicts, redirects, corks and recvmsg()

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

These patches fix bugs in sockmap:

  patches  area
  -------  ----------------------------------------------------------------
  01       recvmsg() after a fault
  02-04    the verdict of an sk_msg prog and the return value of sendmsg()
  05       the backlog of a stopped psock
  06       TCP listeners
  07-08    the target of an egress redirect
  09       the TCP state in recvmsg()
  10-14    corks and the redirect target
  15       bpf_msg_push_data() and bpf_msg_pop_data()

8 of the patches are from Chris Mason and Josef Bacik.

Patch 08 changes net/core/stream.c. Patch 09 moves tcp_can_send_ack() from
net/mptcp/protocol.c to include/net/tcp.h. Patches 10 and 15 change the
sk_msg helpers in net/core/filter.c.

v1 -> v2:
- Patch 03: end the verdict after a failed push too if apply_bytes is 0.
  Another sender can use up apply_bytes during the push (CI bot)
- Patch 04: correct the Fixes tag (CI bot)
- Rebase on the bpf tree

v1: https://lore.kernel.org/bpf/20261101192057.2296489-1-alexei.starovoitov@gmail.com/

Signed-off-by: Alexei Starovoitov <ast@kernel.org>

Alexei Starovoitov (7):
  bpf, sockmap: Don't let an SK_PASS verdict cover the next message
  bpf, sockmap: Subtract all dropped bytes from the return value
  net: Leave sk_stream_wait_memory() once the socket is dead
  bpf, sockmap: Clear the redirect target before the sk_msg prog runs
  bpf, sockmap: Check that the redirected message is the cork
  bpf, sockmap: Keep the rest of the cork after a redirect
  bpf, sockmap: Clear the end mark of elements added by push and pop

Chris Mason (5):
  bpf, sockmap: Give no progs to a TCP listener
  bpf, sockmap: Check the target of an egress redirect before each send
  bpf, sockmap: Check the TCP state in recvmsg()
  bpf, sockmap: Reset the copy position when corking
  bpf, sockmap: Don't copy 0 bytes into a full cork

Josef Bacik (3):
  bpf, sockmap: Write sg.start when recvmsg() faults
  bpf, sockmap: Don't let an SK_DROP verdict stick
  bpf, sockmap: Clear the resume state of a stopped psock

 Documentation/bpf/map_sockmap.rst |  4 +-
 include/linux/skmsg.h             | 19 +++++++
 include/net/tcp.h                 |  6 ++
 net/core/filter.c                 | 17 +-----
 net/core/skmsg.c                  | 13 ++++-
 net/core/sock_map.c               |  9 +++
 net/core/stream.c                 |  6 +-
 net/ipv4/tcp_bpf.c                | 93 ++++++++++++++++++++++++++++---
 net/mptcp/protocol.c              |  6 --
 9 files changed, 141 insertions(+), 32 deletions(-)


base-commit: 5e96760b25fdedbbf9312827f1f237d6b18b03e8
-- 
2.55.0

[PATCH bpf v2 01/15] bpf, sockmap: Write sg.start when recvmsg() faults

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

__sk_msg_recvmsg() copies an sk_msg to the user buffer and starts at
sg.start. After a full copy of an element it calls put_page(), unless an
skb owns the page. It handles 0 copied bytes as a fault. It writes the new
sg.start after the loop, but not when copy_page_to_iter() faults.

  element    0: length 0, put_page()   1: fault here   2: not copied
  sg.start   ^ today                   ^ this patch

Each subsequent call copies 0 bytes from element 0 and returns -EFAULT. The
user cannot read the rest of the data. sk_msg_free() starts at sg.start
too. It calls put_page() for element 0 again and can free a page in use.

Write sg.start in the fault path too. The commit in the Fixes tag removed
this write. Do not write it for MSG_PEEK, which changes no element.

Fixes: c9c89dcd872e ("bpf, sockmap: Fix partial copy_page_to_iter so progress can still be made")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/skmsg.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index df385a5a961e..f03519468ad1 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -443,6 +443,9 @@ int __sk_msg_recvmsg(struct sock *sk, struct sk_psock *psock, struct msghdr *msg
 			if (copy)
 				copy = copy_page_to_iter(page, sge->offset, copy, iter);
 			if (!copy) {
+				/* The elements in front of i are used up */
+				if (!peek)
+					msg_rx->sg.start = i;
 				copied = copied ? copied : -EFAULT;
 				goto out;
 			}
-- 
2.55.0

Re: [PATCH bpf v2 01/15] bpf, sockmap: Write sg.start when recvmsg() faults

[Jiayuan Chen]

On 11/2/26 3:20 PM, Alexei Starovoitov wrote:
> From: Josef Bacik <josef@toxicpanda.com>
>
> __sk_msg_recvmsg() copies an sk_msg to the user buffer and starts at
> sg.start. After a full copy of an element it calls put_page(), unless an
> skb owns the page. It handles 0 copied bytes as a fault. It writes the new
> sg.start after the loop, but not when copy_page_to_iter() faults.
>
>   element    0: length 0, put_page()   1: fault here   2: not copied
>   sg.start   ^ today                   ^ this patch
>
> Each subsequent call copies 0 bytes from element 0 and returns -EFAULT. The
> user cannot read the rest of the data. sk_msg_free() starts at sg.start
> too. It calls put_page() for element 0 again and can free a page in use.
>
> Write sg.start in the fault path too. The commit in the Fixes tag removed
> this write. Do not write it for MSG_PEEK, which changes no element.
>
> Fixes: c9c89dcd872e ("bpf, sockmap: Fix partial copy_page_to_iter so progress can still be made")
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>


Reviewed-by: Jiayuan Chen <jiayuan.chen@linux.dev>

[PATCH bpf v2 02/15] bpf, sockmap: Don't let an SK_DROP verdict stick

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

The verdict of an sk_msg prog is valid for one message, or for the bytes
that bpf_msg_apply_bytes() requests. These bytes can come from many
sendmsg() calls. Thus tcp_bpf_send_verdict() keeps the verdict in
psock->eval and runs the prog only when psock->eval is __SK_NONE. The code
that resets psock->eval is after the switch statement, but the __SK_DROP
branch returns before that code. Then sendmsg() drops all subsequent
messages with -EACCES and does not run the prog.

Reset psock->eval when apply_bytes is 0, as the __SK_REDIRECT branch does.

Fixes: 4f738adba30a ("bpf: create tcp_bpf_ulp allowing BPF to monitor socket TX/RX data")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/ipv4/tcp_bpf.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index 8cf63a9e7fed..d90bcc911b36 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -509,6 +509,9 @@ static int tcp_bpf_send_verdict(struct sock *sk, struct sk_psock *psock,
 	default:
 		sk_msg_free(sk, msg);
 		sk_msg_apply_bytes(psock, tosend);
+		/* The verdict holds for apply_bytes, not for ever */
+		if (!psock->apply_bytes)
+			psock->eval = __SK_NONE;
 		*copied -= (tosend + delta);
 		return -EACCES;
 	}
-- 
2.55.0

Re: [PATCH bpf v2 02/15] bpf, sockmap: Don't let an SK_DROP verdict stick

[Jiayuan Chen]

On 11/2/26 3:20 PM, Alexei Starovoitov wrote:
> From: Josef Bacik <josef@toxicpanda.com>
>
> The verdict of an sk_msg prog is valid for one message, or for the bytes
> that bpf_msg_apply_bytes() requests. These bytes can come from many
> sendmsg() calls. Thus tcp_bpf_send_verdict() keeps the verdict in
> psock->eval and runs the prog only when psock->eval is __SK_NONE. The code
> that resets psock->eval is after the switch statement, but the __SK_DROP
> branch returns before that code. Then sendmsg() drops all subsequent
> messages with -EACCES and does not run the prog.
>
> Reset psock->eval when apply_bytes is 0, as the __SK_REDIRECT branch does.
>
> Fixes: 4f738adba30a ("bpf: create tcp_bpf_ulp allowing BPF to monitor socket TX/RX data")
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>


Reviewed-by: Jiayuan Chen <jiayuan.chen@linux.dev>


> ---
>  net/ipv4/tcp_bpf.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index 8cf63a9e7fed..d90bcc911b36 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -509,6 +509,9 @@ static int tcp_bpf_send_verdict(struct sock *sk, struct sk_psock *psock,
>  	default:
>  		sk_msg_free(sk, msg);
>  		sk_msg_apply_bytes(psock, tosend);
> +		/* The verdict holds for apply_bytes, not for ever */
> +		if (!psock->apply_bytes)
> +			psock->eval = __SK_NONE;
>  		*copied -= (tosend + delta);
>  		return -EACCES;
>  	}

Re: [PATCH bpf v2 02/15] bpf, sockmap: Don't let an SK_DROP verdict stick

[John Fastabend]

On Mon, 02 Nov 2026 00:20, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>From: Josef Bacik <josef@toxicpanda.com>
>
>The verdict of an sk_msg prog is valid for one message, or for the bytes
>that bpf_msg_apply_bytes() requests. These bytes can come from many
>sendmsg() calls. Thus tcp_bpf_send_verdict() keeps the verdict in
>psock->eval and runs the prog only when psock->eval is __SK_NONE. The code
>that resets psock->eval is after the switch statement, but the __SK_DROP
>branch returns before that code. Then sendmsg() drops all subsequent
>messages with -EACCES and does not run the prog.
>
>Reset psock->eval when apply_bytes is 0, as the __SK_REDIRECT branch does.

So no one has tried to do apply_bytes + DROP I guess. If you want after
this series is merged I can add some test.

Thanks.

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

>
>Fixes: 4f738adba30a ("bpf: create tcp_bpf_ulp allowing BPF to monitor socket TX/RX data")
>Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>Signed-off-by: Alexei Starovoitov <ast@kernel.org>
>---
> net/ipv4/tcp_bpf.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
>index 8cf63a9e7fed..d90bcc911b36 100644
>--- a/net/ipv4/tcp_bpf.c
>+++ b/net/ipv4/tcp_bpf.c
>@@ -509,6 +509,9 @@ static int tcp_bpf_send_verdict(struct sock *sk, struct sk_psock *psock,
> 	default:
> 		sk_msg_free(sk, msg);
> 		sk_msg_apply_bytes(psock, tosend);
>+		/* The verdict holds for apply_bytes, not for ever */
>+		if (!psock->apply_bytes)
>+			psock->eval = __SK_NONE;
> 		*copied -= (tosend + delta);
> 		return -EACCES;
> 	}
>-- 
>2.55.0

[PATCH bpf v2 03/15] bpf, sockmap: Don't let an SK_PASS verdict cover the next message

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

tcp_bpf_send_verdict() keeps the verdict of the sk_msg prog in psock->eval
and runs the prog only when eval is __SK_NONE. For __SK_PASS it resets eval
only after tcp_bpf_push() returns 0. But the push can release the socket
lock in sk_stream_wait_memory(), and it can fail. Then another
tcp_bpf_sendmsg() sends its message and does not run the prog.

With bpf_msg_apply_bytes() the verdict covers apply_bytes bytes. After a
failed push, apply_bytes still includes the sent bytes. Thus the verdict
covers more bytes than the prog requested.

Reset eval before the push when apply_bytes is 0. If this push fails, the
prog now runs for the rest of the message. The same already occurs when a
message does not fit into one sk_msg.

When the push fails, subtract the sent bytes from apply_bytes. When
apply_bytes is not 0, eval stays __SK_PASS during the push, and the prog
does not run for another message, as today. The verdict must still cover
the bytes that a failed push does not send. But such a message also lowers
apply_bytes. Thus, if apply_bytes is 0 after the subtraction, end the
verdict, as after a push that returns 0.

Fixes: 4f738adba30a ("bpf: create tcp_bpf_ulp allowing BPF to monitor socket TX/RX data")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/ipv4/tcp_bpf.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index d90bcc911b36..b016aa419fb2 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -457,8 +457,22 @@ static int tcp_bpf_send_verdict(struct sock *sk, struct sk_psock *psock,
 
 	switch (psock->eval) {
 	case __SK_PASS:
+		/* The push may sleep without the socket lock, and it may fail */
+		if (!psock->apply_bytes)
+			psock->eval = __SK_NONE;
+		origsize = msg->sg.size;
 		ret = tcp_bpf_push(sk, msg, tosend, flags, true);
 		if (unlikely(ret)) {
+			/* A part of it may have gone out */
+			sk_msg_apply_bytes(psock, origsize - msg->sg.size);
+			/* Another sender can use up the rest during the push */
+			if (!psock->apply_bytes) {
+				psock->eval = __SK_NONE;
+				if (psock->sk_redir) {
+					sock_put(psock->sk_redir);
+					psock->sk_redir = NULL;
+				}
+			}
 			*copied -= sk_msg_free(sk, msg);
 			break;
 		}
-- 
2.55.0

Re: [PATCH bpf v2 03/15] bpf, sockmap: Don't let an SK_PASS verdict cover the next message

[John Fastabend]

On Mon, 02 Nov 2026 00:21, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>From: Alexei Starovoitov <ast@kernel.org>
>
>tcp_bpf_send_verdict() keeps the verdict of the sk_msg prog in psock->eval
>and runs the prog only when eval is __SK_NONE. For __SK_PASS it resets eval
>only after tcp_bpf_push() returns 0. But the push can release the socket
>lock in sk_stream_wait_memory(), and it can fail. Then another
>tcp_bpf_sendmsg() sends its message and does not run the prog.
>
>With bpf_msg_apply_bytes() the verdict covers apply_bytes bytes. After a
>failed push, apply_bytes still includes the sent bytes. Thus the verdict
>covers more bytes than the prog requested.
>
>Reset eval before the push when apply_bytes is 0. If this push fails, the
>prog now runs for the rest of the message. The same already occurs when a
>message does not fit into one sk_msg.
>
>When the push fails, subtract the sent bytes from apply_bytes. When
>apply_bytes is not 0, eval stays __SK_PASS during the push, and the prog
>does not run for another message, as today. The verdict must still cover
>the bytes that a failed push does not send. But such a message also lowers
>apply_bytes. Thus, if apply_bytes is 0 after the subtraction, end the
>verdict, as after a push that returns 0.

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

>
>Fixes: 4f738adba30a ("bpf: create tcp_bpf_ulp allowing BPF to monitor socket TX/RX data")
>Signed-off-by: Alexei Starovoitov <ast@kernel.org>
>---
> net/ipv4/tcp_bpf.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
>diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
>index d90bcc911b36..b016aa419fb2 100644
>--- a/net/ipv4/tcp_bpf.c
>+++ b/net/ipv4/tcp_bpf.c
>@@ -457,8 +457,22 @@ static int tcp_bpf_send_verdict(struct sock *sk, struct sk_psock *psock,
> 
> 	switch (psock->eval) {
> 	case __SK_PASS:
>+		/* The push may sleep without the socket lock, and it may fail */
>+		if (!psock->apply_bytes)
>+			psock->eval = __SK_NONE;
>+		origsize = msg->sg.size;
> 		ret = tcp_bpf_push(sk, msg, tosend, flags, true);
> 		if (unlikely(ret)) {
>+			/* A part of it may have gone out */
>+			sk_msg_apply_bytes(psock, origsize - msg->sg.size);
>+			/* Another sender can use up the rest during the push */

Interesting. We've never hit this with http parsers. We do use this to
skip payloads some times.

Thanks.

Re: [PATCH bpf v2 03/15] bpf, sockmap: Don't let an SK_PASS verdict cover the next message

[Alexei Starovoitov]

On Mon, Nov 02, 2026 at 10:51 AM John Fastabend <john.fastabend@gmail.com> wrote:
>>+			/* A part of it may have gone out */
>>+			sk_msg_apply_bytes(psock, origsize - msg->sg.size);
>>+			/* Another sender can use up the rest during the push */
>
> Interesting. We've never hit this with http parsers. We do use this to
> skip payloads some times.

That one needs two senders on one socket.
With one sender per socket you can't hit it.
A non-blocking send() fills sndbuf in the middle of the push.
N bytes go out and tcp_bpf_push() returns -EAGAIN.
Today apply_bytes still includes these N bytes.
Then the prog doesn't see the first N bytes after the payload.
There is no error and no splat, so it's easy to miss.

Re: [PATCH bpf v2 03/15] bpf, sockmap: Don't let an SK_PASS verdict cover the next message

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 1 · High: 1 · Medium: 1 · Low: 0

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).

- [Critical] Use-after-free of psock->cork.
- [High] The fix is incomplete when the verdict has a byte budget
  (psock->apply_bytes != 0).
- [Medium] The patch resets eval early, which lets one sender's bytes be
  charged against another sender's verdict and end that verdict early.

--

bpf, sockmap: Don't let an SK_PASS verdict cover the next message

This commit changes the __SK_PASS branch of tcp_bpf_send_verdict(). When
psock->apply_bytes is 0, it now resets psock->eval to __SK_NONE before
tcp_bpf_push(). When the push fails, it subtracts the bytes that went out
from apply_bytes and ends the verdict if apply_bytes reaches 0.

> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index d90bcc911b36b..b016aa419fb2d 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -457,8 +457,22 @@ static int tcp_bpf_send_verdict(struct sock *sk, struct sk_psock *psock,
>  
>  	switch (psock->eval) {
>  	case __SK_PASS:
> +		/* The push may sleep without the socket lock, and it may fail */
> +		if (!psock->apply_bytes)
> +			psock->eval = __SK_NONE;

[Severity: High]

When psock->apply_bytes is not 0, eval stays __SK_PASS during the push.
The budget is still debited only after tcp_bpf_push() returns. Can a
second sender on the same socket still get past the prog here?

This call chain releases the socket lock:

tcp_bpf_push()->tcp_sendmsg_locked()->sk_stream_wait_memory()->sk_wait_event()

While sender A sleeps there, sender B can enter tcp_bpf_send_verdict(). B
sees eval == __SK_PASS, so the prog doesn't run, and B computes its tosend
from the budget before A's bytes are taken out:

	tosend = msg->sg.size;
	if (psock->apply_bytes && psock->apply_bytes < tosend)
		tosend = psock->apply_bytes;

A's bytes are subtracted only later, by sk_msg_apply_bytes(psock, tosend)
on success or by the new sk_msg_apply_bytes(psock, origsize -
msg->sg.size) on failure.

If both pushes succeed, a verdict that bpf_msg_apply_bytes() limited to N
bytes covers up to 2N bytes. If A's message alone uses up the budget, B's
message is the next message, and it passes without running the prog.

The commit message says:

    When apply_bytes is not 0, eval stays __SK_PASS during the push, and
    the prog does not run for another message, as today. The verdict must
    still cover the bytes that a failed push does not send. But such a
    message also lowers apply_bytes.

But B's message is sized against the budget before A's in-flight bytes
are removed from it. Would it be better to reserve the budget before the
push? That means debiting tosend, and ending the verdict when the budget
reaches 0, before calling tcp_bpf_push(), instead of subtracting after a
failure.

The __SK_PASS branch also looks unchanged at the end of the series.

> +		origsize = msg->sg.size;
>  		ret = tcp_bpf_push(sk, msg, tosend, flags, true);

[Severity: Critical]

Can another sender free msg while this push sleeps?

msg can be psock->cork here. tcp_bpf_sendmsg() picks it up with:

		if (psock->cork) {
			msg_tx = psock->cork;

The __SK_REDIRECT branch detaches psock->cork before release_sock(). The
__SK_PASS branch leaves it attached during tcp_bpf_push(). tcp_bpf_push()
keeps sge and page pointing into the cork's sg ring across
tcp_sendmsg_locked(), and that call can sleep in sk_stream_wait_memory()
without the socket lock.

With this patch and apply_bytes == 0 (the default), eval is __SK_NONE
during that sleep. Sender B can take the lock, reuse the same psock->cork,
append to it, and run the prog on A's partly sent cork. If the prog
returns SK_REDIRECT, B's __SK_REDIRECT branch does:

		if (psock->cork) {
			cork = true;
			psock->cork = NULL;
		}
		...
		if (cork) {
			sk_msg_free(sk, msg);
			kfree(msg);

When A resumes, tcp_bpf_push() writes into the freed struct sk_msg:

		msg->sg.size -= ret;
		sge->offset += ret;
		sge->length -= ret;

It may also retry with, or call put_page() on, a page that was already
released. If the push failed, the new failure path reads msg->sg.size and
calls sk_msg_free(sk, msg) on the freed object.

If B's prog returns SK_DROP instead, sk_msg_free() in the __SK_DROP branch
drops page references that A is still using.

The shared-cork problem with two PASS senders already existed. The kfree()
variant used to need a third sender to end a byte-limited verdict. The
early reset makes it reachable with only two senders.

The later commit "bpf, sockmap: Check that the redirected message is the
cork" also says that "a cork is still not safe for two senders on one
socket". The __SK_PASS branch is unchanged at the end of the series.

Is it safe to reset eval here while psock->cork is still attached and
being pushed?

>  		if (unlikely(ret)) {
> +			/* A part of it may have gone out */
> +			sk_msg_apply_bytes(psock, origsize - msg->sg.size);
> +			/* Another sender can use up the rest during the push */
> +			if (!psock->apply_bytes) {
> +				psock->eval = __SK_NONE;
> +				if (psock->sk_redir) {
> +					sock_put(psock->sk_redir);
> +					psock->sk_redir = NULL;
> +				}
> +			}

[Severity: Medium]

With the early reset, can sender A's bytes be charged against a verdict
that another sender installed?

When apply_bytes is 0 on entry, A's tosend is its whole message. eval is
now __SK_NONE while A sleeps in tcp_sendmsg_locked(). Sender B can take
the socket lock and run the prog, which installs B's verdict:

net/core/skmsg.c:sk_psock_msg_verdict() {
    ...
	psock->apply_bytes = msg->apply_bytes;
	if (ret == __SK_REDIRECT) {
    ...
		psock->sk_redir = msg->sk_redir;
		sock_hold(psock->sk_redir);
    ...
}

If B's apply_bytes is larger than B's message, B's verdict (PASS, DROP or
REDIRECT) stays active with some budget left. When A resumes, it
subtracts its bytes from B's budget, even though A's own verdict had no
byte limit. On success that happens through the unchanged
sk_msg_apply_bytes(psock, tosend). On failure it happens through the new
call above.

If that brings apply_bytes to 0, either this new block or the common block
at the end of tcp_bpf_send_verdict() sets eval to __SK_NONE and drops
psock->sk_redir:

	if (likely(!ret)) {
		if (!psock->apply_bytes) {
			psock->eval =  __SK_NONE;
			if (psock->sk_redir) {
				sock_put(psock->sk_redir);
				psock->sk_redir = NULL;
			}
		}

That ends B's verdict before it has covered the bytes B's prog asked for.
Later bytes that should have been dropped or redirected get a fresh prog
run instead.

Before this patch, eval stayed __SK_PASS during A's push, so this needed a
third sender. Could A record locally whether its verdict had a byte limit
on entry? Then it would touch psock->apply_bytes, eval and sk_redir only
when that is true and the verdict is still its own.

>  			*copied -= sk_msg_free(sk, msg);
>  			break;
>  		}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102072112.4160195-1-alexei.starovoitov%40gmail.com

[PATCH bpf v2 04/15] bpf, sockmap: Subtract all dropped bytes from the return value

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

When an sk_msg prog returns SK_DROP, tcp_bpf_send_verdict() frees the full
message. It then subtracts tosend bytes from *copied, the return value of
sendmsg(). But tosend is the number of bytes that the verdict applies to.
After bpf_msg_apply_bytes() tosend can be less than the message size. Then
sendmsg() reports dropped bytes as sent.

Subtract the number of bytes that sk_msg_free() frees. After an error the
SK_PASS branch already does this. The SK_REDIRECT branch does the same if
the message is not the cork. Nothing changes when tosend is equal to the
message size. Otherwise sendmsg() now returns -EACCES if it sends nothing.

Fixes: ca70b8baf2bd ("tcp_bpf: Fix the sk_mem_uncharge logic in tcp_bpf_sendmsg")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/ipv4/tcp_bpf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index b016aa419fb2..36d1bf46353a 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -521,12 +521,12 @@ static int tcp_bpf_send_verdict(struct sock *sk, struct sk_psock *psock,
 		break;
 	case __SK_DROP:
 	default:
-		sk_msg_free(sk, msg);
+		/* All of it is thrown away, not only tosend bytes */
+		*copied -= sk_msg_free(sk, msg) + delta;
 		sk_msg_apply_bytes(psock, tosend);
 		/* The verdict holds for apply_bytes, not for ever */
 		if (!psock->apply_bytes)
 			psock->eval = __SK_NONE;
-		*copied -= (tosend + delta);
 		return -EACCES;
 	}
 
-- 
2.55.0

Re: [PATCH bpf v2 04/15] bpf, sockmap: Subtract all dropped bytes from the return value

[Jiayuan Chen]

On 11/2/26 3:21 PM, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@kernel.org>
>
> When an sk_msg prog returns SK_DROP, tcp_bpf_send_verdict() frees the full
> message. It then subtracts tosend bytes from *copied, the return value of
> sendmsg(). But tosend is the number of bytes that the verdict applies to.
> After bpf_msg_apply_bytes() tosend can be less than the message size. Then
> sendmsg() reports dropped bytes as sent.
>
> Subtract the number of bytes that sk_msg_free() frees. After an error the
> SK_PASS branch already does this. The SK_REDIRECT branch does the same if
> the message is not the cork. Nothing changes when tosend is equal to the
> message size. Otherwise sendmsg() now returns -EACCES if it sends nothing.
>
> Fixes: ca70b8baf2bd ("tcp_bpf: Fix the sk_mem_uncharge logic in tcp_bpf_sendmsg")
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>


Reviewed-by: Jiayuan Chen <jiayuan.chen@linux.dev>


> ---
>  net/ipv4/tcp_bpf.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index b016aa419fb2..36d1bf46353a 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -521,12 +521,12 @@ static int tcp_bpf_send_verdict(struct sock *sk, struct sk_psock *psock,
>  		break;
>  	case __SK_DROP:
>  	default:
> -		sk_msg_free(sk, msg);
> +		/* All of it is thrown away, not only tosend bytes */
> +		*copied -= sk_msg_free(sk, msg) + delta;
>  		sk_msg_apply_bytes(psock, tosend);
>  		/* The verdict holds for apply_bytes, not for ever */
>  		if (!psock->apply_bytes)
>  			psock->eval = __SK_NONE;
> -		*copied -= (tosend + delta);
>  		return -EACCES;
>  	}
>  

Re: [PATCH bpf v2 04/15] bpf, sockmap: Subtract all dropped bytes from the return value

[John Fastabend]

On Mon, 02 Nov 2026 00:21, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>From: Alexei Starovoitov <ast@kernel.org>
>
>When an sk_msg prog returns SK_DROP, tcp_bpf_send_verdict() frees the full
>message. It then subtracts tosend bytes from *copied, the return value of
>sendmsg(). But tosend is the number of bytes that the verdict applies to.
>After bpf_msg_apply_bytes() tosend can be less than the message size. Then
>sendmsg() reports dropped bytes as sent.
>
>Subtract the number of bytes that sk_msg_free() frees. After an error the
>SK_PASS branch already does this. The SK_REDIRECT branch does the same if
>the message is not the cork. Nothing changes when tosend is equal to the
>message size. Otherwise sendmsg() now returns -EACCES if it sends nothing.
>
>Fixes: ca70b8baf2bd ("tcp_bpf: Fix the sk_mem_uncharge logic in tcp_bpf_sendmsg")
>Signed-off-by: Alexei Starovoitov <ast@kernel.org>
>---

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

[PATCH bpf v2 05/15] bpf, sockmap: Clear the resume state of a stopped psock

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

sk_psock_backlog() passes the skbs of psock->ingress_skb to the socket of
the psock. When the socket has no room, the function saves the length and
offset of the remaining data in psock->work_state. A later run continues at
this offset and clears the state when the skb is complete.

sk_psock_skb_state() saves and clears the state, but only while
SK_PSOCK_TX_ENABLED is set. sk_psock_backlog() does not test this flag for
each skb. If the socket closes during the loop, sk_psock_stop() clears the
flag. The loop continues, but the state of the completed skb stays. The
loop uses this state for all subsequent skbs and sends only part of their
data. A closing socket may lose the end of a stream, but not the middle.

The state belongs to the skb at the head of ingress_skb. Always clear the
state when this skb is complete. A stopped psock still saves no new state.

Fixes: 3b4f14b79428 ("bpf, sockmap: fix duplicated data transmission")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/skmsg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index f03519468ad1..a5d498982848 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -664,7 +664,8 @@ static void sk_psock_skb_state(struct sk_psock *psock,
 			       int len, int off)
 {
 	spin_lock_bh(&psock->ingress_lock);
-	if (sk_psock_test_state(psock, SK_PSOCK_TX_ENABLED)) {
+	/* No new state once the psock is stopped, but never keep an old one */
+	if (!len || sk_psock_test_state(psock, SK_PSOCK_TX_ENABLED)) {
 		state->len = len;
 		state->off = off;
 	}
-- 
2.55.0

Re: [PATCH bpf v2 05/15] bpf, sockmap: Clear the resume state of a stopped psock

[Jiayuan Chen]

On 11/2/26 3:21 PM, Alexei Starovoitov wrote:
> From: Josef Bacik <josef@toxicpanda.com>
>
> sk_psock_backlog() passes the skbs of psock->ingress_skb to the socket of
> the psock. When the socket has no room, the function saves the length and
> offset of the remaining data in psock->work_state. A later run continues at
> this offset and clears the state when the skb is complete.
>
> sk_psock_skb_state() saves and clears the state, but only while
> SK_PSOCK_TX_ENABLED is set. sk_psock_backlog() does not test this flag for
> each skb. If the socket closes during the loop, sk_psock_stop() clears the
> flag. The loop continues, but the state of the completed skb stays. The
> loop uses this state for all subsequent skbs and sends only part of their
> data. A closing socket may lose the end of a stream, but not the middle.
>
> The state belongs to the skb at the head of ingress_skb. Always clear the
> state when this skb is complete. A stopped psock still saves no new state.
>
> Fixes: 3b4f14b79428 ("bpf, sockmap: fix duplicated data transmission")
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
>  net/core/skmsg.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index f03519468ad1..a5d498982848 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -664,7 +664,8 @@ static void sk_psock_skb_state(struct sk_psock *psock,
>  			       int len, int off)
>  {
>  	spin_lock_bh(&psock->ingress_lock);
> -	if (sk_psock_test_state(psock, SK_PSOCK_TX_ENABLED)) {
> +	/* No new state once the psock is stopped, but never keep an old one */
> +	if (!len || sk_psock_test_state(psock, SK_PSOCK_TX_ENABLED)) {
>  		state->len = len;
>  		state->off = off;
>  	}


Reviewed-by: Jiayuan Chen <jiayuan.chen@linux.dev>


The 'state' should be clear unconditionally.

Re: [PATCH bpf v2 05/15] bpf, sockmap: Clear the resume state of a stopped psock

[John Fastabend]

On Mon, 02 Nov 2026 04:40, Jiayuan Chen <jiayuan.chen@linux.dev> wrote:
>
>On 11/2/26 3:21 PM, Alexei Starovoitov wrote:
>> From: Josef Bacik <josef@toxicpanda.com>
>>
>> sk_psock_backlog() passes the skbs of psock->ingress_skb to the socket of
>> the psock. When the socket has no room, the function saves the length and
>> offset of the remaining data in psock->work_state. A later run continues at
>> this offset and clears the state when the skb is complete.
>>
>> sk_psock_skb_state() saves and clears the state, but only while
>> SK_PSOCK_TX_ENABLED is set. sk_psock_backlog() does not test this flag for
>> each skb. If the socket closes during the loop, sk_psock_stop() clears the
>> flag. The loop continues, but the state of the completed skb stays. The
>> loop uses this state for all subsequent skbs and sends only part of their
>> data. A closing socket may lose the end of a stream, but not the middle.
>>
>> The state belongs to the skb at the head of ingress_skb. Always clear the
>> state when this skb is complete. A stopped psock still saves no new state.
>>
>> Fixes: 3b4f14b79428 ("bpf, sockmap: fix duplicated data transmission")
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
>> ---
>>  net/core/skmsg.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
>> index f03519468ad1..a5d498982848 100644
>> --- a/net/core/skmsg.c
>> +++ b/net/core/skmsg.c
>> @@ -664,7 +664,8 @@ static void sk_psock_skb_state(struct sk_psock *psock,
>>  			       int len, int off)
>>  {
>>  	spin_lock_bh(&psock->ingress_lock);
>> -	if (sk_psock_test_state(psock, SK_PSOCK_TX_ENABLED)) {
>> +	/* No new state once the psock is stopped, but never keep an old one */
>> +	if (!len || sk_psock_test_state(psock, SK_PSOCK_TX_ENABLED)) {
>>  		state->len = len;
>>  		state->off = off;
>>  	}
>
>
>Reviewed-by: Jiayuan Chen <jiayuan.chen@linux.dev>

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

[PATCH bpf v2 06/15] bpf, sockmap: Give no progs to a TCP listener

[Alexei Starovoitov]

From: Chris Mason <mason@kernel.org>

A psock with a verdict prog replaces sk_data_ready and sk_write_space of
its socket. It saves the old callbacks, and the new callbacks call them.
sock_map_link() does this to a TCP listener too. Its new sk_data_ready
cannot read and does not call the old one. Thus it does not wake accept().

sk_clone() copies the new callbacks to each child, but not the psock.
tcp_bpf_clone() restores only sk_prot. Without a psock the child loses data
or does not wake its reader. If it gets a psock, the psock saves the copies
as the old callbacks. Then sk_psock_verdict_data_ready() calls itself until
the stack overflows. sk_psock_write_space() calls itself too, without end.

Give no progs to a TCP listener, as before the first commit below. A
listener receives and sends no data. It stays in the map and keeps its own
callbacks. Add this exception to map_sockmap.rst. A listener no longer gets
-EBUSY from a second map with progs of the same kind.

Since the second commit below, a socket keeps its psock when TCP unhashes
it. sock_map_link() does not see the state that the socket gets later. Do
not depend on TCP to keep such a socket out of TCP_LISTEN. Set both
callbacks in tcp_bpf_clone() to the TCP defaults.

Fixes: 0c48eefae712 ("sock_map: Lift socket state restriction for datagram sockets")
Fixes: b8b8315e39ff ("bpf, sockmap: Remove unhash handler for BPF sockmap usage")
Signed-off-by: Chris Mason <mason@kernel.org>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 Documentation/bpf/map_sockmap.rst |  4 +++-
 net/core/sock_map.c               |  9 +++++++++
 net/ipv4/tcp_bpf.c                | 15 +++++++++++++--
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/Documentation/bpf/map_sockmap.rst b/Documentation/bpf/map_sockmap.rst
index 2d630686a00b..eb466b86df17 100644
--- a/Documentation/bpf/map_sockmap.rst
+++ b/Documentation/bpf/map_sockmap.rst
@@ -33,7 +33,9 @@ of ``__SK_DROP``, ``__SK_PASS``, or ``__SK_REDIRECT``.
 
 When a socket is inserted into one of these maps, its socket callbacks are
 replaced and a ``struct sk_psock`` is attached to it. Additionally, this
-``sk_psock`` inherits the programs that are attached to the map.
+``sk_psock`` inherits the programs that are attached to the map. A TCP listener
+is an exception. It gets a ``struct sk_psock``, but it keeps its callbacks and
+inherits no programs.
 
 A sock object may be in multiple maps, but can only inherit a single
 parse or verdict program. If adding a sock object to a map would result
diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index efe49551caba..282c6a4b2b05 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -228,6 +228,14 @@ static int sock_map_link(struct bpf_map *map, struct sock *sk)
 	struct sk_psock *psock;
 	int ret;
 
+	/*
+	 * A listener receives no data. With the callbacks of a verdict program
+	 * or a parser it would no longer wake accept(), and its children would
+	 * copy them.
+	 */
+	if (sk_is_tcp(sk) && sk->sk_state == TCP_LISTEN)
+		goto no_progs;
+
 	stream_verdict = READ_ONCE(progs->stream_verdict);
 	if (stream_verdict) {
 		stream_verdict = bpf_prog_inc_not_zero(stream_verdict);
@@ -262,6 +270,7 @@ static int sock_map_link(struct bpf_map *map, struct sock *sk)
 		}
 	}
 
+no_progs:
 	psock = sock_map_psock_get_checked(sk);
 	if (IS_ERR(psock)) {
 		ret = PTR_ERR(psock);
diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index 36d1bf46353a..fd443350f9b8 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -785,7 +785,18 @@ void tcp_bpf_clone(const struct sock *sk, struct sock *newsk)
 {
 	struct proto *prot = newsk->sk_prot;
 
-	if (is_insidevar(prot, tcp_bpf_prots))
-		newsk->sk_prot = sk->sk_prot_creator;
+	if (!is_insidevar(prot, tcp_bpf_prots))
+		return;
+
+	newsk->sk_prot = sk->sk_prot_creator;
+	/*
+	 * sock_map_link() gives a listener no programs, but a socket which got
+	 * them in another state and listens now has the psock's sk_data_ready
+	 * and sk_write_space, and sock_copy() has copied them.
+	 * sk_psock_init() refuses a socket that has sk_user_data or a ULP, so
+	 * what the psock replaced were the defaults of a TCP socket.
+	 */
+	newsk->sk_data_ready = sock_def_readable;
+	newsk->sk_write_space = sk_stream_write_space;
 }
 #endif /* CONFIG_BPF_SYSCALL */
-- 
2.55.0

Re: [PATCH bpf v2 06/15] bpf, sockmap: Give no progs to a TCP listener

[Jiayuan Chen]

On 11/2/26 3:21 PM, Alexei Starovoitov wrote:
> From: Chris Mason <mason@kernel.org>
>
> A psock with a verdict prog replaces sk_data_ready and sk_write_space of
> its socket. It saves the old callbacks, and the new callbacks call them.
> sock_map_link() does this to a TCP listener too. Its new sk_data_ready
> cannot read and does not call the old one. Thus it does not wake accept().
>
> sk_clone() copies the new callbacks to each child, but not the psock.
> tcp_bpf_clone() restores only sk_prot. Without a psock the child loses data
> or does not wake its reader. If it gets a psock, the psock saves the copies
> as the old callbacks. Then sk_psock_verdict_data_ready() calls itself until
> the stack overflows. sk_psock_write_space() calls itself too, without end.
>
> Give no progs to a TCP listener, as before the first commit below. A
> listener receives and sends no data. It stays in the map and keeps its own
> callbacks. Add this exception to map_sockmap.rst. A listener no longer gets
> -EBUSY from a second map with progs of the same kind.
>
> Since the second commit below, a socket keeps its psock when TCP unhashes
> it. sock_map_link() does not see the state that the socket gets later. Do
> not depend on TCP to keep such a socket out of TCP_LISTEN. Set both
> callbacks in tcp_bpf_clone() to the TCP defaults.
>
> Fixes: 0c48eefae712 ("sock_map: Lift socket state restriction for datagram sockets")
> Fixes: b8b8315e39ff ("bpf, sockmap: Remove unhash handler for BPF sockmap usage")
> Signed-off-by: Chris Mason <mason@kernel.org>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
>  Documentation/bpf/map_sockmap.rst |  4 +++-
>  net/core/sock_map.c               |  9 +++++++++
>  net/ipv4/tcp_bpf.c                | 15 +++++++++++++--
>  3 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/bpf/map_sockmap.rst b/Documentation/bpf/map_sockmap.rst
> index 2d630686a00b..eb466b86df17 100644
> --- a/Documentation/bpf/map_sockmap.rst
> +++ b/Documentation/bpf/map_sockmap.rst
> @@ -33,7 +33,9 @@ of ``__SK_DROP``, ``__SK_PASS``, or ``__SK_REDIRECT``.
>  
>  When a socket is inserted into one of these maps, its socket callbacks are
>  replaced and a ``struct sk_psock`` is attached to it. Additionally, this
> -``sk_psock`` inherits the programs that are attached to the map.
> +``sk_psock`` inherits the programs that are attached to the map. A TCP listener
> +is an exception. It gets a ``struct sk_psock``, but it keeps its callbacks and
> +inherits no programs.
>  
>  A sock object may be in multiple maps, but can only inherit a single
>  parse or verdict program. If adding a sock object to a map would result
> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index efe49551caba..282c6a4b2b05 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -228,6 +228,14 @@ static int sock_map_link(struct bpf_map *map, struct sock *sk)
>  	struct sk_psock *psock;
>  	int ret;
>  
> +	/*
> +	 * A listener receives no data. With the callbacks of a verdict program
> +	 * or a parser it would no longer wake accept(), and its children would
> +	 * copy them.
> +	 */
> +	if (sk_is_tcp(sk) && sk->sk_state == TCP_LISTEN)
> +		goto no_progs;
> +
>  	stream_verdict = READ_ONCE(progs->stream_verdict);
>  	if (stream_verdict) {
>  		stream_verdict = bpf_prog_inc_not_zero(stream_verdict);
> @@ -262,6 +270,7 @@ static int sock_map_link(struct bpf_map *map, struct sock *sk)
>  		}
>  	}
>  
> +no_progs:
>  	psock = sock_map_psock_get_checked(sk);
>  	if (IS_ERR(psock)) {
>  		ret = PTR_ERR(psock);
> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index 36d1bf46353a..fd443350f9b8 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -785,7 +785,18 @@ void tcp_bpf_clone(const struct sock *sk, struct sock *newsk)
>  {
>  	struct proto *prot = newsk->sk_prot;
>  
> -	if (is_insidevar(prot, tcp_bpf_prots))
> -		newsk->sk_prot = sk->sk_prot_creator;
> +	if (!is_insidevar(prot, tcp_bpf_prots))
> +		return;
> +
> +	newsk->sk_prot = sk->sk_prot_creator;
> +	/*
> +	 * sock_map_link() gives a listener no programs, but a socket which got
> +	 * them in another state and listens now has the psock's sk_data_ready
> +	 * and sk_write_space, and sock_copy() has copied them.
> +	 * sk_psock_init() refuses a socket that has sk_user_data or a ULP, so
> +	 * what the psock replaced were the defaults of a TCP socket.
> +	 */
> +	newsk->sk_data_ready = sock_def_readable;
> +	newsk->sk_write_space = sk_stream_write_space;




Reviewed-by: Jiayuan Chen <jiayuan.chen@linux.dev>


I guess the scenario is: connect(AF_UNSPEC) unhashes a connected socket
that is still in the map, then listen() turns it into a listener. Since
we no longer have an unhash handler to drop the psock, the callbacks have
to be reset in tcp_bpf_clone().

Re: [PATCH bpf v2 06/15] bpf, sockmap: Give no progs to a TCP listener

[John Fastabend]

On Mon, 02 Nov 2026 00:21, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>From: Chris Mason <mason@kernel.org>
>
>A psock with a verdict prog replaces sk_data_ready and sk_write_space of
>its socket. It saves the old callbacks, and the new callbacks call them.
>sock_map_link() does this to a TCP listener too. Its new sk_data_ready
>cannot read and does not call the old one. Thus it does not wake accept().
>
>sk_clone() copies the new callbacks to each child, but not the psock.
>tcp_bpf_clone() restores only sk_prot. Without a psock the child loses data
>or does not wake its reader. If it gets a psock, the psock saves the copies
>as the old callbacks. Then sk_psock_verdict_data_ready() calls itself until
>the stack overflows. sk_psock_write_space() calls itself too, without end.
>
>Give no progs to a TCP listener, as before the first commit below. A
>listener receives and sends no data. It stays in the map and keeps its own
>callbacks. Add this exception to map_sockmap.rst. A listener no longer gets
>-EBUSY from a second map with progs of the same kind.
>
>Since the second commit below, a socket keeps its psock when TCP unhashes
>it. sock_map_link() does not see the state that the socket gets later. Do
>not depend on TCP to keep such a socket out of TCP_LISTEN. Set both
>callbacks in tcp_bpf_clone() to the TCP defaults.
>
>Fixes: 0c48eefae712 ("sock_map: Lift socket state restriction for datagram sockets")
>Fixes: b8b8315e39ff ("bpf, sockmap: Remove unhash handler for BPF sockmap usage")
>Signed-off-by: Chris Mason <mason@kernel.org>
>Signed-off-by: Alexei Starovoitov <ast@kernel.org>

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

Re: [PATCH bpf v2 06/15] bpf, sockmap: Give no progs to a TCP listener

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 0 · Low: 1 (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).

- [High] The patch does not fix the listener-side bug that its commit
  message names first: a TCP listener carrying the psock's sk_data_ready…
- [High] tcp_bpf_clone() decides whether to reset the child's callbacks
  only by checking whether the copied sk_prot is a tcp_bpf proto…
- [Low] The helper descriptions still promise that every inserted socket
  inherits the map's programs, which contradicts the new TCP-listener…

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but the
  sockmap_listen child-clone tests do not test what their comments…

--

bpf, sockmap: Give no progs to a TCP listener

This patch makes sock_map_link() skip program inheritance for TCP sockets
in TCP_LISTEN. Such a listener gets a psock but keeps its own
sk_data_ready and sk_write_space. It also makes tcp_bpf_clone() reset the
child's sk_data_ready and sk_write_space to the TCP defaults, and it
documents the listener exception in map_sockmap.rst.

> diff --git a/Documentation/bpf/map_sockmap.rst b/Documentation/bpf/map_sockmap.rst
> index 2d630686a00ba..eb466b86df170 100644
> --- a/Documentation/bpf/map_sockmap.rst
> +++ b/Documentation/bpf/map_sockmap.rst
> @@ -33,7 +33,9 @@ of ``__SK_DROP``, ``__SK_PASS``, or ``__SK_REDIRECT``.
>  
>  When a socket is inserted into one of these maps, its socket callbacks are
>  replaced and a ``struct sk_psock`` is attached to it. Additionally, this
> -``sk_psock`` inherits the programs that are attached to the map.
> +``sk_psock`` inherits the programs that are attached to the map. A TCP listener
> +is an exception. It gets a ``struct sk_psock``, but it keeps its callbacks and
> +inherits no programs.

[Severity: Low]

This isn't a bug, but a few other places still describe inheritance as
unconditional. bpf_sock_map_update() and bpf_sock_hash_update() in
include/uapi/linux/bpf.h still say:

 *		If the *map* has eBPF programs (parser and verdict), those will
 *		be inherited by the socket being added. If the socket is
 *		already attached to eBPF programs, this results in an error.

The bpf_sock_map_update() and bpf_sock_hash_update() helper sections
further down in map_sockmap.rst say the same thing.

After this patch, a TCP_LISTEN socket inherits no programs and no longer
gets -EBUSY. Should these descriptions, and the bpf-helpers man page that
is generated from the UAPI header, mention the listener exception too?

> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index efe49551caba9..282c6a4b2b05f 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -228,6 +228,14 @@ static int sock_map_link(struct bpf_map *map, struct sock *sk)
>  	struct sk_psock *psock;
>  	int ret;
>  
> +	/*
> +	 * A listener receives no data. With the callbacks of a verdict program
> +	 * or a parser it would no longer wake accept(), and its children would
> +	 * copy them.
> +	 */
> +	if (sk_is_tcp(sk) && sk->sk_state == TCP_LISTEN)
> +		goto no_progs;

[Severity: High]

This only checks sk_state at insert time. Can a socket that was inserted
while ESTABLISHED still become a listener that carries the psock's
sk_data_ready?

sock_map_sk_state_allowed() accepts an ESTABLISHED TCP socket. In that
case sock_map_link() gives it the map's programs, and
sk_psock_start_verdict() or sk_psock_start_strp() replaces sk_data_ready.
tcp_bpf_prots override neither disconnect nor unhash, so the psock
survives this sequence:

  connect(AF_UNSPEC)
    __inet_stream_connect()
      err = sk->sk_prot->disconnect(sk, flags);
      sock->state = err ? SS_DISCONNECTING : SS_UNCONNECTED;
  listen()
    __inet_listen_sk()    /* TCP_CLOSE is accepted */

When a handshake completes, parent->sk_data_ready() is
sk_psock_verdict_data_ready():

net/core/skmsg.c:sk_psock_verdict_data_ready() {
    ...
	copied = ops->read_skb(sk, sk_psock_verdict_recv);
	if (copied >= 0) {
		...
			sk_psock_data_ready(sk, psock);
		...
	}
}

On a listener, tcp_read_skb() always fails:

net/ipv4/tcp.c:tcp_read_skb() {
    ...
	if (sk->sk_state == TCP_LISTEN)
		return -ENOTCONN;
    ...
}

As a result the saved sock_def_readable never runs. Wouldn't a blocking
accept(), or poll/epoll on the listener, then never be woken? The
strparser variant looks the same, because tcp_read_sock_noack() also
returns -ENOTCONN on TCP_LISTEN.

The new comment in tcp_bpf_clone() describes exactly this socket, but
only newsk gets repaired. The commit message names "does not wake
accept()" as the bug and tags b8b8315e39ff for the listen-later case.

Would the listener's callbacks need to be restored or stripped when it
moves to TCP_LISTEN, or that transition be refused?

For this socket, the new map_sockmap.rst sentence "it keeps its callbacks
and inherits no programs" also doesn't hold.

[ ... ]

> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index 36d1bf46353a9..fd443350f9b81 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -785,7 +785,18 @@ void tcp_bpf_clone(const struct sock *sk, struct sock *newsk)
>  {
>  	struct proto *prot = newsk->sk_prot;
>  
> -	if (is_insidevar(prot, tcp_bpf_prots))
> -		newsk->sk_prot = sk->sk_prot_creator;
> +	if (!is_insidevar(prot, tcp_bpf_prots))
> +		return;

[Severity: High]

The callback reset only happens when the copied sk_prot is a tcp_bpf
proto. Can a child copy the restored sk_prot together with a stale psock
sk_data_ready?

sk_psock_drop() restores sk_prot before it restores sk_data_ready:

net/core/skmsg.c:sk_psock_drop() {
	write_lock_bh(&sk->sk_callback_lock);
	sk_psock_restore_proto(sk, psock);
	rcu_assign_sk_user_data(sk, NULL);
	if (psock->progs.stream_parser)
		sk_psock_stop_strp(sk, psock);
	else if (psock->progs.stream_verdict || psock->progs.skb_verdict)
		sk_psock_stop_verdict(sk, psock);
	write_unlock_bh(&sk->sk_callback_lock);
	...
}

sk_psock_restore_proto()->tcp_bpf_update_proto(restore=true) publishes
the original proto with sock_replace_proto(). Only later does
sk_psock_stop_verdict() write back saved_data_ready.

The clone side takes neither sk_callback_lock nor the listener's lock:

tcp_v4_rcv()    /* TCP_NEW_SYN_RECV */
  tcp_check_req()
    syn_recv_sock()
      tcp_create_openreq_child()
        sk_clone()
          sock_copy()    /* skc_prot and sk_data_ready copied locklessly */

A child cloned between those two steps would get sk_prot == tcp_prot and
sk_data_ready == sk_psock_verdict_data_ready (or
sk_psock_strp_data_ready). tcp_bpf_clone() then returns early here and
the stale callback stays. sk_user_data is not copied, so the child has
no psock.

The precondition is a listener that still has psock callbacks, such as
the listen-later socket described above. The window opens in
sk_psock_drop() when sock_map_del_link() did not stop the verdict first.
That happens if the map's programs were detached before the socket was
deleted or closed:

net/core/sock_map.c:sock_map_del_link() {
    ...
	if (!strp_stop && !verdict_stop)
		return;
    ...
}

After accept(), sk_psock_verdict_recv() on the child finds no psock. It
drops the received data and the reader is never woken. If the child is
later added to a sockmap, sk_psock_start_verdict() does:

	psock->saved_data_ready = sk->sk_data_ready;
	WRITE_ONCE(sk->sk_data_ready, sk_psock_verdict_data_ready);

That saves sk_psock_verdict_data_ready as its own saved_data_ready, so
sk_psock_data_ready() would recurse until the stack overflows. This is
the case the commit message says it closes.

Would it be more robust to check for the psock callbacks themselves
(sk_psock_verdict_data_ready, sk_psock_strp_data_ready,
sk_psock_write_space) instead of sk_prot? Or to order the teardown so a
lockless clone cannot see a mismatched pair?

> +
> +	newsk->sk_prot = sk->sk_prot_creator;
> +	/*
> +	 * sock_map_link() gives a listener no programs, but a socket which got
> +	 * them in another state and listens now has the psock's sk_data_ready
> +	 * and sk_write_space, and sock_copy() has copied them.
> +	 * sk_psock_init() refuses a socket that has sk_user_data or a ULP, so
> +	 * what the psock replaced were the defaults of a TCP socket.
> +	 */
> +	newsk->sk_data_ready = sock_def_readable;
> +	newsk->sk_write_space = sk_stream_write_space;

[Severity: Low]

This is a pre-existing issue, not one introduced by this patch, but does
any test exercise this reset?

test_accept_after_delete() and test_accept_before_delete() in
tools/testing/selftests/bpf/prog_tests/sockmap_listen.c say the child is
"cloned without parent psock state or callbacks". However, test_ops()
runs them with:

	t->fn(skel, family, sotype, map_fd);

No verdict or parser program is attached, so the listener's sk_data_ready
and sk_write_space are never replaced. Those tests would still pass if
the new resets in tcp_bpf_clone() were removed.

>  }
>  #endif /* CONFIG_BPF_SYSCALL */

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102072112.4160195-1-alexei.starovoitov%40gmail.com

[PATCH bpf v2 07/15] bpf, sockmap: Check the target of an egress redirect before each send

[Alexei Starovoitov]

From: Chris Mason <mason@kernel.org>

tcp_sendmsg_locked() expects a caller that has the socket open. It uses
sk->sk_socket and the wait queue sk->sk_wq. For an egress redirect of an
sk_msg prog the sender calls it on the target without a reference to the
target's file. Both pointers are NULL before accept() and after close().
close() can occur while the sender sleeps. Then the kernel crashes.

Fail the redirect when the target has no struct socket, or when the sender
has to wait for the connection. The error is EINVAL. After close() it is
EPIPE, as today. sk_stream_wait_connect() calls remove_wait_queue() without
a test of SOCK_DEAD. sk_stream_wait_memory() has this test, but it can
still use sk_wq after the sleep. This patch does not change that. The
target can change while the sender sleeps there. Thus do both checks before
each call of tcp_sendmsg_locked(). Clear MSG_FASTOPEN, so that the sender
does not call the connect code of the target, which uses both pointers.

Today the sender can send before accept() and waits in SYN_SENT. send()
with MSG_FASTOPEN sends the data. Today it fails with EISCONN. When the
redirect of a cork fails, send() returns success and the kernel drops the
data, as today.

The Fixes tags are for the two checks and for MSG_FASTOPEN, in this order.

Fixes: 4f738adba30a ("bpf: create tcp_bpf_ulp allowing BPF to monitor socket TX/RX data")
Fixes: b8b8315e39ff ("bpf, sockmap: Remove unhash handler for BPF sockmap usage")
Fixes: c5c37af6ecad ("tcp: Convert do_tcp_sendpages() to use MSG_SPLICE_PAGES")
Signed-off-by: Chris Mason <mason@kernel.org>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/ipv4/tcp_bpf.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index fd443350f9b8..ef2e15de0fe3 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -88,6 +88,24 @@ static int bpf_tcp_ingress(struct sock *sk, struct sk_psock *psock,
 	return ret;
 }
 
+/*
+ * The target of a redirect belongs to somebody else. A child that nobody has
+ * accepted yet, and an orphan, have no struct socket and no wait queue.
+ * tcp_sendmsg_locked() needs both. See sendmsg_locked() in skbuff.c.
+ * The sender holds no reference to the file of the target, which can be closed
+ * whenever tcp_sendmsg_locked() sleeps. sk_stream_wait_connect() and
+ * inet_wait_for_connect() go back to the wait queue when they wake up, so the
+ * sender must not wait for a connect.
+ */
+static bool tcp_bpf_redir_allowed(const struct sock *sk)
+{
+	if (!sk->sk_socket)
+		return false;
+	if ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV))
+		return tcp_passive_fastopen(sk);
+	return true;
+}
+
 static int tcp_bpf_push(struct sock *sk, struct sk_msg *msg, u32 apply_bytes,
 			int flags, bool uncharge)
 {
@@ -109,6 +127,10 @@ static int tcp_bpf_push(struct sock *sk, struct sk_msg *msg, u32 apply_bytes,
 		page = sg_page(sge);
 
 retry:
+		/* Only a redirect sends data which is not charged to sk */
+		if (!uncharge && unlikely(!tcp_bpf_redir_allowed(sk)))
+			return sock_flag(sk, SOCK_DEAD) ? -EPIPE : -EINVAL;
+
 		msghdr.msg_flags = flags | MSG_SPLICE_PAGES;
 		has_tx_ulp = tls_sw_has_ctx_tx(sk);
 		if (has_tx_ulp)
@@ -555,7 +577,7 @@ static int tcp_bpf_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 	int flags;
 
 	/* Don't let internal flags through */
-	flags = (msg->msg_flags & ~MSG_SENDPAGE_DECRYPTED);
+	flags = (msg->msg_flags & ~(MSG_SENDPAGE_DECRYPTED | MSG_FASTOPEN));
 	flags |= MSG_NO_SHARED_FRAGS;
 
 	psock = sk_psock_get(sk);
-- 
2.55.0

Re: [PATCH bpf v2 07/15] bpf, sockmap: Check the target of an egress redirect before each send

[Jiayuan Chen]

On 11/2/26 3:21 PM, Alexei Starovoitov wrote:
> From: Chris Mason <mason@kernel.org>
>
> tcp_sendmsg_locked() expects a caller that has the socket open. It uses
> sk->sk_socket and the wait queue sk->sk_wq. For an egress redirect of an
> sk_msg prog the sender calls it on the target without a reference to the
> target's file. Both pointers are NULL before accept() and after close().
> close() can occur while the sender sleeps. Then the kernel crashes.
>
> Fail the redirect when the target has no struct socket, or when the sender
> has to wait for the connection. The error is EINVAL. After close() it is
> EPIPE, as today. sk_stream_wait_connect() calls remove_wait_queue() without
> a test of SOCK_DEAD. sk_stream_wait_memory() has this test, but it can
> still use sk_wq after the sleep. This patch does not change that. The
> target can change while the sender sleeps there. Thus do both checks before
> each call of tcp_sendmsg_locked(). Clear MSG_FASTOPEN, so that the sender
> does not call the connect code of the target, which uses both pointers.
>
> Today the sender can send before accept() and waits in SYN_SENT. send()
> with MSG_FASTOPEN sends the data. Today it fails with EISCONN. When the
> redirect of a cork fails, send() returns success and the kernel drops the
> data, as today.
>
> The Fixes tags are for the two checks and for MSG_FASTOPEN, in this order.
>
> Fixes: 4f738adba30a ("bpf: create tcp_bpf_ulp allowing BPF to monitor socket TX/RX data")
> Fixes: b8b8315e39ff ("bpf, sockmap: Remove unhash handler for BPF sockmap usage")
> Fixes: c5c37af6ecad ("tcp: Convert do_tcp_sendpages() to use MSG_SPLICE_PAGES")
> Signed-off-by: Chris Mason <mason@kernel.org>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
>  net/ipv4/tcp_bpf.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index fd443350f9b8..ef2e15de0fe3 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -88,6 +88,24 @@ static int bpf_tcp_ingress(struct sock *sk, struct sk_psock *psock,
>  	return ret;
>  }
>  
> +/*
> + * The target of a redirect belongs to somebody else. A child that nobody has
> + * accepted yet, and an orphan, have no struct socket and no wait queue.
> + * tcp_sendmsg_locked() needs both. See sendmsg_locked() in skbuff.c.
> + * The sender holds no reference to the file of the target, which can be closed
> + * whenever tcp_sendmsg_locked() sleeps. sk_stream_wait_connect() and
> + * inet_wait_for_connect() go back to the wait queue when they wake up, so the
> + * sender must not wait for a connect.
> + */
> +static bool tcp_bpf_redir_allowed(const struct sock *sk)
> +{
> +	if (!sk->sk_socket)
> +		return false;
> +	if ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV))
> +		return tcp_passive_fastopen(sk);
> +	return true;
> +}
> +
>  static int tcp_bpf_push(struct sock *sk, struct sk_msg *msg, u32 apply_bytes,
>  			int flags, bool uncharge)
>  {
> @@ -109,6 +127,10 @@ static int tcp_bpf_push(struct sock *sk, struct sk_msg *msg, u32 apply_bytes,
>  		page = sg_page(sge);
>  
>  retry:
> +		/* Only a redirect sends data which is not charged to sk */
> +		if (!uncharge && unlikely(!tcp_bpf_redir_allowed(sk)))
> +			return sock_flag(sk, SOCK_DEAD) ? -EPIPE : -EINVAL;
> +


Reviewed-by: Jiayuan Chen <jiayuan.chen@linux.dev>

I can trigger it by redirecting data to a non-accept sk.

==================================================================
BUG: KASAN: null-ptr-deref in tcp_sendmsg_locked+0xfb8/0x4550
Write of size 8 at addr 0000000000000008 by task repro/488
Tainted: [W]=WARN
Call Trace:
 <TASK>
 dump_stack_lvl+0x91/0xf0
 print_report+0x442/0x630
 kasan_report+0xf3/0x130
 kasan_check_range+0x11c/0x200
 __kasan_check_write+0x14/0x30
 tcp_sendmsg_locked+0xfb8/0x4550
 tcp_bpf_push+0x3d0/0xbc0
 tcp_bpf_sendmsg_redir+0x639/0x1050
 tcp_bpf_sendmsg+0xa32/0x1b90
 inet_sendmsg+0xdf/0x100
 __sys_sendto+0x3ed/0x4f0
 __x64_sys_sendto+0xe0/0x1b0
 x64_sys_call+0x1375/0x27e0
 do_syscall_64+0xe5/0x610

Re: [PATCH bpf v2 07/15] bpf, sockmap: Check the target of an egress redirect before each send

[Alexei Starovoitov]

On Mon, Nov 02, 2026 at 04:32 AM Jiayuan Chen <jiayuan.chen@linux.dev> wrote:
> I can trigger it by redirecting data to a non-accept sk.
>
> ==================================================================
> BUG: KASAN: null-ptr-deref in tcp_sendmsg_locked+0xfb8/0x4550
> Write of size 8 at addr 0000000000000008 by task repro/488

Thanks for testing.
All patches have reproducers.
I didn't share them, hough it's a weak defense.
Anyone can build a repro from the patch.

Re: [PATCH bpf v2 07/15] bpf, sockmap: Check the target of an egress redirect before each send

[John Fastabend]

On Mon, 02 Nov 2026 00:21, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>From: Chris Mason <mason@kernel.org>
>
>tcp_sendmsg_locked() expects a caller that has the socket open. It uses
>sk->sk_socket and the wait queue sk->sk_wq. For an egress redirect of an
>sk_msg prog the sender calls it on the target without a reference to the
>target's file. Both pointers are NULL before accept() and after close().
>close() can occur while the sender sleeps. Then the kernel crashes.
>
>Fail the redirect when the target has no struct socket, or when the sender
>has to wait for the connection. The error is EINVAL. After close() it is
>EPIPE, as today. sk_stream_wait_connect() calls remove_wait_queue() without
>a test of SOCK_DEAD. sk_stream_wait_memory() has this test, but it can
>still use sk_wq after the sleep. This patch does not change that. The
>target can change while the sender sleeps there. Thus do both checks before
>each call of tcp_sendmsg_locked(). Clear MSG_FASTOPEN, so that the sender
>does not call the connect code of the target, which uses both pointers.
>
>Today the sender can send before accept() and waits in SYN_SENT. send()
>with MSG_FASTOPEN sends the data. Today it fails with EISCONN. When the
>redirect of a cork fails, send() returns success and the kernel drops the
>data, as today.
>
>The Fixes tags are for the two checks and for MSG_FASTOPEN, in this order.
>

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

>Fixes: 4f738adba30a ("bpf: create tcp_bpf_ulp allowing BPF to monitor socket TX/RX data")
>Fixes: b8b8315e39ff ("bpf, sockmap: Remove unhash handler for BPF sockmap usage")
>Fixes: c5c37af6ecad ("tcp: Convert do_tcp_sendpages() to use MSG_SPLICE_PAGES")
>Signed-off-by: Chris Mason <mason@kernel.org>
>Signed-off-by: Alexei Starovoitov <ast@kernel.org>
>---

Re: [PATCH bpf v2 07/15] bpf, sockmap: Check the target of an egress redirect before each send

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (2) · Low: 3

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 claim: tcp_bpf_redir_allowed() runs only before
  tcp_sendmsg_locked().
- [Low] Non-blocking egress redirects to a connecting target now fail
  with EINVAL instead of EAGAIN, and the commit message does not say so.
- [Low] The claim: tcp_bpf_sendmsg() strips MSG_FASTOPEN for every psock
  send, including __SK_PASS to the sender's own socket, while the…
- [Low] The comment in tcp_bpf_sendmsg() is now inaccurate.

Pre-existing issues:
- [Medium] An egress-redirect sender sleeping in sk_stream_wait_memory()
  on a target's wait queue is not woken when the target's owner closes…
- [Medium] tcp_bpf_push_locked() (and bpf_tcp_ingress()) call lock_sock()
  on a redirect target that may be a child still in a listener's accept…

--

bpf, sockmap: Check the target of an egress redirect before each send

This adds tcp_bpf_redir_allowed() and calls it in tcp_bpf_push() before
each tcp_sendmsg_locked() call made for an egress redirect. The redirect
fails with -EINVAL when the target has no struct socket or is still
connecting, and with -EPIPE after close(). It also clears MSG_FASTOPEN in
tcp_bpf_sendmsg(), so a redirect cannot enter the target's connect code.

> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index fd443350f9b81..ef2e15de0fe3b 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -88,6 +88,24 @@ static int bpf_tcp_ingress(struct sock *sk, struct sk_psock *psock,
>  	return ret;
>  }
>  
> +/*
> + * The target of a redirect belongs to somebody else. A child that nobody has
> + * accepted yet, and an orphan, have no struct socket and no wait queue.
> + * tcp_sendmsg_locked() needs both. See sendmsg_locked() in skbuff.c.
> + * The sender holds no reference to the file of the target, which can be closed
> + * whenever tcp_sendmsg_locked() sleeps. sk_stream_wait_connect() and
> + * inet_wait_for_connect() go back to the wait queue when they wake up, so the
> + * sender must not wait for a connect.
> + */

[Severity: Medium]
This isn't a bug introduced by this patch, but the comment mentions
unaccepted children as redirect targets. Can tcp_bpf_push_locked() hold
the lock of such a child while the listener tears it down?

A sockops BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB program can put a child that
is still in the accept queue into a sockmap. sock_map_sk_state_allowed()
accepts TCP_ESTABLISHED:

    return (1 << sk->sk_state) & (TCPF_ESTABLISHED | TCPF_LISTEN);

The redirect sender then does:

tcp_bpf_push_locked()
    lock_sock(sk);
    ret = tcp_bpf_push(sk, msg, apply_bytes, flags, uncharge);
    release_sock(sk);

If the listener is closed in that window, inet_csk_listen_stop() does:

    bh_lock_sock(child);
    WARN_ON(sock_owned_by_user(child));
    sock_hold(child);

Then inet_child_forget() disconnects, orphans and destroys the child
without waiting for the lock owner.

Would the sender's release_sock() then run __release_sock() and
tcp_release_cb() on the torn-down child?

bpf_tcp_ingress() takes lock_sock() on the target in the same way. This
patch makes the egress window shorter, because the lock is now held only
long enough to find sk_socket == NULL and return -EINVAL. The race itself
seems to remain.

> +static bool tcp_bpf_redir_allowed(const struct sock *sk)
> +{
> +	if (!sk->sk_socket)
> +		return false;
> +	if ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV))
> +		return tcp_passive_fastopen(sk);

[Severity: Low]
Does this change the errno that non-blocking senders see? Every target in
TCP_SYN_SENT is rejected, whatever the sender's timeout is. One example is
a sockmap member that disconnected and then reconnected.

Before this patch, a sender using MSG_DONTWAIT got timeo == 0 in
tcp_sendmsg_locked(). sk_stream_wait_connect() then returned before
add_wait_queue():

net/core/stream.c:sk_stream_wait_connect() {
    ...
		if (!*timeo_p)
			return -EAGAIN;
    ...
}

So this case never reached the remove_wait_queue() path. Now tcp_bpf_push()
returns -EINVAL before it calls tcp_sendmsg_locked(), so a retryable EAGAIN
becomes EINVAL.

The commit message says the redirect fails "when the sender has to wait
for the connection". A non-blocking sender never waits, though.

Could non-blocking senders keep getting -EAGAIN here? If not, could the
commit message mention the errno change?

> +	return true;
> +}
> +
>  static int tcp_bpf_push(struct sock *sk, struct sk_msg *msg, u32 apply_bytes,
>  			int flags, bool uncharge)
>  {
> @@ -109,6 +127,10 @@ static int tcp_bpf_push(struct sock *sk, struct sk_msg *msg, u32 apply_bytes,
>  		page = sg_page(sge);
>  
>  retry:
> +		/* Only a redirect sends data which is not charged to sk */
> +		if (!uncharge && unlikely(!tcp_bpf_redir_allowed(sk)))
> +			return sock_flag(sk, SOCK_DEAD) ? -EPIPE : -EINVAL;

[Severity: Medium]
This check only runs before each tcp_sendmsg_locked() call. Suppose
tcp_sendmsg_locked() sleeps in sk_stream_wait_memory() on an accepted
ESTABLISHED target, and the owner calls close(). sock_orphan() then clears
sk_wq:

    write_lock_bh(&sk->sk_callback_lock);
    sock_set_flag(sk, SOCK_DEAD);
    sk_set_socket(sk, NULL);
    sk->sk_wq  = NULL;

On wakeup, the loop in sk_stream_wait_memory() goes back to:

	while (1) {
		sk_set_bit(SOCKWQ_ASYNC_NOSPACE, sk);

Can this dereference a NULL sk_wq_raw when SOCK_FASYNC is set on the
target?

The commit message says so: "sk_stream_wait_memory() has this test, but
it can still use sk_wq after the sleep. This patch does not change that."

The next patch in the series, "net: Leave sk_stream_wait_memory() once the
socket is dead", adds this right after sk_wait_event():

		if (ret < 0 || sock_flag(sk, SOCK_DEAD))
			goto do_error;

After that, the retry check here returns -EPIPE. So the window exists only
at this commit, or in a backport that takes this patch without the next
one.

Could the series be reordered so that fix lands first?

The out: label also skips remove_wait_queue() for a dead socket, which
leaves the on-stack wait entry on the old queue. That skip predates this
series, and no waker was found that could still walk that queue.

[Severity: Medium]
This is a pre-existing issue, but it is related to the sleep above. Is a
redirect sender that sleeps in sk_stream_wait_memory() on the target woken
at all when the target's owner closes it?

In the default close path, __tcp_close() does:

    WRITE_ONCE(sk->sk_shutdown, SHUTDOWN_MASK);

Then it calls tcp_close_state() and tcp_send_fin(), or tcp_set_state(TCP_CLOSE)
plus a reset when data was unread, and then sock_orphan(). None of these
wakes sk_sleep(sk). Only the tcp_disconnect() paths call
sk_error_report().

After sock_orphan(), sk_stream_write_space() does nothing:

	if (__sk_stream_is_writeable(sk, 1) && sock) {

tcp_done() also skips sk_state_change() for a SOCK_DEAD socket.

The sender would then stay in TASK_INTERRUPTIBLE until the target's
sk_sndtimeo expires (MAX_SCHEDULE_TIMEOUT by default) or a signal arrives.
All that time it pins the target sock (sock_hold() in
tcp_bpf_send_verdict()) and its psock (sk_psock_get() in
tcp_bpf_sendmsg_redir()).

The check added here runs before tcp_sendmsg_locked(), so it can't help a
sender that is already asleep inside it. The SOCK_DEAD exit added later in
the series only helps after a wakeup, so the hang seems to remain once the
whole series is applied.

> +
>  		msghdr.msg_flags = flags | MSG_SPLICE_PAGES;
>  		has_tx_ulp = tls_sw_has_ctx_tx(sk);
>  		if (has_tx_ulp)
> @@ -555,7 +577,7 @@ static int tcp_bpf_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>  	int flags;
>  
>  	/* Don't let internal flags through */
> -	flags = (msg->msg_flags & ~MSG_SENDPAGE_DECRYPTED);
> +	flags = (msg->msg_flags & ~(MSG_SENDPAGE_DECRYPTED | MSG_FASTOPEN));

[Severity: Low]
This clears MSG_FASTOPEN for every psock send. That includes __SK_PASS to
the sender's own socket in tcp_bpf_send_verdict():

    ret = tcp_bpf_push(sk, msg, tosend, flags, true);

The fallback without a psock keeps the flag:

	if (unlikely(!psock))
		return tcp_sendmsg(sk, msg, size);

So the same sendmsg(MSG_FASTOPEN) on a connected socket returns -EISCONN
without a psock and succeeds with one. Is that difference intended?

The commit message does describe it ("send() with MSG_FASTOPEN sends the
data. Today it fails with EISCONN."). It also matches the behaviour before
c5c37af6ecad, when do_tcp_sendpages() ignored MSG_FASTOPEN.

tcp_bpf_push() builds its msghdr with no msg_name, so a fastopen connect
could never succeed on this path anyway. This may well be fine as is.

[Severity: Low]
This isn't a bug, but the comment above still says "Don't let internal
flags through". The mask now also clears MSG_FASTOPEN, which is a UAPI
send flag rather than an internal one.

The reason for clearing it is that the target's connect code uses
sk_socket and sk_wq without a file reference. That reason is only in the
commit message, and the comment on tcp_bpf_redir_allowed() does not
mention MSG_FASTOPEN.

Could the comment be updated to say why MSG_FASTOPEN is cleared?

>  	flags |= MSG_NO_SHARED_FRAGS;
>  
>  	psock = sk_psock_get(sk);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102072112.4160195-1-alexei.starovoitov%40gmail.com

[PATCH bpf v2 08/15] net: Leave sk_stream_wait_memory() once the socket is dead

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

When an sk_msg prog redirects data to the egress of another socket, the
sender calls tcp_sendmsg_locked() on that socket. The sender holds a
reference to the sock, but not to its file. Thus __tcp_close() can call
sock_orphan() while the sender sleeps in sk_stream_wait_memory().
sock_orphan() sets SOCK_DEAD and sets sk_wq to NULL.

After the sleep the loop continues and does not test SOCK_DEAD. The
subsequent sk_set_bit() can dereference the NULL sk_wq. The tests of sk_err
and sk_shutdown do not always stop the sender. sock_error() clears sk_err,
and __tcp_close() can call tcp_disconnect(), which clears sk_shutdown. The
sender then adds data to the write queue of the dead socket, and
inet_sock_destruct() warns.

Leave the loop with -EPIPE when the socket is dead after the sleep. The
function already tests SOCK_DEAD before remove_wait_queue() for the same
reason. A caller that has the socket open never sees SOCK_DEAD.

Fixes: 4f738adba30a ("bpf: create tcp_bpf_ulp allowing BPF to monitor socket TX/RX data")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/stream.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/core/stream.c b/net/core/stream.c
index cb255e2a3a5d..334124d683a7 100644
--- a/net/core/stream.c
+++ b/net/core/stream.c
@@ -150,7 +150,11 @@ int sk_stream_wait_memory(struct sock *sk, long *timeo_p)
 				    (sk_stream_memory_free(sk) && !vm_wait),
 				    &wait);
 		sk->sk_write_pending--;
-		if (ret < 0)
+		/*
+		 * A sender which sockmap has redirected to sk does not keep it
+		 * open. A dead socket has no sk_wq.
+		 */
+		if (ret < 0 || sock_flag(sk, SOCK_DEAD))
 			goto do_error;
 
 		if (vm_wait) {
-- 
2.55.0

Re: [PATCH bpf v2 08/15] net: Leave sk_stream_wait_memory() once the socket is dead

[Jiayuan Chen]

On 11/2/26 3:21 PM, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@kernel.org>
>
> When an sk_msg prog redirects data to the egress of another socket, the
> sender calls tcp_sendmsg_locked() on that socket. The sender holds a
> reference to the sock, but not to its file. Thus __tcp_close() can call
> sock_orphan() while the sender sleeps in sk_stream_wait_memory().
> sock_orphan() sets SOCK_DEAD and sets sk_wq to NULL.
>
> After the sleep the loop continues and does not test SOCK_DEAD. The
> subsequent sk_set_bit() can dereference the NULL sk_wq. The tests of sk_err
> and sk_shutdown do not always stop the sender. sock_error() clears sk_err,
> and __tcp_close() can call tcp_disconnect(), which clears sk_shutdown. The
> sender then adds data to the write queue of the dead socket, and
> inet_sock_destruct() warns.
>
> Leave the loop with -EPIPE when the socket is dead after the sleep. The
> function already tests SOCK_DEAD before remove_wait_queue() for the same
> reason. A caller that has the socket open never sees SOCK_DEAD.
>
> Fixes: 4f738adba30a ("bpf: create tcp_bpf_ulp allowing BPF to monitor socket TX/RX data")
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>


Reviewed-by: Jiayuan Chen <jiayuan.chen@linux.dev>


> ---
>  net/core/stream.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/stream.c b/net/core/stream.c
> index cb255e2a3a5d..334124d683a7 100644
> --- a/net/core/stream.c
> +++ b/net/core/stream.c
> @@ -150,7 +150,11 @@ int sk_stream_wait_memory(struct sock *sk, long *timeo_p)
>  				    (sk_stream_memory_free(sk) && !vm_wait),
>  				    &wait);
>  		sk->sk_write_pending--;
> -		if (ret < 0)
> +		/*
> +		 * A sender which sockmap has redirected to sk does not keep it
> +		 * open. A dead socket has no sk_wq.
> +		 */
> +		if (ret < 0 || sock_flag(sk, SOCK_DEAD))
>  			goto do_error;
>  
>  		if (vm_wait) {

[PATCH bpf v2 09/15] bpf, sockmap: Check the TCP state in recvmsg()

[Alexei Starovoitov]

From: Chris Mason <mason@kernel.org>

With an sk_skb verdict prog, tcp_bpf_recvmsg_parser() copies data from the
psock. Then it calls __tcp_cleanup_rbuf() to calculate the receive window
and to send an ACK. The window and the ACK need a connection on the socket.
But tcp_disconnect() keeps the socket in the sockmap and the data on the
psock. For a TCP socket sock_map_redirect_allowed() rejects only
TCP_LISTEN. Thus the psock can also get data after tcp_disconnect().

__tcp_select_window() divides by icsk_ack.rcv_mss, which can be 0 in
TCP_CLOSE. In TCP_SYN_SENT the new peer can get an ACK with the sequence
numbers of the old connection.

Call __tcp_cleanup_rbuf() only when tcp_can_send_ack() returns true.
recvmsg() returns the data as before. Move tcp_can_send_ack() from MPTCP to
tcp.h. It also returns false for a fastopen child in TCP_SYN_RECV. From its
peer the child has only the SYN data, and the SYN-ACK acknowledges it.

Fixes: e5c6de5fa025 ("bpf, sockmap: Incorrectly handling copied_seq")
Signed-off-by: Chris Mason <mason@kernel.org>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/net/tcp.h    | 6 ++++++
 net/ipv4/tcp_bpf.c   | 3 ++-
 net/mptcp/protocol.c | 6 ------
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 3560adddf821..23a22d70718e 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1845,6 +1845,12 @@ static inline void tcp_adjust_rcv_ssthresh(struct sock *sk)
 void tcp_cleanup_rbuf(struct sock *sk, int copied);
 void __tcp_cleanup_rbuf(struct sock *sk, int copied);
 
+/* Is the socket in a state in which TCP may send an ACK? */
+static inline bool tcp_can_send_ack(const struct sock *sk)
+{
+	return !((1 << inet_sk_state_load(sk)) &
+	       (TCPF_SYN_SENT | TCPF_SYN_RECV | TCPF_TIME_WAIT | TCPF_CLOSE | TCPF_LISTEN));
+}
 
 /* We provision sk_rcvbuf around 200% of sk_rcvlowat.
  * If 87.5 % (7/8) of the space has been consumed, we want to override
diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index ef2e15de0fe3..3d703f1517d8 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -343,7 +343,8 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
 	if (!peek)
 		WRITE_ONCE(tcp->copied_seq, seq);
 	tcp_rcv_space_adjust(sk);
-	if (copied > 0)
+	/* Data on the psock outlives the connection that it came in on */
+	if (copied > 0 && tcp_can_send_ack(sk))
 		__tcp_cleanup_rbuf(sk, copied);
 
 unlock:
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 7a22e9388e0e..d62cfbf2d490 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -619,12 +619,6 @@ static void mptcp_set_timeout(struct sock *sk)
 	__mptcp_set_timeout(sk, tout);
 }
 
-static inline bool tcp_can_send_ack(const struct sock *ssk)
-{
-	return !((1 << inet_sk_state_load(ssk)) &
-	       (TCPF_SYN_SENT | TCPF_SYN_RECV | TCPF_TIME_WAIT | TCPF_CLOSE | TCPF_LISTEN));
-}
-
 void __mptcp_subflow_send_ack(struct sock *ssk)
 {
 	if (tcp_can_send_ack(ssk))
-- 
2.55.0

Re: [PATCH bpf v2 09/15] bpf, sockmap: Check the TCP state in recvmsg()

[Jiayuan Chen]

On 11/2/26 3:21 PM, Alexei Starovoitov wrote:
> From: Chris Mason <mason@kernel.org>
>
> With an sk_skb verdict prog, tcp_bpf_recvmsg_parser() copies data from the
> psock. Then it calls __tcp_cleanup_rbuf() to calculate the receive window
> and to send an ACK. The window and the ACK need a connection on the socket.
> But tcp_disconnect() keeps the socket in the sockmap and the data on the
> psock. For a TCP socket sock_map_redirect_allowed() rejects only
> TCP_LISTEN. Thus the psock can also get data after tcp_disconnect().
>
> __tcp_select_window() divides by icsk_ack.rcv_mss, which can be 0 in
> TCP_CLOSE. In TCP_SYN_SENT the new peer can get an ACK with the sequence
> numbers of the old connection.
>
> Call __tcp_cleanup_rbuf() only when tcp_can_send_ack() returns true.
> recvmsg() returns the data as before. Move tcp_can_send_ack() from MPTCP to
> tcp.h. It also returns false for a fastopen child in TCP_SYN_RECV. From its
> peer the child has only the SYN data, and the SYN-ACK acknowledges it.
>
> Fixes: e5c6de5fa025 ("bpf, sockmap: Incorrectly handling copied_seq")
> Signed-off-by: Chris Mason <mason@kernel.org>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>


Reviewed-by: Jiayuan Chen <jiayuan.chen@linux.dev>


> ---
>  include/net/tcp.h    | 6 ++++++
>  net/ipv4/tcp_bpf.c   | 3 ++-
>  net/mptcp/protocol.c | 6 ------
>  3 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 3560adddf821..23a22d70718e 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1845,6 +1845,12 @@ static inline void tcp_adjust_rcv_ssthresh(struct sock *sk)
>  void tcp_cleanup_rbuf(struct sock *sk, int copied);
>  void __tcp_cleanup_rbuf(struct sock *sk, int copied);
>  
> +/* Is the socket in a state in which TCP may send an ACK? */
> +static inline bool tcp_can_send_ack(const struct sock *sk)
> +{
> +	return !((1 << inet_sk_state_load(sk)) &
> +	       (TCPF_SYN_SENT | TCPF_SYN_RECV | TCPF_TIME_WAIT | TCPF_CLOSE | TCPF_LISTEN));
> +}
>  
>  /* We provision sk_rcvbuf around 200% of sk_rcvlowat.
>   * If 87.5 % (7/8) of the space has been consumed, we want to override
> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index ef2e15de0fe3..3d703f1517d8 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -343,7 +343,8 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
>  	if (!peek)
>  		WRITE_ONCE(tcp->copied_seq, seq);
>  	tcp_rcv_space_adjust(sk);
> -	if (copied > 0)
> +	/* Data on the psock outlives the connection that it came in on */
> +	if (copied > 0 && tcp_can_send_ack(sk))
>  		__tcp_cleanup_rbuf(sk, copied);
>  
>  unlock:
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 7a22e9388e0e..d62cfbf2d490 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -619,12 +619,6 @@ static void mptcp_set_timeout(struct sock *sk)
>  	__mptcp_set_timeout(sk, tout);
>  }
>  
> -static inline bool tcp_can_send_ack(const struct sock *ssk)
> -{
> -	return !((1 << inet_sk_state_load(ssk)) &
> -	       (TCPF_SYN_SENT | TCPF_SYN_RECV | TCPF_TIME_WAIT | TCPF_CLOSE | TCPF_LISTEN));
> -}
> -
>  void __mptcp_subflow_send_ack(struct sock *ssk)
>  {
>  	if (tcp_can_send_ack(ssk))

Re: [PATCH bpf v2 09/15] bpf, sockmap: Check the TCP state in recvmsg()

[Matthieu Baerts]

Hi Alexei, Chris,

Thank you for the fix!

On 02/11/2026 08:21, Alexei Starovoitov wrote:
> From: Chris Mason <mason@kernel.org>
> 
> With an sk_skb verdict prog, tcp_bpf_recvmsg_parser() copies data from the
> psock. Then it calls __tcp_cleanup_rbuf() to calculate the receive window
> and to send an ACK. The window and the ACK need a connection on the socket.
> But tcp_disconnect() keeps the socket in the sockmap and the data on the
> psock. For a TCP socket sock_map_redirect_allowed() rejects only
> TCP_LISTEN. Thus the psock can also get data after tcp_disconnect().
> 
> __tcp_select_window() divides by icsk_ack.rcv_mss, which can be 0 in
> TCP_CLOSE. In TCP_SYN_SENT the new peer can get an ACK with the sequence
> numbers of the old connection.
> 
> Call __tcp_cleanup_rbuf() only when tcp_can_send_ack() returns true.
> recvmsg() returns the data as before. Move tcp_can_send_ack() from MPTCP to
> tcp.h.

Good idea!

Acked-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH bpf v2 09/15] bpf, sockmap: Check the TCP state in recvmsg()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Concern (commit message accuracy): the concern says the commit
  message wrongly claims icsk_ack.rcv_mss "can be 0 in TCP_CLOSE",…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but psock data
  from an old connection still advances tp->copied_seq of the next…

--

bpf, sockmap: Check the TCP state in recvmsg()

This commit makes tcp_bpf_recvmsg_parser() call __tcp_cleanup_rbuf() only
when tcp_can_send_ack() returns true. The reason is that psock data can
outlive the connection it arrived on, for example after tcp_disconnect().
tcp_can_send_ack() moves from net/mptcp/protocol.c to include/net/tcp.h so
both users can share it.

> __tcp_select_window() divides by icsk_ack.rcv_mss, which can be 0 in
> TCP_CLOSE.

[Severity: Low]
This isn't a bug, and the statement looks accurate. Still, a reader may
wonder how rcv_mss gets to 0 in TCP_CLOSE, because tcp_disconnect()
explicitly guards against it:

net/ipv4/tcp.c:tcp_disconnect() {
    ...
	inet_csk_delack_init(sk);
	/* Initialize rcv_mss to TCP_MIN_MSS to avoid division by 0
	 * issue in __tcp_select_window()
	 */
	icsk->icsk_ack.rcv_mss = TCP_MIN_MSS;
    ...
}

It seems to happen when a later listen() on the disconnected socket fails.
inet_csk_listen_start() calls inet_csk_delack_init(), which zeroes rcv_mss.
If get_port() or hash() then fails, the socket returns to TCP_CLOSE with
rcv_mss still 0:

net/ipv4/inet_connection_sock.c:inet_csk_listen_start() {
    ...
	inet_csk_delack_init(sk);
    ...
	err = sk->sk_prot->get_port(sk, inet->inet_num);
    ...
	inet_sk_set_state(sk, TCP_CLOSE);
	return err;
}

Would it help to mention this path, and that TCP_LISTEN is covered as
well, in the commit message?

> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index ef2e15de0fe3b..3d703f1517d88 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -343,7 +343,8 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
>  	if (!peek)
>  		WRITE_ONCE(tcp->copied_seq, seq);
>  	tcp_rcv_space_adjust(sk);
> -	if (copied > 0)
> +	/* Data on the psock outlives the connection that it came in on */
> +	if (copied > 0 && tcp_can_send_ack(sk))
>  		__tcp_cleanup_rbuf(sk, copied);

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but the
new comment points at it. If psock data outlives the connection, do the
copied_seq and tcp_rcv_space_adjust() updates just above the new check
still charge bytes from the old connection to the new one?

SK_PASS data is tagged as self-originated in sk_psock_skb_ingress_self():

	msg->sk = sk;

tcp_read_skb() does not advance copied_seq for these skbs. Instead,
tcp_bpf_recvmsg_parser() adds them later:

	seq += copied_from_self;
    ...
	if (!peek)
		WRITE_ONCE(tcp->copied_seq, seq);

tcp_disconnect() purges sk_receive_queue and sets copied_seq to rcv_nxt.
It leaves psock->ingress_msg alone, and the socket stays in the sockmap.
A later connect() resets copied_seq to the new rcv_nxt.

Here is a possible sequence:

  socket in a sockmap with an sk_skb stream_verdict prog returning SK_PASS
  data is received but not read (D bytes in psock->ingress_msg)
  connect(AF_UNSPEC) -> tcp_disconnect()
  connect() to a new peer, reaches ESTABLISHED
  recvmsg() -> tcp_bpf_recvmsg_parser() reads the D stale bytes

After this, copied_seq is new rcv_nxt + D and stays D bytes ahead for the
rest of the connection.

tcp_rcv_space_adjust() then computes inq = rcv_nxt - copied_seq = -D. That
inflates copied on every measurement and grows sk_rcvbuf and window_clamp
through tcp_rcvbuf_grow() for no reason.

If the socket later leaves the sockmap and falls back to
tcp_recvmsg_locked(), offset = *seq - TCP_SKB_CB(skb)->seq starts at D.
Wouldn't that silently skip the first D bytes of the new connection's
data? Or, when D is not less than skb->len, wouldn't it trigger the
"TCP recvmsg seq # bug 2" WARN in tcp_recvmsg_locked() or the
"cleanup rbuf bug" WARN in tcp_cleanup_rbuf()?

No later patch in the series seems to change how copied_seq is updated
here.

Should data that arrived on an earlier connection be kept out of the
copied_seq update too, and not only out of __tcp_cleanup_rbuf()?

>  
>  unlock:

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102072112.4160195-1-alexei.starovoitov%40gmail.com

[PATCH bpf v2 10/15] bpf, sockmap: Reset the copy position when corking

[Alexei Starovoitov]

From: Chris Mason <mason@kernel.org>

sk_msg_memcopy_from_iter() writes to the copy position: byte sg.copybreak
after the offset of element sg.curr. Both tcp_bpf_push() and sk_msg_xfer()
remove bytes from the front of an element. They increase its offset, but
they do not change sg.copybreak when the element is sg.curr.

tcp_bpf_send_verdict() can send a part of a message and keep the rest as
the cork. tcp_bpf_sendmsg() appends new data to it. If sk_msg_alloc() then
extends element sg.curr, the peer can get memory that nobody wrote:

  old offset    offset    end of data     offset + sg.copybreak
  | removed     | rest    | not written   | new data

Move sk_msg_reset_curr() from filter.c to skmsg.h. Call it when a message
becomes the cork, as the helpers that change a message do. It sets the copy
position to the end of the data. Without removed bytes it changes nothing.

Fixes: 91843d540a13 ("bpf: sockmap, add msg_cork_bytes() helper")
Signed-off-by: Chris Mason <mason@kernel.org>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/skmsg.h | 19 +++++++++++++++++++
 net/core/filter.c     | 14 --------------
 net/ipv4/tcp_bpf.c    |  5 +++++
 3 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
index d5e35f24738d..f0e27b4b929f 100644
--- a/include/linux/skmsg.h
+++ b/include/linux/skmsg.h
@@ -196,6 +196,25 @@ static inline void sk_msg_init(struct sk_msg *msg)
 	sg_init_marker(msg->sg.data, NR_MSG_FRAG_IDS);
 }
 
+/*
+ * sk_msg_memcopy_from_iter() writes to byte sg.copybreak of element sg.curr,
+ * counted from the offset of that element. Point both past the last byte of
+ * the message, after something other than a copy has changed its elements.
+ */
+static inline void sk_msg_reset_curr(struct sk_msg *msg)
+{
+	if (!msg->sg.size) {
+		msg->sg.curr = msg->sg.start;
+		msg->sg.copybreak = 0;
+	} else {
+		u32 i = msg->sg.end;
+
+		sk_msg_iter_var_prev(i);
+		msg->sg.curr = i;
+		msg->sg.copybreak = msg->sg.data[i].length;
+	}
+}
+
 static inline void sk_msg_xfer(struct sk_msg *dst, struct sk_msg *src,
 			       int which, u32 size)
 {
diff --git a/net/core/filter.c b/net/core/filter.c
index 9245009d9f1e..50c0cda778db 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2677,20 +2677,6 @@ BPF_CALL_2(bpf_msg_cork_bytes, struct sk_msg *, msg, u32, bytes)
 	return 0;
 }
 
-static void sk_msg_reset_curr(struct sk_msg *msg)
-{
-	if (!msg->sg.size) {
-		msg->sg.curr = msg->sg.start;
-		msg->sg.copybreak = 0;
-	} else {
-		u32 i = msg->sg.end;
-
-		sk_msg_iter_var_prev(i);
-		msg->sg.curr = i;
-		msg->sg.copybreak = msg->sg.data[i].length;
-	}
-}
-
 static bool sk_msg_elem_is_copy(const struct sk_msg *msg, u32 i)
 {
 	return test_bit(i, msg->sg.copy);
diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index 3d703f1517d8..d534974c33f8 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -469,6 +469,11 @@ static int tcp_bpf_send_verdict(struct sock *sk, struct sk_psock *psock,
 				return -ENOMEM;
 			}
 		}
+		/*
+		 * A push or a redirect may have taken bytes off the front of
+		 * element sg.curr. The next sendmsg() appends to the cork.
+		 */
+		sk_msg_reset_curr(msg);
 		memcpy(psock->cork, msg, sizeof(*msg));
 		return 0;
 	}
-- 
2.55.0

Re: [PATCH bpf v2 10/15] bpf, sockmap: Reset the copy position when corking

[Jiayuan Chen]

On 11/2/26 3:21 PM, Alexei Starovoitov wrote:
> From: Chris Mason <mason@kernel.org>
>
> sk_msg_memcopy_from_iter() writes to the copy position: byte sg.copybreak
> after the offset of element sg.curr. Both tcp_bpf_push() and sk_msg_xfer()
> remove bytes from the front of an element. They increase its offset, but
> they do not change sg.copybreak when the element is sg.curr.
>
> tcp_bpf_send_verdict() can send a part of a message and keep the rest as
> the cork. tcp_bpf_sendmsg() appends new data to it. If sk_msg_alloc() then
> extends element sg.curr, the peer can get memory that nobody wrote:
>
>   old offset    offset    end of data     offset + sg.copybreak
>   | removed     | rest    | not written   | new data
>
> Move sk_msg_reset_curr() from filter.c to skmsg.h. Call it when a message
> becomes the cork, as the helpers that change a message do. It sets the copy
> position to the end of the data. Without removed bytes it changes nothing.
>
> Fixes: 91843d540a13 ("bpf: sockmap, add msg_cork_bytes() helper")
> Signed-off-by: Chris Mason <mason@kernel.org>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>


Reviewed-by: Jiayuan Chen <jiayuan.chen@linux.dev>

[PATCH bpf v2 11/15] bpf, sockmap: Don't copy 0 bytes into a full cork

[Alexei Starovoitov]

From: Chris Mason <mason@kernel.org>

psock->cork is a ring. sk_msg_memcopy_from_iter() writes at sg.curr.
sk_msg_alloc() adds room at sg.end, where a full ring has a spare element.

When only a part of the data fits, tcp_bpf_sendmsg() copies this part and
flushes the cork. If nothing fits, it copies 0 bytes. This copy writes
nothing, but it moves sg.curr to sg.end and returns -ENOSPC. Thus sendmsg()
fails without a flush. sg.curr stays at sg.end, and from there a copy moves
it past the spare element. Then a copy can skip the first new element, and
the peer gets memory that nobody wrote.

Skip the copy when sk_msg_alloc() adds nothing, as tls_sw_sendmsg_locked()
does. This pass of the loop flushes the cork. The flush empties the element
at sg.curr, thus the copy in the next pass starts at the first new element.
Now sendmsg() on a full cork does not fail with ENOSPC. The prog runs on
less data than it wants to cork, as it does when a part fits.

The first Fixes tag is for the -ENOSPC without a flush. The commit in the
second tag added the spare element. Since then sg.curr can move past it.

Fixes: 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface")
Fixes: 031097d9e079 ("net: skmsg: fix TLS 1.3 crash with full sk_msg")
Signed-off-by: Chris Mason <mason@kernel.org>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/ipv4/tcp_bpf.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index d534974c33f8..d8e29b09818c 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -619,8 +619,15 @@ static int tcp_bpf_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 			copy = msg_tx->sg.size - osize;
 		}
 
-		ret = sk_msg_memcopy_from_iter(sk, &msg->msg_iter, msg_tx,
-					       copy);
+		/*
+		 * copy is 0 if the ring of the cork was full already and its
+		 * last element could not be extended. Flush the cork below,
+		 * and let the next pass take the data.
+		 */
+		ret = 0;
+		if (copy)
+			ret = sk_msg_memcopy_from_iter(sk, &msg->msg_iter,
+						       msg_tx, copy);
 		if (ret < 0) {
 			sk_msg_trim(sk, msg_tx, osize);
 			err = ret;
-- 
2.55.0

Re: [PATCH bpf v2 11/15] bpf, sockmap: Don't copy 0 bytes into a full cork

[Jiayuan Chen]

On 11/2/26 3:21 PM, Alexei Starovoitov wrote:
> From: Chris Mason <mason@kernel.org>
>
> psock->cork is a ring. sk_msg_memcopy_from_iter() writes at sg.curr.
> sk_msg_alloc() adds room at sg.end, where a full ring has a spare element.
>
> When only a part of the data fits, tcp_bpf_sendmsg() copies this part and
> flushes the cork. If nothing fits, it copies 0 bytes. This copy writes
> nothing, but it moves sg.curr to sg.end and returns -ENOSPC. Thus sendmsg()
> fails without a flush. sg.curr stays at sg.end, and from there a copy moves
> it past the spare element. Then a copy can skip the first new element, and
> the peer gets memory that nobody wrote.
>
> Skip the copy when sk_msg_alloc() adds nothing, as tls_sw_sendmsg_locked()
> does. This pass of the loop flushes the cork. The flush empties the element
> at sg.curr, thus the copy in the next pass starts at the first new element.
> Now sendmsg() on a full cork does not fail with ENOSPC. The prog runs on
> less data than it wants to cork, as it does when a part fits.
>
> The first Fixes tag is for the -ENOSPC without a flush. The commit in the
> second tag added the spare element. Since then sg.curr can move past it.
>
> Fixes: 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface")
> Fixes: 031097d9e079 ("net: skmsg: fix TLS 1.3 crash with full sk_msg")
> Signed-off-by: Chris Mason <mason@kernel.org>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>


Reviewed-by: Jiayuan Chen <jiayuan.chen@linux.dev>

[PATCH bpf v2 12/15] bpf, sockmap: Clear the redirect target before the sk_msg prog runs

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

bpf_msg_redirect_{map,hash}() store the target in msg->sk_redir, without a
reference. After the prog has run, sk_psock_msg_verdict() makes SK_PASS a
redirect if this field is set. But tcp_bpf_send_verdict() runs the prog
again on a cork, and on the rest of a partly sent message. Nothing clears
the field between runs. Thus SK_PASS without a new target is a redirect to
the old target. If that sock is already free, this is a use after free.

Clear msg->sk_redir before each run, as the skb verdict paths do with
skb_bpf_redirect_clear(). Only SK_PASS without a target changes. It now
sends the data on psock->sk. Then sendmsg() can return another value. A
redirect that applies to more bytes uses psock->sk_redir, with its own
reference. The counts msg->apply_bytes and msg->cork_bytes stay as today.

Fixes: e5cd3abcb31a ("bpf: sockmap, refactor sockmap routines to work with hashmap")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/skmsg.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index a5d498982848..8a3b8536c0c2 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -936,6 +936,13 @@ int sk_psock_msg_verdict(struct sock *sk, struct sk_psock *psock,
 
 	sk_msg_compute_data_pointers(msg);
 	msg->sk = sk;
+	/*
+	 * The prog may have run on this sk_msg before: it is a cork, or the
+	 * rest of a message of which only a part was sent. The target of that
+	 * run is not the target of this run, and the sk_msg holds no reference
+	 * to it.
+	 */
+	msg->sk_redir = NULL;
 	ret = bpf_prog_run_pin_on_cpu(prog, msg);
 	msg->sk = NULL;
 	ret = sk_psock_map_verd(ret, msg->sk_redir);
-- 
2.55.0

Re: [PATCH bpf v2 12/15] bpf, sockmap: Clear the redirect target before the sk_msg prog runs

[Jiayuan Chen]

On 11/2/26 3:21 PM, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@kernel.org>
>
> bpf_msg_redirect_{map,hash}() store the target in msg->sk_redir, without a
> reference. After the prog has run, sk_psock_msg_verdict() makes SK_PASS a
> redirect if this field is set. But tcp_bpf_send_verdict() runs the prog
> again on a cork, and on the rest of a partly sent message. Nothing clears
> the field between runs. Thus SK_PASS without a new target is a redirect to
> the old target. If that sock is already free, this is a use after free.
>
> Clear msg->sk_redir before each run, as the skb verdict paths do with
> skb_bpf_redirect_clear(). Only SK_PASS without a target changes. It now
> sends the data on psock->sk. Then sendmsg() can return another value. A
> redirect that applies to more bytes uses psock->sk_redir, with its own
> reference. The counts msg->apply_bytes and msg->cork_bytes stay as today.
>
> Fixes: e5cd3abcb31a ("bpf: sockmap, refactor sockmap routines to work with hashmap")
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>


Reviewed-by: Jiayuan Chen <jiayuan.chen@linux.dev>

[PATCH bpf v2 13/15] bpf, sockmap: Check that the redirected message is the cork

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

In tcp_bpf_send_verdict(), msg is psock->cork or the sk_msg on the stack of
tcp_bpf_sendmsg(). If psock->cork is not NULL, the SK_REDIRECT branch sets
it to NULL before release_sock() and calls kfree(msg) after the redirect.
Thus the branch assumes that a cork is msg.

This is not true after release_sock(), because a second sender on the same
socket can make a cork. The first sender can run the branch again, and then
psock->cork is not its msg. Nobody sends or frees the cork of the second
sender. kfree() gets the sk_msg on the stack, and the kernel crashes.

Compare psock->cork with msg. Nothing changes for one sender, because
psock->cork is then NULL or msg. The other branches do not change, and a
cork is still not safe for two senders on one socket.

Fixes: 4f738adba30a ("bpf: create tcp_bpf_ulp allowing BPF to monitor socket TX/RX data")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/ipv4/tcp_bpf.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index d8e29b09818c..b1071b0960cf 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -517,7 +517,11 @@ static int tcp_bpf_send_verdict(struct sock *sk, struct sk_psock *psock,
 			psock->eval = __SK_NONE;
 			psock->sk_redir = NULL;
 		}
-		if (psock->cork) {
+		/*
+		 * Another thread can make a cork while this one sends msg
+		 * without the socket lock. Leave that cork alone.
+		 */
+		if (psock->cork == msg) {
 			cork = true;
 			psock->cork = NULL;
 		}
-- 
2.55.0

Re: [PATCH bpf v2 13/15] bpf, sockmap: Check that the redirected message is the cork

[Jiayuan Chen]

On 11/2/26 3:21 PM, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@kernel.org>
>
> In tcp_bpf_send_verdict(), msg is psock->cork or the sk_msg on the stack of
> tcp_bpf_sendmsg(). If psock->cork is not NULL, the SK_REDIRECT branch sets
> it to NULL before release_sock() and calls kfree(msg) after the redirect.
> Thus the branch assumes that a cork is msg.
>
> This is not true after release_sock(), because a second sender on the same
> socket can make a cork. The first sender can run the branch again, and then
> psock->cork is not its msg. Nobody sends or frees the cork of the second
> sender. kfree() gets the sk_msg on the stack, and the kernel crashes.
>
> Compare psock->cork with msg. Nothing changes for one sender, because
> psock->cork is then NULL or msg. The other branches do not change, and a
> cork is still not safe for two senders on one socket.


I think I should try to serialize senders on the same socket. The underlying risk here probably goes beyond this series.


>
> Fixes: 4f738adba30a ("bpf: create tcp_bpf_ulp allowing BPF to monitor socket TX/RX data")
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>


Reviewed-by: Jiayuan Chen <jiayuan.chen@linux.dev>


> ---
>  net/ipv4/tcp_bpf.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index d8e29b09818c..b1071b0960cf 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -517,7 +517,11 @@ static int tcp_bpf_send_verdict(struct sock *sk, struct sk_psock *psock,
>  			psock->eval = __SK_NONE;
>  			psock->sk_redir = NULL;
>  		}
> -		if (psock->cork) {
> +		/*
> +		 * Another thread can make a cork while this one sends msg
> +		 * without the socket lock. Leave that cork alone.
> +		 */
> +		if (psock->cork == msg) {
>  			cork = true;
>  			psock->cork = NULL;
>  		}

[PATCH bpf v2 14/15] bpf, sockmap: Keep the rest of the cork after a redirect

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

After bpf_msg_apply_bytes() a verdict can apply to a part of the message.
Then tcp_bpf_send_verdict() sends this part and goes to more_data, and the
sk_msg prog runs on the rest. The SK_REDIRECT branch does not go there if
the message is the cork. It removes the cork from the psock before it
releases the socket lock. After the redirect it frees the cork together
with the rest. Thus the rest is lost, and sendmsg() reports it as sent.

If bytes remain after the redirect, put the cork back into the psock. Then
the prog runs on the rest, as after SK_PASS. If the new verdict is SK_DROP
or the push fails, sendmsg() now returns a smaller value or the error.

msg->cork_bytes keeps its value, thus the rest can stay in the cork. If the
psock has a new cork after lock_sock(), the kernel still frees the rest. If
the redirect of a cork fails, sendmsg() still returns success and frees it.

Fixes: 4f738adba30a ("bpf: create tcp_bpf_ulp allowing BPF to monitor socket TX/RX data")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/ipv4/tcp_bpf.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index b1071b0960cf..788ce08eef4d 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -544,6 +544,14 @@ static int tcp_bpf_send_verdict(struct sock *sk, struct sk_psock *psock,
 			if (!cork)
 				*copied -= free;
 		}
+		/*
+		 * The verdict was for tosend bytes. The rest goes back, unless
+		 * another thread made a new cork while the lock was released.
+		 */
+		if (cork && msg->sg.size && !psock->cork) {
+			psock->cork = msg;
+			cork = false;
+		}
 		if (cork) {
 			sk_msg_free(sk, msg);
 			kfree(msg);
-- 
2.55.0

Re: [PATCH bpf v2 14/15] bpf, sockmap: Keep the rest of the cork after a redirect

[Jiayuan Chen]

On 11/2/26 3:21 PM, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@kernel.org>
>
> After bpf_msg_apply_bytes() a verdict can apply to a part of the message.
> Then tcp_bpf_send_verdict() sends this part and goes to more_data, and the
> sk_msg prog runs on the rest. The SK_REDIRECT branch does not go there if
> the message is the cork. It removes the cork from the psock before it
> releases the socket lock. After the redirect it frees the cork together
> with the rest. Thus the rest is lost, and sendmsg() reports it as sent.
>
> If bytes remain after the redirect, put the cork back into the psock. Then
> the prog runs on the rest, as after SK_PASS. If the new verdict is SK_DROP
> or the push fails, sendmsg() now returns a smaller value or the error.
>
> msg->cork_bytes keeps its value, thus the rest can stay in the cork. If the
> psock has a new cork after lock_sock(), the kernel still frees the rest. If
> the redirect of a cork fails, sendmsg() still returns success and frees it.
>
> Fixes: 4f738adba30a ("bpf: create tcp_bpf_ulp allowing BPF to monitor socket TX/RX data")
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>


Reviewed-by: Jiayuan Chen <jiayuan.chen@linux.dev>

Re: [PATCH bpf v2 14/15] bpf, sockmap: Keep the rest of the cork after a redirect

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 (1) · High: 0 · Medium: 1 (3) · Low: 0

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 fix is incomplete: the cork tail is still lost when
  another sender installs a new cork while the lock is dropped.

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but the
  SK_PASS branch of `tcp_bpf_send_verdict()` pushes a message that is…
- [Medium] This problem wasn't introduced by this patch, but the selftest
  `test_txmsg_cork_hangs()` in…
- [Medium] This problem wasn't introduced by this patch, but when
  `tcp_bpf_sendmsg_redir()` fails for a cork in…
- [Medium] This problem wasn't introduced by this patch, but in the
  __SK_REDIRECT branch of `tcp_bpf_send_verdict()`,…

--

bpf, sockmap: Keep the rest of the cork after a redirect

When a verdict covers only part of a cork after bpf_msg_apply_bytes(),
the SK_REDIRECT branch of tcp_bpf_send_verdict() used to free the bytes
that were left over along with the cork. This patch puts the cork back
into psock->cork when bytes remain, unless another thread installed a
new cork, so the sk_msg program runs again on the rest.

> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index b1071b0960cf5..788ce08eef4d7 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -544,6 +544,14 @@ static int tcp_bpf_send_verdict(struct sock *sk, struct sk_psock *psock,
>  			if (!cork)
>  				*copied -= free;
>  		}

[Severity: Medium]
This is a pre-existing issue, but what happens when tcp_bpf_sendmsg_redir()
fails for a cork here?

The ret < 0 path frees the whole sk_msg. It skips the *copied adjustment
because cork is true. The new restore check is then skipped because
msg->sg.size is 0, and the if (cork) block runs:

    sk_msg_free(sk, msg);
    kfree(msg);
    msg = NULL;
    ret = 0;

tcp_bpf_sendmsg() then returns copied > 0 ? copied : err with copied
unchanged. So sendmsg() reports success for bytes that this call put into
the cork and that were then thrown away. A non-cork message with the same
failure lowers *copied and returns the error.

Failures that can get here include -EPIPE when the target has no psock,
-EINVAL/-EPIPE from tcp_bpf_redir_allowed() in tcp_bpf_push(), -ENOMEM
from bpf_tcp_ingress(), and errors from tcp_sendmsg_locked().

The commit message says this stays unchanged ("If the redirect of a cork
fails, sendmsg() still returns success and frees it."). So does the earlier
patch "bpf, sockmap: Check the target of an egress redirect before each
send".

Should the cork case also subtract the freed bytes from *copied and keep
the error?

> +		/*
> +		 * The verdict was for tosend bytes. The rest goes back, unless
> +		 * another thread made a new cork while the lock was released.
> +		 */
> +		if (cork && msg->sg.size && !psock->cork) {

[Severity: Medium]
Is the tail still lost if another sender installs a new cork while the lock
is dropped?

Earlier in the __SK_REDIRECT branch, the cork is detached and the lock is
released:

    if (psock->cork == msg) {
        cork = true;
        psock->cork = NULL;
    }
    release_sock(sk);

A second thread in tcp_bpf_sendmsg() on the same socket then sees
psock->cork == NULL and uses the on-stack tmp. The first thread's call to
sk_msg_apply_bytes(psock, tosend) used up apply_bytes, so psock->eval was
reset to __SK_NONE and the prog runs on tmp. If the prog calls
bpf_msg_cork_bytes() with a size larger than tmp, the top of
tcp_bpf_send_verdict() allocates a new psock->cork.

When the first thread takes the lock again, this check is false. The
if (cork) block then frees the unsent tail with ret = 0 and does not reduce
*copied, and tcp_bpf_sendmsg() returns the positive copied count.

That is the symptom the commit message describes ("the rest is lost, and
sendmsg() reports it as sent"). The prog also never runs on those bytes,
although bpf_msg_apply_bytes() says it should.

The commit message acknowledges this ("If the psock has a new cork after
lock_sock(), the kernel still frees the rest"). The earlier patch "bpf,
sockmap: Check that the redirected message is the cork" also says corks are
not safe for two senders.

Is this case meant to stay out of scope, or should *copied at least be
reduced by the freed tail?

> +			psock->cork = msg;
> +			cork = false;
> +		}

[Severity: Critical]
This isn't a bug introduced by this patch, but putting the tail back here
adds a new way into a race that already exists in the SK_PASS branch. Can
a restored cork be freed while tcp_bpf_push() is still using it?

After this code runs, we reach more_data with psock->cork == msg. If the
next verdict is SK_PASS:

tcp_bpf_send_verdict()
    case __SK_PASS:
        if (!psock->apply_bytes)
            psock->eval = __SK_NONE;
        ...
        ret = tcp_bpf_push(sk, msg, tosend, flags, true);
            tcp_sendmsg_locked()
                sk_stream_wait_memory()
                    sk_wait_event()  <- drops and re-takes the socket lock

The cork stays in psock->cork during the push. A second sender in
tcp_bpf_sendmsg() can take the lock and pick it up:

    if (psock->cork) {
        msg_tx = psock->cork;

psock->cork_bytes is 0 and psock->eval is __SK_NONE, so the second sender
runs the prog on the shared msg:

- On SK_REDIRECT it detaches the cork, redirects it without the lock, and
  later calls kfree(msg).
- On SK_PASS it pushes the same element, then calls put_page() and
  sg_init_table() on it.

The first thread then wakes up in tcp_bpf_push() with stale sge, page, off
and size locals, and runs:

    msg->sg.size -= ret;
    sge->offset += ret;
    sge->length -= ret;
    ...
    if (!sge->length) {
        put_page(page);

Could this cause a use-after-free of the kfree()d sk_msg and a double
put_page(), and send the same bytes twice?

The earlier patch "bpf, sockmap: Check that the redirected message is the
cork" already says "a cork is still not safe for two senders on one
socket".

Would it fix this to detach psock->cork in the SK_PASS branch before any
push that can drop the lock, and attach it again under the lock afterwards,
as the SK_REDIRECT branch does?

[Severity: Medium]
This is a pre-existing issue, but now that the tail goes to more_data, can
some of the restored bytes get a different verdict than the program asked
for?

In the __SK_REDIRECT branch, the whole planned tosend is charged before
the redirect runs:

    sk_msg_apply_bytes(psock, tosend);
    if (!psock->apply_bytes) {
        /* Clean up before releasing the sock lock. */
        eval = psock->eval;
        psock->eval = __SK_NONE;
        psock->sk_redir = NULL;
    }

bpf_tcp_ingress() can move only part of it. Once one element has moved, a
later __sk_rmem_schedule() failure breaks out with ret == 0, because copied
is not zero:

    if (!__sk_rmem_schedule(sk, size, false)) {
        if (!copied)
            ret = -ENOMEM;
        break;
    }

For example, take an 8192-byte cork with apply_bytes 6144 where only the
first 4096-byte element moves. The prog run at more_data then decides all
4096 remaining bytes, including 2048 that the redirect verdict covered.

Non-cork messages already have the same accounting problem. Should
sk_msg_apply_bytes() be charged with the bytes actually sent rather than
tosend?

>  		if (cork) {
>  			sk_msg_free(sk, msg);
>  			kfree(msg);

[Severity: Medium]
This is a pre-existing issue in the selftests, but is there a test that
would catch the data loss this patch fixes?

The third case in test_txmsg_cork_hangs() in
tools/testing/selftests/bpf/test_sockmap.c reaches this path:

    txmsg_redir = 1;
    txmsg_apply = 4097;
    txmsg_cork = 4097;
    test_send_large(opt, cgrp);

With txmsg_cork set, msg_loop() waits only 300ms per select(). An
incomplete receive becomes errno = -EIO. The rx child in sendmsg_test()
then hides that error:

    if (err && err != -EDATAINTEGRITY && txmsg_cork)
        err = 0;

So the test passes both when the cork tail is freed (before this patch)
and when it is put back and sent (after this patch).

Could a test be added that checks how many bytes the peer receives?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102072112.4160195-1-alexei.starovoitov%40gmail.com

[PATCH bpf v2 15/15] bpf, sockmap: Clear the end mark of elements added by push and pop

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

The elements of an sk_msg are in a ring of scatterlist slots. A free slot
can have the end mark, because sk_msg_init() marks the last slot of the
ring. Also, sg_init_table() in tcp_bpf_push() marks each emptied slot. Thus
sk_msg_alloc() and sk_msg_page_add() clear the mark of a new element.

bpf_msg_push_data() and bpf_msg_pop_data() fill a free slot with
sg_set_page(), which keeps the mark. Then the sk_msg can have an end mark
before its last element. bpf_tcp_ingress() copies the mark. Since the
commit below, __sk_msg_recvmsg() stops at the mark and frees the sk_msg.
The data after the mark is lost, and its pages and memory charge leak.

Clear the mark after the two sg_set_page() calls that can fill a free slot.
A free slot keeps its mark, as today. tcp_bpf_push() does not read the
mark, thus SK_PASS and egress redirect do not change.

Fixes: 9974d37ea75f ("skmsg: Fix invalid last sg check in sk_msg_recvmsg()")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/filter.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/filter.c b/net/core/filter.c
index 50c0cda778db..d68b52150405 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2995,6 +2995,8 @@ BPF_CALL_4(bpf_msg_push_data, struct sk_msg *, msg, u32, start,
 	msg->sg.size += len;
 	sk_msg_clear_elem_copy(msg, new);
 	sg_set_page(&msg->sg.data[new], page, len + copy, 0);
+	/* A free slot can have the end mark */
+	sg_unmark_end(&msg->sg.data[new]);
 	if (rsge.length) {
 		get_page(sg_page(&rsge));
 		sk_msg_iter_var_next(new);
@@ -3128,6 +3130,7 @@ BPF_CALL_4(bpf_msg_pop_data, struct sk_msg *, msg, u32, start,
 				sg_set_page(nsge,
 					    sg_page(sge),
 					    b, sge->offset + pop + a);
+				sg_unmark_end(nsge);
 				sk_msg_set_elem_copy(msg, i, sge_copy);
 			} else {
 				struct page *page, *orig;
-- 
2.55.0

Re: [PATCH bpf v2 15/15] bpf, sockmap: Clear the end mark of elements added by push and pop

[Jiayuan Chen]

On 11/2/26 3:21 PM, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@kernel.org>
>
> The elements of an sk_msg are in a ring of scatterlist slots. A free slot
> can have the end mark, because sk_msg_init() marks the last slot of the
> ring. Also, sg_init_table() in tcp_bpf_push() marks each emptied slot. Thus
> sk_msg_alloc() and sk_msg_page_add() clear the mark of a new element.
>
> bpf_msg_push_data() and bpf_msg_pop_data() fill a free slot with
> sg_set_page(), which keeps the mark. Then the sk_msg can have an end mark
> before its last element. bpf_tcp_ingress() copies the mark. Since the
> commit below, __sk_msg_recvmsg() stops at the mark and frees the sk_msg.
> The data after the mark is lost, and its pages and memory charge leak.
>
> Clear the mark after the two sg_set_page() calls that can fill a free slot.
> A free slot keeps its mark, as today. tcp_bpf_push() does not read the
> mark, thus SK_PASS and egress redirect do not change.
>
> Fixes: 9974d37ea75f ("skmsg: Fix invalid last sg check in sk_msg_recvmsg()")
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>


Reviewed-by: Jiayuan Chen <jiayuan.chen@linux.dev>


> ---
>  net/core/filter.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 50c0cda778db..d68b52150405 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2995,6 +2995,8 @@ BPF_CALL_4(bpf_msg_push_data, struct sk_msg *, msg, u32, start,
>  	msg->sg.size += len;
>  	sk_msg_clear_elem_copy(msg, new);
>  	sg_set_page(&msg->sg.data[new], page, len + copy, 0);
> +	/* A free slot can have the end mark */
> +	sg_unmark_end(&msg->sg.data[new]);
>  	if (rsge.length) {
>  		get_page(sg_page(&rsge));
>  		sk_msg_iter_var_next(new);
> @@ -3128,6 +3130,7 @@ BPF_CALL_4(bpf_msg_pop_data, struct sk_msg *, msg, u32, start,
>  				sg_set_page(nsge,
>  					    sg_page(sge),
>  					    b, sge->offset + pop + a);
> +				sg_unmark_end(nsge);
>  				sk_msg_set_elem_copy(msg, i, sge_copy);
>  			} else {
>  				struct page *page, *orig;

Re: [PATCH bpf v2 00/15] bpf, sockmap: Fixes for verdicts, redirects, corks and recvmsg()

[John Fastabend]

On Mon, Nov 02, 2026 at 07:20:57AM +0000, Alexei Starovoitov wrote:
>From: Alexei Starovoitov <ast@kernel.org>
>
>These patches fix bugs in sockmap:
>
>  patches  area
>  -------  ----------------------------------------------------------------
>  01       recvmsg() after a fault
>  02-04    the verdict of an sk_msg prog and the return value of sendmsg()
>  05       the backlog of a stopped psock
>  06       TCP listeners
>  07-08    the target of an egress redirect
>  09       the TCP state in recvmsg()
>  10-14    corks and the redirect target
>  15       bpf_msg_push_data() and bpf_msg_pop_data()
>
>8 of the patches are from Chris Mason and Josef Bacik.

Thanks everyone lgtm. For the series.

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