* [PATCH bpf v3 0/2] bpf, sockmap: fix forward allocation accounting in strparser self-pass path
@ 2026-08-17 15:50 Junseo Lim
2026-08-17 15:50 ` [PATCH bpf v3 1/2] bpf, sockmap: settle sk_forward_alloc for strparser SK_PASS Junseo Lim
2026-08-17 15:50 ` [PATCH bpf v3 2/2] selftests/bpf: Cover strparser self-pass forward allocation Junseo Lim
0 siblings, 2 replies; 6+ messages in thread
From: Junseo Lim @ 2026-08-17 15:50 UTC (permalink / raw)
To: John Fastabend, Jakub Sitnicki, Jiayuan Chen
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Andrii Nakryiko, Eduard Zingerman, linux-kernel,
bpf, netdev, Sechang Lim, Daniel Borkmann, Emil Tsalapatis
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 assignments for unowned clones 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 by avoiding another receive-owner transition for same-socket skbs
that are already receive-owned by the socket. For unowned strparser
self-pass skbs, settle any existing sk_forward_alloc deficit with
sk_rmem_schedule(sk, skb, 0) before skb_set_owner_r().
Patch 1 also fixes psock backlog retries by restoring the original skb
redirect metadata after skb_bpf_redirect_clear(). If a deferred strparser
self-pass skb needs forward-allocation settlement, that work is done under
the socket lock.
The selftest adds a sockmap_strp case 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:
v2 -> v3:
- Do not call skb_set_owner_r() again for already receive-owned same-socket
skbs.
- Preserve the original _sk_redir value across psock backlog retries.
- Add a backlog-specific self-pass path so deferred strparser
forward-allocation settlement runs under the socket lock.
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/
v2: https://lore.kernel.org/bpf/20260801102633.1872012-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 | 125 +++++++++++--
.../selftests/bpf/prog_tests/sockmap_strp.c | 171 ++++++++++++++++++
.../selftests/bpf/progs/test_sockmap_strp.c | 6 +
3 files changed, 282 insertions(+), 20 deletions(-)
--
2.55.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH bpf v3 1/2] bpf, sockmap: settle sk_forward_alloc for strparser SK_PASS
2026-08-17 15:50 [PATCH bpf v3 0/2] bpf, sockmap: fix forward allocation accounting in strparser self-pass path Junseo Lim
@ 2026-08-17 15:50 ` Junseo Lim
2026-08-17 17:13 ` bot+bpf-ci
2026-08-17 15:50 ` [PATCH bpf v3 2/2] selftests/bpf: Cover strparser self-pass forward allocation Junseo Lim
1 sibling, 1 reply; 6+ messages in thread
From: Junseo Lim @ 2026-08-17 15:50 UTC (permalink / raw)
To: John Fastabend, Jakub Sitnicki, Jiayuan Chen
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Andrii Nakryiko, Eduard Zingerman, linux-kernel,
bpf, netdev, Sechang Lim, Daniel Borkmann, Emil Tsalapatis
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. The strparser clones are unowned, but keep the original
truesize.
sk_psock_skb_ingress_self() assigns receive ownership with
skb_set_owner_r(). That charges each clone to the socket. When this is
repeated for strparser clones, sk_forward_alloc can already be in
deficit before the next owner assignment. Releasing the queued skbs can
then uncharge more memcg pages than were reserved and trigger a
page_counter underflow.
Fix by making same-socket ingress preserve existing receive
ownership and only assign ownership to unowned self-pass skbs. For
strparser clones, use a zero-sized sk_rmem_schedule() before
skb_set_owner_r() to settle any sk_forward_alloc deficit without
reserving the skb's full truesize again.
When the skb is retried from the psock backlog, preserve the original
_sk_redir value across skb_bpf_redirect_clear() so the deferred path
keeps the same ingress and strparser state. Perform the deferred owner
assignment under the socket lock because psock backlog work only holds
psock->work_mutex.
Fixes: 144748eb0c44 ("bpf, sockmap: Fix incorrect fwd_alloc accounting")
Reported-by: Sechang Lim <rhkrqnwk98@gmail.com>
Suggested-by: Emil Tsalapatis <emil@etsalapatis.com>
Assisted-by: Codex:gpt-5.5
Signed-off-by: Junseo Lim <zirajs7@gmail.com>
---
net/core/skmsg.c | 125 +++++++++++++++++++++++++++++++++++++++--------
1 file changed, 105 insertions(+), 20 deletions(-)
diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index 2521b643fa05..347cb168f749 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -586,21 +586,24 @@ 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_self_backlog(struct sk_psock *psock,
+ struct sk_buff *skb,
+ 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)
+ u32 off, u32 len, bool settle_fwd_alloc)
{
struct sock *sk = psock->sk;
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_backlog(psock, skb, off,
+ len, true,
+ settle_fwd_alloc);
msg = sk_psock_create_ingress_msg(sk, skb);
if (!msg)
return -EAGAIN;
@@ -618,34 +621,100 @@ 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.
+static int sk_psock_skb_ingress_self_assign(struct sock *sk,
+ struct sk_buff *skb,
+ bool settle_fwd_alloc)
+{
+ /* Leave skbs already receive-accounted to sk untouched. */
+ if (skb->sk == sk && skb->destructor == sock_rfree)
+ return 0;
+
+ if (settle_fwd_alloc) {
+ sock_owned_by_me(sk);
+
+ if (!sk_rmem_schedule(sk, skb, 0))
+ return -EAGAIN;
+ }
+
+ skb_set_owner_r(skb, sk);
+ return 0;
+}
+
+/* Puts an skb on the ingress queue for psock->sk.
+ *
+ * If the skb already has receive ownership for this socket, leave socket
+ * memory accounting untouched. Otherwise, before assigning receive ownership
+ * to an unowned strparser SK_PASS skb, 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 sock *sk = psock->sk;
struct sk_msg *msg = alloc_sk_msg(GFP_ATOMIC);
+ int err;
+
+ if (unlikely(!msg))
+ return -EAGAIN;
+
+ err = sk_psock_skb_ingress_self_assign(sk, skb, settle_fwd_alloc);
+ if (err)
+ goto free;
+
+ /* This is used in tcp_bpf_recvmsg_parser() to determine whether the
+ * data originates from the socket's own protocol stack. No need to
+ * refcount sk because msg's lifetime is bound to sk via the ingress_msg.
+ */
+ msg->sk = sk;
+ err = sk_psock_skb_ingress_enqueue(skb, off, len, psock, sk, msg,
+ take_ref);
+ if (err < 0)
+ goto free;
+
+ return err;
+free:
+ kfree(msg);
+ return err;
+}
+
+static int sk_psock_skb_ingress_self_backlog(struct sk_psock *psock,
+ struct sk_buff *skb,
+ u32 off, u32 len, bool take_ref,
+ bool settle_fwd_alloc)
+{
struct sock *sk = psock->sk;
+ struct sk_msg *msg = alloc_sk_msg(GFP_ATOMIC);
int err;
if (unlikely(!msg))
return -EAGAIN;
- skb_set_owner_r(skb, sk);
+
+ lock_sock(sk);
+ err = sk_psock_skb_ingress_self_assign(sk, skb, settle_fwd_alloc);
+ release_sock(sk);
+ if (err)
+ goto free;
/* This is used in tcp_bpf_recvmsg_parser() to determine whether the
* data originates from the socket's own protocol stack. No need to
* refcount sk because msg's lifetime is bound to sk via the ingress_msg.
*/
msg->sk = sk;
- err = sk_psock_skb_ingress_enqueue(skb, off, len, psock, sk, msg, take_ref);
+ err = sk_psock_skb_ingress_enqueue(skb, off, len, psock, sk, msg,
+ take_ref);
if (err < 0)
- kfree(msg);
+ goto free;
+
+ return err;
+free:
+ kfree(msg);
return err;
}
static int sk_psock_handle_skb(struct sk_psock *psock, struct sk_buff *skb,
- u32 off, u32 len, bool ingress)
+ u32 off, u32 len, bool ingress,
+ bool self_pass, bool strparser)
{
if (!ingress) {
if (!sock_writeable(psock->sk))
@@ -653,7 +722,11 @@ static int sk_psock_handle_skb(struct sk_psock *psock, struct sk_buff *skb,
return skb_send_sock(psock->sk, skb, off, len);
}
- return sk_psock_skb_ingress(psock, skb, off, len);
+ if (self_pass)
+ return sk_psock_skb_ingress_self_backlog(psock, skb, off,
+ len, true, strparser);
+
+ return sk_psock_skb_ingress(psock, skb, off, len, strparser);
}
static void sk_psock_skb_state(struct sk_psock *psock,
@@ -694,9 +767,14 @@ static void sk_psock_backlog(struct work_struct *work)
return;
mutex_lock(&psock->work_mutex);
while ((skb = skb_peek(&psock->ingress_skb))) {
+ unsigned long saved_redir;
+ bool strparser;
+ bool self_pass;
+
len = skb->len;
off = 0;
- if (skb_bpf_strparser(skb)) {
+ strparser = skb_bpf_strparser(skb);
+ if (strparser) {
struct strp_msg *stm = strp_msg(skb);
off = stm->offset;
@@ -710,17 +788,20 @@ static void sk_psock_backlog(struct work_struct *work)
}
ingress = skb_bpf_ingress(skb);
+ self_pass = ingress && !skb_bpf_redirect_fetch(skb);
+ saved_redir = skb->_sk_redir;
skb_bpf_redirect_clear(skb);
do {
ret = -EIO;
if (!sock_flag(psock->sk, SOCK_DEAD))
ret = sk_psock_handle_skb(psock, skb, off,
- len, ingress);
+ len, ingress,
+ self_pass, strparser);
if (ret <= 0) {
if (ret == -EAGAIN) {
sk_psock_skb_state(psock, state, len, off);
/* Restore redir info we cleared before */
- skb_bpf_set_redir(skb, psock->sk, ingress);
+ skb->_sk_redir = saved_redir;
/* Delay slightly to prioritize any
* other work that might be here.
*/
@@ -1017,6 +1098,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 +1107,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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH bpf v3 2/2] selftests/bpf: Cover strparser self-pass forward allocation
2026-08-17 15:50 [PATCH bpf v3 0/2] bpf, sockmap: fix forward allocation accounting in strparser self-pass path Junseo Lim
2026-08-17 15:50 ` [PATCH bpf v3 1/2] bpf, sockmap: settle sk_forward_alloc for strparser SK_PASS Junseo Lim
@ 2026-08-17 15:50 ` Junseo Lim
2026-08-17 16:52 ` bot+bpf-ci
1 sibling, 1 reply; 6+ messages in thread
From: Junseo Lim @ 2026-08-17 15:50 UTC (permalink / raw)
To: John Fastabend, Jakub Sitnicki, Jiayuan Chen
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Andrii Nakryiko, Eduard Zingerman, linux-kernel,
bpf, netdev, Sechang Lim, Daniel Borkmann, Emil Tsalapatis
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf v3 2/2] selftests/bpf: Cover strparser self-pass forward allocation
2026-08-17 15:50 ` [PATCH bpf v3 2/2] selftests/bpf: Cover strparser self-pass forward allocation Junseo Lim
@ 2026-08-17 16:52 ` bot+bpf-ci
2026-09-02 8:40 ` Emil Tsalapatis
0 siblings, 1 reply; 6+ messages in thread
From: bot+bpf-ci @ 2026-08-17 16:52 UTC (permalink / raw)
To: zirajs7, john.fastabend, jakub, jiayuan.chen
Cc: davem, edumazet, kuba, pabeni, horms, andrii, eddyz87,
linux-kernel, bpf, netdev, rhkrqnwk98, daniel, emil, ast, andrii,
daniel, martin.lau, eddyz87, yonghong.song, clm, ihor.solodrai
[-- Attachment #1: Type: text/plain, Size: 9749 bytes --]
> 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_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,
sockmap_strp_get_fwd_alloc() takes an arbitrary socket fd but hardcodes
.sdiag_family = AF_INET and casts the sockaddr_storage results of
getsockname()/getpeername() to struct sockaddr_in * without checking
local.ss_family.
For an AF_INET6 socket it would build a request from IPv4-shaped fields of
an IPv6 address and then fail the exact lookup, returning -ENOENT and
causing a confusing test failure rather than a wrong-family diagnostic.
This matters because every other subtest in this file is parameterised over
AF_INET and AF_INET6 (test_sockmap_strp_pass(AF_INET6, ...) etc.), so this
helper is a candidate for reuse in the v6 direction.
Could the helper derive sdiag_family/idiag_src/idiag_dst from ss_family, or
reject a non-AF_INET socket explicitly?
> + .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;
A subsystem pattern flags this as potentially concerning: the new subtest
adds an undeclared kernel-config dependency on the INET socket-monitoring
interface and turns its absence into a test failure rather than a skip.
socket(AF_NETLINK, SOCK_RAW, NETLINK_SOCK_DIAG) fails with EPROTONOSUPPORT
when sock_diag is unavailable, and when inet_diag/tcp_diag is not built in
(or is a module that cannot be auto-loaded) inet_diag_lock_handler() /
__sock_diag_cmd() reply with NLMSG_ERROR, which this helper maps to
-EINVAL. Either way, the ASSERT_OK(err, "sockmap_strp_get_fwd_alloc") at
test_sockmap_strp_self_pass_fwd_alloc() reports a failure that has nothing
to do with the sockmap/strparser behaviour under test.
CONFIG_INET_DIAG and CONFIG_INET_TCP_DIAG do not appear in
tools/testing/selftests/bpf/config or any of the per-arch config fragments.
They only default to y from net/ipv4/Kconfig, and are tristate, so a base
config with =m and no installed modules, or =n, breaks the subtest.
Should the config symbols be added to tools/testing/selftests/bpf/config,
or should the code detect the missing capability and call test__skip() with
a reason?
[ ... ]
> +/* 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;
Does anything here confirm that the one-byte parser actually split the 4 KB
write into many strparser messages?
Reading a single byte and comparing it to snd[0] succeeds identically
whether the stream parser/verdict programs took effect or not (a plain TCP
socket returns the same byte). The only other assertion is sk_forward_alloc
>= 0, which is trivially true when no self-pass clone was ever charged.
If the setup silently stops exercising the path -- for example, a future
change rejects a parse verdict of 1, the parser attach becomes a no-op for
some reason, or the messages end up on the psock backlog and are dropped --
the subtest degrades into a tautological pass and stops guarding the fix.
> +
> + 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;
Can the asserted condition (sk_forward_alloc >= 0 while self-pass strparser
clones are queued) fail on a fixed kernel?
With the fix, sk_psock_skb_ingress_self_assign() does:
if (settle_fwd_alloc) {
sock_owned_by_me(sk);
if (!sk_rmem_schedule(sk, skb, 0))
return -EAGAIN;
}
skb_set_owner_r(skb, sk);
sk_rmem_schedule(sk, skb, 0) reserves only ceil(deficit / PAGE_SIZE) pages,
leaving sk_forward_alloc in [0, PAGE_SIZE). skb_set_owner_r() then calls
sk_mem_charge(sk, skb->truesize) for the full clone truesize.
net/strparser/strparser.c gives every one of the ~4096 one-byte messages a
skb_clone() of the original 4 KB skb, so truesize is ~4.8 KB per clone.
Therefore, after every single clone charge, sk_forward_alloc is negative
(roughly -(truesize - PAGE_SIZE) .. -truesize) on the fixed kernel too.
The assertion only passes because the one recv() of 1 byte fully consumes
the first message, and kfree_sk_msg() -> consume_skb() -> sock_rfree()
uncharges exactly one truesize, cancelling the last charge and leaving
sk_forward_alloc in [0, PAGE_SIZE).
Any clone charged between that recv() and the inet_diag read makes the
sampled value negative and the subtest fails even though the kernel is
correct. Two realistic ways that happens:
1. Deferred strparser work. tcp_bpf_recvmsg_parser() calls tcp_data_ready()
while holding lock_sock() when sk_receive_queue is non-empty
(net/ipv4/tcp_bpf.c) -- exactly the behaviour the comment at the top of
this file already documents for sockmap_strp_consume_pre_data().
strp_data_ready() then queue_work()s the parse, which runs after recv()
returns and charges more clones.
2. psock backlog. If any one ingress attempt returns -EAGAIN
(sk_rmem_schedule() failing under memcg or global tcp_mem pressure -- the
test intentionally piles ~20 MB of clone truesize onto one socket), the skb
is queued on psock->ingress_skb and from then on every message is charged
asynchronously by sk_psock_backlog(), potentially after the recv().
Could the test sample something that is monotone with respect to the bug
instead: drain all 4 KB (so every clone is uncharged), or re-read/poll
until the value settles, or assert on a value that the fixed kernel
guarantees at all times?
> +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();
> }
[ ... ]
---
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/32045036810
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf v3 1/2] bpf, sockmap: settle sk_forward_alloc for strparser SK_PASS
2026-08-17 15:50 ` [PATCH bpf v3 1/2] bpf, sockmap: settle sk_forward_alloc for strparser SK_PASS Junseo Lim
@ 2026-08-17 17:13 ` bot+bpf-ci
0 siblings, 0 replies; 6+ messages in thread
From: bot+bpf-ci @ 2026-08-17 17:13 UTC (permalink / raw)
To: zirajs7, john.fastabend, jakub, jiayuan.chen
Cc: davem, edumazet, kuba, pabeni, horms, andrii, eddyz87,
linux-kernel, bpf, netdev, rhkrqnwk98, daniel, emil, ast, andrii,
daniel, martin.lau, eddyz87, yonghong.song, clm, ihor.solodrai
[-- Attachment #1: Type: text/plain, Size: 4936 bytes --]
> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index 2521b643fa05..347cb168f749 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -586,21 +586,24 @@ 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_self_backlog(struct sk_psock *psock,
> + struct sk_buff *skb,
> + u32 off, u32 len, bool take_ref,
> + bool settle_fwd_alloc);
The forward declaration of sk_psock_skb_ingress_self() at line 588 is
now dead code. Before the commit, sk_psock_skb_ingress() (defined at
line 596) called it for the skb->sk == psock->sk case, which required
the forward declaration. This commit redirects that call site to
sk_psock_skb_ingress_self_backlog() instead. The only remaining caller
is sk_psock_verdict_apply(), which is well after the definition. Should
this prototype be removed?
[ ... ]
> @@ -618,34 +621,100 @@ 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.
> +static int sk_psock_skb_ingress_self_assign(struct sock *sk,
> + struct sk_buff *skb,
> + bool settle_fwd_alloc)
> +{
> + /* Leave skbs already receive-accounted to sk untouched. */
> + if (skb->sk == sk && skb->destructor == sock_rfree)
> + return 0;
> +
> + if (settle_fwd_alloc) {
> + sock_owned_by_me(sk);
> +
> + if (!sk_rmem_schedule(sk, skb, 0))
> + return -EAGAIN;
> + }
> +
> + skb_set_owner_r(skb, sk);
> + return 0;
> +}
> +
> +/* Puts an skb on the ingress queue for psock->sk.
> + *
> + * If the skb already has receive ownership for this socket, leave socket
> + * memory accounting untouched. Otherwise, before assigning receive ownership
> + * to an unowned strparser SK_PASS skb, 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 sock *sk = psock->sk;
> struct sk_msg *msg = alloc_sk_msg(GFP_ATOMIC);
> + int err;
> +
> + if (unlikely(!msg))
> + return -EAGAIN;
> +
> + err = sk_psock_skb_ingress_self_assign(sk, skb, settle_fwd_alloc);
> + if (err)
> + goto free;
> +
> + /* This is used in tcp_bpf_recvmsg_parser() to determine whether the
> + * data originates from the socket's own protocol stack. No need to
> + * refcount sk because msg's lifetime is bound to sk via the ingress_msg.
> + */
> + msg->sk = sk;
> + err = sk_psock_skb_ingress_enqueue(skb, off, len, psock, sk, msg,
> + take_ref);
> + if (err < 0)
> + goto free;
> +
> + return err;
> +free:
> + kfree(msg);
> + return err;
> +}
> +
> +static int sk_psock_skb_ingress_self_backlog(struct sk_psock *psock,
> + struct sk_buff *skb,
> + u32 off, u32 len, bool take_ref,
> + bool settle_fwd_alloc)
> +{
> struct sock *sk = psock->sk;
> + struct sk_msg *msg = alloc_sk_msg(GFP_ATOMIC);
> int err;
>
> if (unlikely(!msg))
> return -EAGAIN;
> - skb_set_owner_r(skb, sk);
> +
> + lock_sock(sk);
> + err = sk_psock_skb_ingress_self_assign(sk, skb, settle_fwd_alloc);
> + release_sock(sk);
> + if (err)
> + goto free;
>
> /* This is used in tcp_bpf_recvmsg_parser() to determine whether the
> * data originates from the socket's own protocol stack. No need to
> * refcount sk because msg's lifetime is bound to sk via the ingress_msg.
> */
> msg->sk = sk;
> - err = sk_psock_skb_ingress_enqueue(skb, off, len, psock, sk, msg, take_ref);
> + err = sk_psock_skb_ingress_enqueue(skb, off, len, psock, sk, msg,
> + take_ref);
> if (err < 0)
> - kfree(msg);
> + goto free;
> +
> + return err;
> +free:
> + kfree(msg);
> return err;
> }
Is GFP_ATOMIC necessary in sk_psock_skb_ingress_self_backlog()? The
function is only called from sk_psock_backlog(), which is the psock
backlog worker running in plain process context - the lock_sock() call
proves it can sleep. Before this commit, these same skbs were allocated
by sk_psock_create_ingress_msg() with alloc_sk_msg(GFP_KERNEL). Would
GFP_KERNEL be more appropriate here to avoid draining atomic reserves
and reduce the chance of allocation failure under memory pressure?
[ ... ]
---
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/32045036810
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf v3 2/2] selftests/bpf: Cover strparser self-pass forward allocation
2026-08-17 16:52 ` bot+bpf-ci
@ 2026-09-02 8:40 ` Emil Tsalapatis
0 siblings, 0 replies; 6+ messages in thread
From: Emil Tsalapatis @ 2026-09-02 8:40 UTC (permalink / raw)
To: bot+bpf-ci
Cc: zirajs7, john.fastabend, jakub, jiayuan.chen, davem, edumazet,
kuba, pabeni, horms, andrii, eddyz87, linux-kernel, bpf, netdev,
rhkrqnwk98, daniel, ast, martin.lau, yonghong.song, clm,
ihor.solodrai
Hi Junseo,
Sorry for the late reply.
On Mon, Aug 17, 2026 at 12:52 PM <bot+bpf-ci@kernel.org> wrote:
>
> > 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_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,
>
> sockmap_strp_get_fwd_alloc() takes an arbitrary socket fd but hardcodes
> .sdiag_family = AF_INET and casts the sockaddr_storage results of
> getsockname()/getpeername() to struct sockaddr_in * without checking
> local.ss_family.
>
> For an AF_INET6 socket it would build a request from IPv4-shaped fields of
> an IPv6 address and then fail the exact lookup, returning -ENOENT and
> causing a confusing test failure rather than a wrong-family diagnostic.
>
> This matters because every other subtest in this file is parameterised over
> AF_INET and AF_INET6 (test_sockmap_strp_pass(AF_INET6, ...) etc.), so this
> helper is a candidate for reuse in the v6 direction.
>
> Could the helper derive sdiag_family/idiag_src/idiag_dst from ss_family, or
> reject a non-AF_INET socket explicitly?
>
> > + .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;
>
> A subsystem pattern flags this as potentially concerning: the new subtest
> adds an undeclared kernel-config dependency on the INET socket-monitoring
> interface and turns its absence into a test failure rather than a skip.
>
I think the bot feedback is all valid (also in the previous patch). We
should esp. address
the above since it would be a pretty nonbvious error to debug.
For the actual fix in the previous patch, the new code seems
significantly more complicated
than the previous iteration. Are we sure we can't keep the fix a bit simpler?
pw-bot: cr
> socket(AF_NETLINK, SOCK_RAW, NETLINK_SOCK_DIAG) fails with EPROTONOSUPPORT
> when sock_diag is unavailable, and when inet_diag/tcp_diag is not built in
> (or is a module that cannot be auto-loaded) inet_diag_lock_handler() /
> __sock_diag_cmd() reply with NLMSG_ERROR, which this helper maps to
> -EINVAL. Either way, the ASSERT_OK(err, "sockmap_strp_get_fwd_alloc") at
> test_sockmap_strp_self_pass_fwd_alloc() reports a failure that has nothing
> to do with the sockmap/strparser behaviour under test.
>
> CONFIG_INET_DIAG and CONFIG_INET_TCP_DIAG do not appear in
> tools/testing/selftests/bpf/config or any of the per-arch config fragments.
> They only default to y from net/ipv4/Kconfig, and are tristate, so a base
> config with =m and no installed modules, or =n, breaks the subtest.
>
> Should the config symbols be added to tools/testing/selftests/bpf/config,
> or should the code detect the missing capability and call test__skip() with
> a reason?
>
> [ ... ]
>
> > +/* 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;
>
> Does anything here confirm that the one-byte parser actually split the 4 KB
> write into many strparser messages?
>
> Reading a single byte and comparing it to snd[0] succeeds identically
> whether the stream parser/verdict programs took effect or not (a plain TCP
> socket returns the same byte). The only other assertion is sk_forward_alloc
> >= 0, which is trivially true when no self-pass clone was ever charged.
>
> If the setup silently stops exercising the path -- for example, a future
> change rejects a parse verdict of 1, the parser attach becomes a no-op for
> some reason, or the messages end up on the psock backlog and are dropped --
> the subtest degrades into a tautological pass and stops guarding the fix.
>
> > +
> > + 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;
>
> Can the asserted condition (sk_forward_alloc >= 0 while self-pass strparser
> clones are queued) fail on a fixed kernel?
>
> With the fix, sk_psock_skb_ingress_self_assign() does:
>
> if (settle_fwd_alloc) {
> sock_owned_by_me(sk);
> if (!sk_rmem_schedule(sk, skb, 0))
> return -EAGAIN;
> }
> skb_set_owner_r(skb, sk);
>
> sk_rmem_schedule(sk, skb, 0) reserves only ceil(deficit / PAGE_SIZE) pages,
> leaving sk_forward_alloc in [0, PAGE_SIZE). skb_set_owner_r() then calls
> sk_mem_charge(sk, skb->truesize) for the full clone truesize.
> net/strparser/strparser.c gives every one of the ~4096 one-byte messages a
> skb_clone() of the original 4 KB skb, so truesize is ~4.8 KB per clone.
>
> Therefore, after every single clone charge, sk_forward_alloc is negative
> (roughly -(truesize - PAGE_SIZE) .. -truesize) on the fixed kernel too.
>
> The assertion only passes because the one recv() of 1 byte fully consumes
> the first message, and kfree_sk_msg() -> consume_skb() -> sock_rfree()
> uncharges exactly one truesize, cancelling the last charge and leaving
> sk_forward_alloc in [0, PAGE_SIZE).
>
> Any clone charged between that recv() and the inet_diag read makes the
> sampled value negative and the subtest fails even though the kernel is
> correct. Two realistic ways that happens:
>
> 1. Deferred strparser work. tcp_bpf_recvmsg_parser() calls tcp_data_ready()
> while holding lock_sock() when sk_receive_queue is non-empty
> (net/ipv4/tcp_bpf.c) -- exactly the behaviour the comment at the top of
> this file already documents for sockmap_strp_consume_pre_data().
> strp_data_ready() then queue_work()s the parse, which runs after recv()
> returns and charges more clones.
>
> 2. psock backlog. If any one ingress attempt returns -EAGAIN
> (sk_rmem_schedule() failing under memcg or global tcp_mem pressure -- the
> test intentionally piles ~20 MB of clone truesize onto one socket), the skb
> is queued on psock->ingress_skb and from then on every message is charged
> asynchronously by sk_psock_backlog(), potentially after the recv().
>
> Could the test sample something that is monotone with respect to the bug
> instead: drain all 4 KB (so every clone is uncharged), or re-read/poll
> until the value settles, or assert on a value that the fixed kernel
> guarantees at all times?
>
> > +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();
> > }
>
> [ ... ]
>
>
> ---
> 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/32045036810
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-09-02 8:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-08-17 15:50 [PATCH bpf v3 0/2] bpf, sockmap: fix forward allocation accounting in strparser self-pass path Junseo Lim
2026-08-17 15:50 ` [PATCH bpf v3 1/2] bpf, sockmap: settle sk_forward_alloc for strparser SK_PASS Junseo Lim
2026-08-17 17:13 ` bot+bpf-ci
2026-08-17 15:50 ` [PATCH bpf v3 2/2] selftests/bpf: Cover strparser self-pass forward allocation Junseo Lim
2026-08-17 16:52 ` bot+bpf-ci
2026-09-02 8:40 ` Emil Tsalapatis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®