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

[bot+bpf-ci]

> 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