[PATCH bpf v2 0/2] bpf, sockmap: fix forward allocation accounting in strparser self-pass path

[PATCH bpf v2 0/2] bpf, sockmap: fix forward allocation accounting in strparser self-pass path

[Junseo Lim]

The strparser SK_PASS path can queue cloned skbs back to the same socket.
When one TCP receive skb is split into many strparser messages, repeated
receive-owner transitions can leave sk_forward_alloc in deficit before
the next skb_set_owner_r() charge. Teardown can then uncharge more memcg
pages than were reserved and trigger a page_counter underflow warning.

Fix this by settling any existing sk_forward_alloc deficit with
sk_rmem_schedule(sk, skb, 0) before skb_set_owner_r() for strparser
self-pass skbs. The same handling is used when retrying from the psock
backlog.

Patch 2 adds a sockmap_strp selftest using a one-byte stream parser and
an SK_PASS verdict program. The test checks INET_DIAG_MEMINFO to verify
that sk_forward_alloc does not go negative after exercising the self-pass
delivery path.

Changelog:
v1 -> v2:
- Keep skb_set_owner_r() and use sk_rmem_schedule(sk, skb, 0) to settle
  sk_forward_alloc instead of skipping the owner transition.
  (Emil Tsalapatis)
- Apply the same handling to psock backlog retries.
- Add a sockmap_strp selftest based on the reproducer.
- Add a Reported-by tag.
- Change the Fixes tag to point to the commit that introduced the issue.

v1: https://lore.kernel.org/bpf/20260723065244.186916-1-zirajs7@gmail.com/T/

Junseo Lim (2):
  bpf, sockmap: settle sk_forward_alloc for strparser SK_PASS
  selftests/bpf: Cover strparser self-pass forward allocation

 net/core/skmsg.c                              |  33 ++--
 .../selftests/bpf/prog_tests/sockmap_strp.c   | 171 ++++++++++++++++++
 .../selftests/bpf/progs/test_sockmap_strp.c   |   6 +
 3 files changed, 199 insertions(+), 11 deletions(-)

-- 
2.55.0

[PATCH bpf v2 1/2] bpf, sockmap: settle sk_forward_alloc for strparser SK_PASS

[Junseo Lim]

The strparser SK_PASS path can queue cloned skbs back to the same
socket. A single TCP receive skb may be split into multiple strparser
messages, and each cloned message still carries the receive owner from
the TCP receive path.

sk_psock_skb_ingress_self() reassigns receive ownership with
skb_set_owner_r(). That first orphans the skb, which runs the existing
receive destructor, and then charges the skb to the socket again. When
this is repeated for strparser clones, sk_forward_alloc can already be
in deficit before the next owner transition. Releasing the queued skbs
can then uncharge more memcg pages than were reserved and trigger a
page_counter underflow.

Call sk_rmem_schedule() with a size of zero before skb_set_owner_r()
for strparser self-pass skbs. Use the zero-sized reservation to top up
any existing sk_forward_alloc deficit without reserving the skb's full
truesize again, then let skb_set_owner_r() perform the receive-owner
transition. Apply the same handling when retrying the skb from the psock
backlog.

Fixes: 144748eb0c44 ("bpf, sockmap: Fix incorrect fwd_alloc accounting")
Reported-by: Sechang Lim <rhkrqnwk98@gmail.com>
Suggested-by: Emil Tsalapatis <emil@etsalapatis.com>
Signed-off-by: Junseo Lim <zirajs7@gmail.com>
---
 net/core/skmsg.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index 2521b643fa05..ce5ad8160282 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -586,7 +586,8 @@ static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb,
 }
 
 static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff *skb,
-				     u32 off, u32 len, bool take_ref);
+				     u32 off, u32 len, bool take_ref,
+				     bool settle_fwd_alloc);
 
 static int sk_psock_skb_ingress(struct sk_psock *psock, struct sk_buff *skb,
 				u32 off, u32 len)
@@ -595,12 +596,9 @@ static int sk_psock_skb_ingress(struct sk_psock *psock, struct sk_buff *skb,
 	struct sk_msg *msg;
 	int err;
 
-	/* If we are receiving on the same sock skb->sk is already assigned,
-	 * skip memory accounting and owner transition seeing it already set
-	 * correctly.
-	 */
 	if (unlikely(skb->sk == sk))
-		return sk_psock_skb_ingress_self(psock, skb, off, len, true);
+		return sk_psock_skb_ingress_self(psock, skb, off, len, true,
+						 skb_bpf_strparser(skb));
 	msg = sk_psock_create_ingress_msg(sk, skb);
 	if (!msg)
 		return -EAGAIN;
@@ -618,12 +616,14 @@ static int sk_psock_skb_ingress(struct sk_psock *psock, struct sk_buff *skb,
 	return err;
 }
 
-/* Puts an skb on the ingress queue of the socket already assigned to the
- * skb. In this case we do not need to check memory limits or skb_set_owner_r
- * because the skb is already accounted for here.
+/* Puts an skb on the ingress queue for psock->sk.
+ *
+ * Before assigning receive ownership to a direct strparser SK_PASS clone,
+ * settle any existing sk_forward_alloc deficit from earlier clone charges.
  */
 static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff *skb,
-				     u32 off, u32 len, bool take_ref)
+				     u32 off, u32 len, bool take_ref,
+				     bool settle_fwd_alloc)
 {
 	struct sk_msg *msg = alloc_sk_msg(GFP_ATOMIC);
 	struct sock *sk = psock->sk;
@@ -631,6 +631,13 @@ static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff *skb
 
 	if (unlikely(!msg))
 		return -EAGAIN;
+
+	if (settle_fwd_alloc &&
+	    !sk_rmem_schedule(sk, skb, 0)) {
+		kfree(msg);
+		return -EAGAIN;
+	}
+
 	skb_set_owner_r(skb, sk);
 
 	/* This is used in tcp_bpf_recvmsg_parser() to determine whether the
@@ -1017,6 +1024,8 @@ static int sk_psock_verdict_apply(struct sk_psock *psock, struct sk_buff *skb,
 		 * retrying later from workqueue.
 		 */
 		if (skb_queue_empty(&psock->ingress_skb)) {
+			bool settle_fwd_alloc = false;
+
 			len = skb->len;
 			off = 0;
 			if (skb_bpf_strparser(skb)) {
@@ -1024,8 +1033,10 @@ static int sk_psock_verdict_apply(struct sk_psock *psock, struct sk_buff *skb,
 
 				off = stm->offset;
 				len = stm->full_len;
+				settle_fwd_alloc = true;
 			}
-			err = sk_psock_skb_ingress_self(psock, skb, off, len, false);
+			err = sk_psock_skb_ingress_self(psock, skb, off, len,
+							false, settle_fwd_alloc);
 		}
 		if (err < 0) {
 			spin_lock_bh(&psock->ingress_lock);
-- 
2.55.0

[PATCH bpf v2 2/2] selftests/bpf: Cover strparser self-pass forward allocation

[Junseo Lim]

Add a sockmap_strp regression test for strparser SK_PASS delivery to the
same socket. A one-byte stream parser splits a single write into many
messages, repeatedly exercising receive ownership transitions while the
skbs remain queued.

Verify through INET_DIAG_MEMINFO that sk_forward_alloc does not become
negative after the self-pass path is exercised.

Signed-off-by: Junseo Lim <zirajs7@gmail.com>
---
 .../selftests/bpf/prog_tests/sockmap_strp.c   | 171 ++++++++++++++++++
 .../selftests/bpf/progs/test_sockmap_strp.c   |   6 +
 2 files changed, 177 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_strp.c b/tools/testing/selftests/bpf/prog_tests/sockmap_strp.c
index 1d7231728eaf..c7ad21d0bbf4 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_strp.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_strp.c
@@ -1,5 +1,9 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <error.h>
+#include <linux/inet_diag.h>
+#include <linux/netlink.h>
+#include <linux/rtnetlink.h>
+#include <linux/sock_diag.h>
 #include <netinet/tcp.h>
 #include <test_progs.h>
 #include "sockmap_helpers.h"
@@ -460,6 +464,171 @@ static void test_sockmap_strp_parser_reject(void)
 	test_sockmap_strp__destroy(strp);
 }
 
+/* Read sk_forward_alloc through inet_diag meminfo. */
+static int sockmap_strp_get_fwd_alloc(int sock, int *fwd_alloc)
+{
+	struct sockaddr_storage local = {}, peer = {};
+	struct sockaddr_in *local_in, *peer_in;
+	socklen_t addr_len = sizeof(local);
+	char buf[1024];
+	struct {
+		struct nlmsghdr nlh;
+		struct inet_diag_req_v2 req;
+	} req = {
+		.nlh = {
+			.nlmsg_len = sizeof(req),
+			.nlmsg_type = SOCK_DIAG_BY_FAMILY,
+			.nlmsg_flags = NLM_F_REQUEST,
+			.nlmsg_seq = 1,
+		},
+		.req = {
+			.sdiag_family = AF_INET,
+			.sdiag_protocol = IPPROTO_TCP,
+			.idiag_ext = 1 << (INET_DIAG_MEMINFO - 1),
+			.idiag_states = ~0U,
+			.id.idiag_cookie = {
+				INET_DIAG_NOCOOKIE,
+				INET_DIAG_NOCOOKIE,
+			},
+		},
+	};
+	int diag_fd, ret, err = -ENOENT;
+
+	if (getsockname(sock, (struct sockaddr *)&local, &addr_len))
+		return -errno;
+	addr_len = sizeof(peer);
+	if (getpeername(sock, (struct sockaddr *)&peer, &addr_len))
+		return -errno;
+
+	local_in = (struct sockaddr_in *)&local;
+	peer_in = (struct sockaddr_in *)&peer;
+	req.req.id.idiag_sport = local_in->sin_port;
+	req.req.id.idiag_dport = peer_in->sin_port;
+	req.req.id.idiag_src[0] = local_in->sin_addr.s_addr;
+	req.req.id.idiag_dst[0] = peer_in->sin_addr.s_addr;
+
+	diag_fd = socket(AF_NETLINK, SOCK_RAW | SOCK_CLOEXEC,
+			 NETLINK_SOCK_DIAG);
+	if (diag_fd < 0)
+		return -errno;
+
+	ret = send(diag_fd, &req, sizeof(req), 0);
+	if (ret < 0) {
+		err = -errno;
+		goto out;
+	}
+	if (ret != sizeof(req)) {
+		err = -EIO;
+		goto out;
+	}
+
+	ret = recv(diag_fd, buf, sizeof(buf), 0);
+	if (ret < 0) {
+		err = -errno;
+		goto out;
+	}
+
+	for (struct nlmsghdr *nlh = (struct nlmsghdr *)buf;
+	     NLMSG_OK(nlh, ret); nlh = NLMSG_NEXT(nlh, ret)) {
+		struct inet_diag_msg *msg = NLMSG_DATA(nlh);
+		struct rtattr *attr;
+		int len;
+
+		if (nlh->nlmsg_type == NLMSG_ERROR) {
+			err = -EINVAL;
+			goto out;
+		}
+		if (nlh->nlmsg_type == NLMSG_DONE)
+			break;
+
+		len = nlh->nlmsg_len - NLMSG_LENGTH(sizeof(*msg));
+		for (attr = (struct rtattr *)(msg + 1); RTA_OK(attr, len);
+		     attr = RTA_NEXT(attr, len)) {
+			struct inet_diag_meminfo *minfo;
+
+			if (attr->rta_type != INET_DIAG_MEMINFO)
+				continue;
+			minfo = RTA_DATA(attr);
+			*fwd_alloc = (__s32)minfo->idiag_fmem;
+			err = 0;
+			goto out;
+		}
+	}
+
+out:
+	close(diag_fd);
+	return err;
+}
+
+/* Test strparser SK_PASS delivery to the same socket. */
+static void test_sockmap_strp_self_pass_fwd_alloc(void)
+{
+	struct test_sockmap_strp *strp = NULL;
+	char snd[4 * 1024];
+	int c = -1, p = -1;
+	int fwd_alloc;
+	int sndbuf = sizeof(snd);
+	int zero = 0;
+	char rcv;
+	int sent, recvd;
+	int map;
+	int err;
+
+	memset(snd, 0xa5, sizeof(snd));
+
+	strp = test_sockmap_strp__open_and_load();
+	if (!ASSERT_OK_PTR(strp, "test_sockmap_strp__open_and_load"))
+		return;
+
+	map = bpf_map__fd(strp->maps.sock_map);
+	err = xbpf_prog_attach(bpf_program__fd(strp->progs.prog_skb_parser_one),
+			       map, BPF_SK_SKB_STREAM_PARSER, 0);
+	if (err)
+		goto out_destroy;
+
+	err = xbpf_prog_attach(bpf_program__fd(strp->progs.prog_skb_verdict_pass),
+			       map, BPF_SK_SKB_STREAM_VERDICT, 0);
+	if (err)
+		goto out_destroy;
+
+	err = create_pair(AF_INET, SOCK_STREAM, &c, &p);
+	if (!ASSERT_OK(err, "create_pair"))
+		goto out_destroy;
+
+	err = xsetsockopt(c, SOL_SOCKET, SO_SNDBUF, &sndbuf, sizeof(sndbuf));
+	if (err)
+		goto out_destroy;
+
+	err = xsetsockopt(p, SOL_SOCKET, SO_RCVBUF, &sndbuf, sizeof(sndbuf));
+	if (err)
+		goto out_destroy;
+
+	err = xbpf_map_update_elem(map, &zero, &p, BPF_NOEXIST);
+	if (err)
+		goto out_destroy;
+
+	sent = send(c, snd, sizeof(snd), MSG_DONTWAIT);
+	if (!ASSERT_EQ(sent, sizeof(snd), "send"))
+		goto out_destroy;
+
+	recvd = recv_timeout(p, &rcv, sizeof(rcv), MSG_DONTWAIT,
+			     IO_TIMEOUT_SEC);
+	if (!ASSERT_EQ(recvd, sizeof(rcv), "recv_timeout") ||
+	    !ASSERT_EQ(rcv, snd[0], "data mismatch"))
+		goto out_destroy;
+
+	err = sockmap_strp_get_fwd_alloc(p, &fwd_alloc);
+	if (!ASSERT_OK(err, "sockmap_strp_get_fwd_alloc") ||
+	    !ASSERT_GE(fwd_alloc, 0, "sk_forward_alloc"))
+		goto out_destroy;
+out_destroy:
+	test_sockmap_strp__destroy(strp);
+	if (c >= 0)
+		close(c);
+	if (p >= 0)
+		close(p);
+}
+
 void test_sockmap_strp(void)
 {
 	if (test__start_subtest("sockmap strp tcp pass"))
@@ -482,4 +651,6 @@ void test_sockmap_strp(void)
 		test_sockmap_strp_dispatch_pkt(AF_INET, SOCK_STREAM);
 	if (test__start_subtest("sockmap strp parser reject pkt mod"))
 		test_sockmap_strp_parser_reject();
+	if (test__start_subtest("sockmap strp self pass fwd alloc"))
+		test_sockmap_strp_self_pass_fwd_alloc();
 }
diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_strp.c b/tools/testing/selftests/bpf/progs/test_sockmap_strp.c
index fe88fa6d40bc..682e53d40751 100644
--- a/tools/testing/selftests/bpf/progs/test_sockmap_strp.c
+++ b/tools/testing/selftests/bpf/progs/test_sockmap_strp.c
@@ -50,6 +50,12 @@ int prog_skb_parser_partial(struct __sk_buff *skb)
 	return 10;
 }
 
+SEC("sk_skb/stream_parser")
+int prog_skb_parser_one(struct __sk_buff *skb)
+{
+	return 1;
+}
+
 SEC("sk_skb/stream_parser")
 int prog_skb_parser_resize(struct __sk_buff *skb)
 {
-- 
2.55.0