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

[PATCH bpf 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.

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                | 85 +++++++++++++++++++++++++++----
 net/mptcp/protocol.c              |  6 ---
 9 files changed, 133 insertions(+), 32 deletions(-)


base-commit: 5b96632457e4ccae88ecb751cf48176b2e3629a7
-- 
2.55.0

[PATCH bpf 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

[PATCH bpf 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

[PATCH bpf 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.

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, 6 insertions(+)

diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index d90bcc911b36..072ffd8347a6 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -457,8 +457,14 @@ 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);
 			*copied -= sk_msg_free(sk, msg);
 			break;
 		}
-- 
2.55.0

[PATCH bpf 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: 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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index 072ffd8347a6..b9761af13989 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -513,12 +513,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

[PATCH bpf 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

[PATCH bpf 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 b9761af13989..aa904ded816f 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -777,7 +777,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

[PATCH bpf 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 aa904ded816f..b635d3ebf4fb 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)
@@ -547,7 +569,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

[PATCH bpf 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

[PATCH bpf 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 b635d3ebf4fb..9848c8e5a2cd 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

[PATCH bpf 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 303c0c44dc2c..e187e3c27999 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 9848c8e5a2cd..d24ebcd7633b 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

[PATCH bpf 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 d24ebcd7633b..a450b1ff3cfb 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -611,8 +611,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

[PATCH bpf 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

[PATCH bpf 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 a450b1ff3cfb..9bdcbb0a5998 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -509,7 +509,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

[PATCH bpf 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 9bdcbb0a5998..a9af6be2aadb 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -536,6 +536,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

[PATCH bpf 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 e187e3c27999..d7b364e43b4f 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 04/15] bpf, sockmap: Subtract all dropped bytes from the return value

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 2387 bytes --]

> bpf, sockmap: Subtract all dropped bytes from the return value

TL;DR: Commit message only: the Fixes: tag may point at the wrong commit; the
freed/subtracted byte mismatch seems to date from ca70b8baf2bd, not
4f738adba30a.

> bpf, sockmap: Subtract all dropped bytes from the return value
>
> 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: 4f738adba30a ("bpf: create tcp_bpf_ulp allowing BPF to monitor socket TX/RX data")
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>

Is the Fixes: tag pointing at the right commit?

In 4f738adba30a (kernel/bpf/sockmap.c, bpf_exec_tx_verdict()), the
__SK_DROP case freed only 'send' bytes with free_bytes_sg(sk, send, m) and
subtracted 'send' from *copied, so the freed and subtracted byte counts
matched.

The same held after the move to net/ipv4/tcp_bpf.c in 604326b41a6f, where
the case did sk_msg_free_partial(sk, msg, tosend) and then
*copied -= tosend (later tosend + delta).

The mismatch appears with ca70b8baf2bd ("tcp_bpf: Fix the sk_mem_uncharge
logic in tcp_bpf_sendmsg"), which replaced sk_msg_free_partial(sk, msg,
tosend) with sk_msg_free(sk, msg) in the __SK_DROP case while leaving
'*copied -= (tosend + delta)' untouched.  That is the point where the whole
message is freed but only tosend (possibly reduced by apply_bytes) is
subtracted from the sendmsg() return value.

ca70b8baf2bd is an upstream commit from October 2024 (v6.12) that predates
this series.  Should the tag be:

  Fixes: ca70b8baf2bd ("tcp_bpf: Fix the sk_mem_uncharge logic in tcp_bpf_sendmsg")


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/41341848185

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

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 2740 bytes --]

> bpf, sockmap: Don't let an SK_DROP verdict stick

TL;DR: Resetting eval after an SK_DROP verdict may make sendmsg() return a
positive byte count instead of -EACCES for every dropped message; the fix is a
later patch, so should they be applied together?

> 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;
>  	}

Now that an exhausted SK_DROP verdict resets psock->eval, the prog runs again
for each later message.  Does that make an older accounting problem in this
branch happen on every message instead of once per socket?

sk_msg_free(sk, msg) frees the whole message, but only tosend is subtracted
from *copied, and tosend is capped to psock->apply_bytes:

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

Take a prog that calls bpf_msg_apply_bytes(msg, A) and returns SK_DROP, and a
sender whose sendmsg() calls each carry S > A bytes in one sk_msg.

Before this patch:

- call 1 ran the prog (apply_bytes = A), set tosend = A and returned S - A;
- eval then stayed __SK_DROP with apply_bytes = 0, so calls 2..N used
  tosend = S, *copied went to 0, and tcp_bpf_sendmsg() returned -EACCES.

After this patch, eval is reset once apply_bytes reaches 0.  Every later call
runs sk_psock_msg_verdict() again, which sets psock->apply_bytes =
msg->apply_bytes = A, so tosend = A again.  Every sendmsg() then returns
S - A > 0 even though all S bytes were freed and none were sent.  Userspace
sees partial success on each message instead of -EACCES.

The subtraction itself predates this patch.  The sticky verdict hid it after
the first message, and this change exposes it on every message.  A backport
of this commit alone would change sendmsg() return values from -EACCES to
positive byte counts for such progs.

A later patch in this series, f54ec6a22738 ("bpf, sockmap: Subtract all
dropped bytes from the return value"), fixes this by subtracting the return
value of sk_msg_free() plus delta instead of tosend + delta.

Should the two patches be applied (and backported) together?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/41341848185

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: corrupt verdict state after tcp_bpf_push() in tcp_bpf_send_verdict()
--

commit cb1b40f55f45104a6d23bf64e86e3c0a379b2459
Author: Alexei Starovoitov <ast@kernel.org>

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

This commit resets psock->eval before calling tcp_bpf_push() when
psock->apply_bytes is 0, and subtracts any sent bytes from
psock->apply_bytes if tcp_bpf_push() fails.

> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index d90bcc911b36b..072ffd8347a6f 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -457,8 +457,14 @@ 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);
>  			*copied -= sk_msg_free(sk, msg);
>  			break;
>  		}

[Severity: High]
Can this corrupt a concurrent sender's verdict state or leave psock->eval
set when psock->apply_bytes reaches 0?

In tcp_bpf_send_verdict() under case __SK_PASS, if Thread A enters with
psock->apply_bytes == 0, it resets psock->eval = __SK_NONE before calling
tcp_bpf_push(). If tcp_bpf_push() -> tcp_sendmsg_locked() releases the
socket lock in sk_stream_wait_memory(), a concurrent Thread B in
tcp_bpf_sendmsg() -> tcp_bpf_send_verdict() can see psock->eval == __SK_NONE,
run sk_psock_msg_verdict(), and install a new psock->eval (including
__SK_REDIRECT with psock->sk_redir) and psock->apply_bytes.

When Thread A returns from tcp_bpf_push(), it still calls
sk_msg_apply_bytes() on both the error and success paths, and on success runs
the post-switch cleanup:

net/ipv4/tcp_bpf.c:tcp_bpf_send_verdict() {
	...
	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);
			*copied -= sk_msg_free(sk, msg);
			break;
		}
		sk_msg_apply_bytes(psock, tosend);
		break;
	...
	if (likely(!ret)) {
		if (!psock->apply_bytes) {
			psock->eval =  __SK_NONE;
			if (psock->sk_redir) {
				sock_put(psock->sk_redir);
				psock->sk_redir = NULL;
			}
		}
}

Wouldn't Thread A subtract its own sent bytes from Thread B's
psock->apply_bytes, and if psock->apply_bytes is 0 when ret == 0, reset
Thread B's psock->eval to __SK_NONE and drop Thread B's psock->sk_redir?

Also, on the error path (unlikely(ret)), if sk_msg_apply_bytes(psock,
origsize - msg->sg.size) reduces a non-zero psock->apply_bytes to 0 (for
instance if another sender consumed part of psock->apply_bytes while
tcp_bpf_push() slept), the post-switch block is skipped because ret != 0.
Wouldn't that leave psock->eval as __SK_PASS with psock->apply_bytes == 0,
causing the next message to bypass the sk_msg program?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101192057.2296489-1-alexei.starovoitov@gmail.com?part=3

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

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 4177 bytes --]

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

TL;DR: In tcp_bpf_send_verdict(), if a failed push races with another sender,
the new apply_bytes subtraction may reach 0 with eval still set, so the next
message could skip the BPF program.

> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index d90bcc911b36..072ffd8347a6 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -457,8 +457,14 @@ 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);
>  			*copied -= sk_msg_free(sk, msg);
>  			break;

Can this subtraction bring psock->apply_bytes down to 0 while psock->eval is
still __SK_PASS?

The break skips the tail of tcp_bpf_send_verdict(), which is the only place
the __SK_PASS path resets eval once apply_bytes reaches 0:

    if (likely(!ret)) {
        if (!psock->apply_bytes) {
            psock->eval =  __SK_NONE;
            ...

With a single sender this looks fine, since a failed tcp_bpf_push() sends
fewer than tosend bytes and tosend <= apply_bytes.

However, tcp_bpf_push() can drop the socket lock in sk_stream_wait_memory(),
and eval is deliberately left at __SK_PASS during the push when apply_bytes
is not 0.  Another sender can lower psock->apply_bytes in the meantime, so
origsize - msg->sg.size is subtracted from a stale value and
sk_msg_apply_bytes() clamps the result at 0.

Example with a prog that calls bpf_msg_apply_bytes(msg, 1000) and returns
SK_PASS:

  Thread A: sendmsg(4000), prog -> SK_PASS, apply_bytes = 1000, tosend = 1000
            eval stays __SK_PASS (apply_bytes != 0)
            tcp_bpf_push() queues 600 bytes, then blocks in
            sk_stream_wait_memory() and drops the socket lock
  Thread B: sendmsg(500), eval == __SK_PASS so the prog is not run
            push succeeds, sk_msg_apply_bytes(psock, 500) -> apply_bytes = 500
  Thread A: wakes up with a signal or SO_SNDTIMEO expiry, push fails
            sk_msg_apply_bytes(psock, 600) -> apply_bytes = 0 (clamped)
            break, eval is still __SK_PASS
  Next sendmsg(64K): eval == __SK_PASS, so the prog is not run
            apply_bytes == 0, so tosend = msg->sg.size
            the whole sk_msg is pushed without a verdict from the prog

Before this patch the failure path did not touch apply_bytes, so it stayed at
500 after the same interleaving.  Only 500 bytes of the next message escaped
the prog, then the tail reset eval and goto more_data ran the prog on the
rest.  With this patch the whole next sk_msg escapes, which is the situation
the commit message says it is meant to prevent.

The new early reset also seems to open a second way in:

    if (!psock->apply_bytes)
        psock->eval = __SK_NONE;

While A sleeps, B sees eval == __SK_NONE, runs the prog, and installs its own
verdict V (pass, redirect or drop) with apply_bytes = M.  If A's push then
fails, A subtracts its sent bytes from B's apply_bytes.  If that reaches 0, V
is left in psock->eval with apply_bytes == 0, and the next message is passed,
redirected (psock->sk_redir is still held) or dropped whole without the prog
running.

On A's success path, the existing sk_msg_apply_bytes(psock, tosend) and the
tail reset likewise cut B's verdict short by A's bytes, which B's verdict
never covered.

Should the failure path reset psock->eval (and clean up psock->sk_redir as
the tail does) when apply_bytes ends up 0?  Or could it skip the subtraction
when this thread already gave up the verdict before the push?

No later patch in this series changes the __SK_PASS failure path.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/41341848185

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] bpf, sockmap: race in tcp_bpf_clone() skips resetting psock callbacks
- [High] bpf, sockmap: listener keeps psock sk_data_ready after tcp_disconnect()
- [High] bpf, sockmap: sock_map_del_link() stops active progs after goto no_progs
- [Medium] bpf, sockmap: TOCTOU race on sk->sk_state in sock_map_link()
--

commit 839857a435a21d698e4cce3b7a94e6c18f688106
Author: Chris Mason <mason@kernel.org>

bpf, sockmap: Give no progs to a TCP listener

This commit skips attaching sockmap BPF programs to TCP sockets in the
TCP_LISTEN state during sock_map_link() so listeners keep their default
callbacks, and resets sk_data_ready and sk_write_space on cloned child
sockets in tcp_bpf_clone().

> 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]
What happens in sock_map_del_link() when a socket linked into a map via
this no_progs path is later removed from that map while also present in
another map that did attach programs?

Because goto no_progs leaves stream_verdict, stream_parser, skb_verdict,
and msg_parser NULL, sock_map_link() skips the -EBUSY conflict check and
still adds a link to the map on psock->link. If a socket inherits active
programs from Map A while in TCP_ESTABLISHED and is also linked into Map B
(which has its own programs) while in TCP_LISTEN, deleting the socket from
Map B calls sock_map_unref() -> sock_map_del_link():

net/core/sock_map.c:sock_map_del_link() {
    ...
			if (progs->stream_parser)
				strp_stop = true;
			if (progs->stream_verdict)
				verdict_stop = true;
			if (progs->skb_verdict)
				verdict_stop = true;
    ...
		if (strp_stop)
			sk_psock_stop_strp(sk, psock);
		if (verdict_stop)
			sk_psock_stop_verdict(sk, psock);
    ...
}

Because sock_map_del_link() checks the unlinked map's progs rather than
whether that map actually installed programs on psock, won't removing the
socket from Map B stop Map A's active verdict or parser callbacks (and
potentially call sk_psock_stop_strp() on an uninitialized strparser if
Map B had a stream_parser while Map A only had a verdict program)?

[Severity: Medium]
Could sk->sk_state change between sock_map_sk_state_allowed() and this
check in sock_map_link()?

In sock_map_update_elem() (reachable from BPF iterator, reuseport, and
sk_lookup programs), only bh_lock_sock(sk) is held without checking
sock_owned_by_user(sk):

net/core/sock_map.c:sock_map_update_elem() {
    ...
	bh_lock_sock(sk);
	if (!sock_map_sk_state_allowed(sk))
		ret = -EOPNOTSUPP;
	else if (map->map_type == BPF_MAP_TYPE_SOCKMAP)
		ret = sock_map_update_common(map, *(u32 *)key, sk, flags);
    ...
}

If a process-context caller holds lock_sock(sk) (where sk->sk_lock.owned
is set and sk->sk_lock.slock is released), sk->sk_state can be observed as
TCP_LISTEN by sock_map_sk_state_allowed() and then transition to TCP_CLOSE
via tcp_disconnect() or the error rollback in inet_csk_listen_start()
before sock_map_link() tests sk->sk_state == TCP_LISTEN.

Wouldn't sock_map_link() then see TCP_CLOSE instead of TCP_LISTEN, skip
the goto no_progs branch, and attach the map's programs and callbacks to a
socket that can subsequently re-enter TCP_LISTEN?

> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index b9761af139892..aa904ded816f8 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -777,7 +777,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: Critical]
Is gating the callback reset on is_insidevar(prot, tcp_bpf_prots) safe
against a concurrent sk_psock_drop() on the listening socket?

When an incoming connection clones the listener via
tcp_create_openreq_child() -> inet_csk_clone_lock() -> sk_clone_lock() ->
sock_copy(), neither the listener's socket lock nor sk_callback_lock is
held. In sock_copy(), sk_prot (before sk_dontcopy_begin) is copied in the
first memcpy(), while sk_data_ready and sk_write_space (after
sk_dontcopy_end) are copied in the second memcpy():

net/core/sock.c:sock_copy() {
    ...
	memcpy(nsk, osk, offsetof(struct sock, sk_dontcopy_begin));

	unsafe_memcpy(&nsk->sk_dontcopy_end, &osk->sk_dontcopy_end,
		      prot->obj_size - offsetof(struct sock, sk_dontcopy_end),
		      /* alloc is larger than struct, see sk_prot_alloc() */);
    ...
}

Concurrently, if the listener is removed from the sockmap, sk_psock_drop()
restores sk->sk_prot before restoring sk->sk_data_ready and
sk->sk_write_space:

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

If sock_copy() copies the already-restored sk_prot alongside the
not-yet-restored sk_data_ready or sk_write_space, won't
is_insidevar(prot, tcp_bpf_prots) evaluate to false in tcp_bpf_clone(),
leaving the child socket with the psock callbacks and allowing the same
unbounded self-recursion if the child is later added to a sockmap?

> +
> +	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: High]
In the scenario described by this comment—where a socket got psock
programs and callbacks in TCP_ESTABLISHED, disconnected via
tcp_disconnect(), and then entered TCP_LISTEN via inet_listen()—doesn't
the listening socket itself still have sk_psock_verdict_data_ready or
sk_psock_strp_data_ready installed as sk->sk_data_ready?

While tcp_bpf_clone() restores the callbacks on newsk, the parent listener
sk retains the psock sk_data_ready callback. When a child connection
completes the handshake in tcp_child_process(), the stack notifies the
parent listener without holding the parent's socket lock:

net/ipv4/tcp_minisocks.c:tcp_child_process() {
    ...
		/* Wakeup parent, send SIGIO */
		if (state == TCP_SYN_RECV && child->sk_state != state)
			READ_ONCE(parent->sk_data_ready)(parent);
    ...
}

If parent->sk_data_ready is sk_psock_verdict_data_ready(), tcp_read_skb()
sees TCP_LISTEN and returns -ENOTCONN without calling
psock->saved_data_ready, so accept() is never woken. If
parent->sk_data_ready is sk_psock_strp_data_ready(), it also fails to wake
accept() and calls strp_data_ready(&psock->strp) without holding parent's
socket lock, violating sk_psock_strp_data_ready()'s locking requirement.

How does a socket that transitions from TCP_ESTABLISHED to TCP_LISTEN while
remaining in a sockmap wake up accept() when new connections arrive?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101192057.2296489-1-alexei.starovoitov@gmail.com?part=6

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

[MPTCP CI]

Hi Alexei,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join): Success! ✅
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Success! ✅
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Unstable: 3 failed test(s): bpftest_test_progs-cpuv4_mptcp bpftest_test_progs-no_alu32_mptcp bpftest_test_progs_mptcp ⚠️ 
- KVM Validation: btf-debug (only bpftest_all): Unstable: 3 failed test(s): bpftest_test_progs-cpuv4_mptcp bpftest_test_progs-no_alu32_mptcp bpftest_test_progs_mptcp ⚠️ 
- Perf: Unstable: 1 failed test(s): error:sched-mem-pressure ⚠️ 
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/41343383116

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/072b58d52e7a
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1198251


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

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

[Alexei Starovoitov]

On Sun, Nov 01, 2026 at 11:39 AM bot+bpf-ci@kernel.org <bot+bpf-ci@kernel.org> wrote:
> Now that an exhausted SK_DROP verdict resets psock->eval, the prog runs again
> for each later message.  Does that make an older accounting problem in this
> branch happen on every message instead of once per socket?

Only between patches 2 and 4.
The wrong return value is an old bug. Call 1 hits it without this patch.
Calls 2..N return -EACCES today only because the prog doesn't run again.
That's the bug that this patch fixes.

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

[Alexei Starovoitov]

On Sun, Nov 01, 2026 at 11:42 AM sashiko-bot@kernel.org <sashiko-bot@kernel.org> wrote:
>>  		if (unlikely(ret)) {
>> +			/* A part of it may have gone out */
>> +			sk_msg_apply_bytes(psock, origsize - msg->sg.size);
>>  			*copied -= sk_msg_free(sk, msg);
>>  			break;
>>  		}
>
> Wouldn't Thread A subtract its own sent bytes from Thread B's
> psock->apply_bytes, and if psock->apply_bytes is 0 when ret == 0, reset
> Thread B's psock->eval to __SK_NONE and drop Thread B's psock->sk_redir?

Not a bug. eval, apply_bytes and sk_redir belong to the socket, not to
a thread. B's verdict ends early and the prog runs again for the rest.
B sends with its own reference to sk_redir.

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

[Alexei Starovoitov]

On Sun, Nov 01, 2026 at 11:51 AM sashiko-bot@kernel.org <sashiko-bot@kernel.org> wrote:
>> +	if (sk_is_tcp(sk) && sk->sk_state == TCP_LISTEN)
>> +		goto no_progs;
>
> Because sock_map_del_link() checks the unlinked map's progs rather than
> whether that map actually installed programs on psock, won't removing the
> socket from Map B stop Map A's active verdict or parser callbacks (and
> potentially call sk_psock_stop_strp() on an uninitialized strparser if
> Map B had a stream_parser while Map A only had a verdict program)?

Not from this patch. sock_map_del_link() looks at the progs that the map
has at the time of the delete. Add a socket to map B, attach a prog to
map B, delete the socket. That stops the progs of map A today.
strp_stop() only sets a flag, and sk_psock_done_strp() checks
SK_PSOCK_RX_STRP_ENABLED.

> Wouldn't sock_map_link() then see TCP_CLOSE instead of TCP_LISTEN, skip
> the goto no_progs branch, and attach the map's programs and callbacks to a
> socket that can subsequently re-enter TCP_LISTEN?

It doesn't matter. connect(), connect(AF_UNSPEC), listen() gets to the
same state without a race. The tcp_bpf_clone() hunk is for such sockets.

>> +	if (!is_insidevar(prot, tcp_bpf_prots))
>> +		return;
>
> If sock_copy() copies the already-restored sk_prot alongside the
> not-yet-restored sk_data_ready or sk_write_space, won't
> is_insidevar(prot, tcp_bpf_prots) evaluate to false in tcp_bpf_clone(),
> leaving the child socket with the psock callbacks and allowing the same
> unbounded self-recursion if the child is later added to a sockmap?

The window is there, but it's tiny and it's not from this patch.

> How does a socket that transitions from TCP_ESTABLISHED to TCP_LISTEN while
> remaining in a sockmap wake up accept() when new connections arrive?

It doesn't. It's an old bug, not from this patch.