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

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?

[ ... ]


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