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

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

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

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

[sashiko-bot]

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

New issues:
- [Low] Multi-line comments added in the patch violate the BPF subsystem comment style.

Pre-existing issues:
- [High] Missing socket lock in sk_psock_skb_ingress() for cross-socket redirects causes sk_forward_alloc corruption.
--

--- Patch [1]: [PATCH bpf v3 1/2] bpf, sockmap: settle sk_forward_alloc for strparser SK_PASS ---
commit 74acfd67ff001cb4d8a5e9b0c6aaa8bfc595f145
Author: Junseo Lim <zirajs7@gmail.com>

bpf, sockmap: settle sk_forward_alloc for strparser SK_PASS

This commit addresses sk_forward_alloc deficits caused by strparser SK_PASS
queuing cloned skbs back to the same socket. It settles any deficit before
assigning receive ownership to unowned self-pass skbs.

> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index 2521b643fa05d..347cb168f7491 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
[ ... ]
>  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 (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;

[Severity: High]
This is a pre-existing issue, but does this cross-socket redirect path modify
sk_forward_alloc without holding the socket lock?

When called asynchronously from the backlog workqueue:
sk_psock_backlog() -> sk_psock_handle_skb() -> sk_psock_skb_ingress()

If the skb is redirected to a different socket (where skb->sk does not match
the destination sk), it bypasses the self_backlog path which properly holds
lock_sock(sk). It then proceeds to call sk_psock_create_ingress_msg() and
skb_set_owner_r() without the target socket's lock. Since sk_forward_alloc
is updated via non-atomic operations, could concurrent updates to the target
socket corrupt sk_forward_alloc and cause page_counter underflows?

> @@ -618,34 +621,100 @@ static int sk_psock_skb_ingress(struct sk_psock *psock, struct sk_buff *skb,
[ ... ]
> +/* 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,

[Severity: Low]
This isn't a bug, but does this comment format follow the BPF subsystem
style?

The BPF subsystem uses the modern kernel multi-line comment style, which
requires the opening /* to be on its own line.

[ ... ]
> +	/* 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;

[Severity: Low]
This isn't a bug, but should this multi-line comment also have its opening
/* on a separate line to match the subsystem conventions?

[ ... ]
> +static int sk_psock_skb_ingress_self_backlog(struct sk_psock *psock,
[ ... ]
> +	/* 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;

[Severity: Low]
This isn't a bug, but is the opening /* on the same line missing the BPF
comment style requirement here as well?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817155009.232670-1-zirajs7@gmail.com?part=1

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

[bot+bpf-ci]

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

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

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

[bot+bpf-ci]

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