Re: [BUG] bpf: tcp: bpf_tcp_ops_set_rcvlowat() missing from BTF with clang KCSAN

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message of "bpf: tcp: bpf_tcp_ops_set_rcvlowat()
  missing from BTF with clang KCSAN" is cut off and missing required…
- [Low] The patch fixes a regression from 5f549c1e49f3 ("bpf: tcp: Add
  kfunc to adjust sk->sk_rcvlowat.") but has no Fixes: tag.

--

bpf: tcp: bpf_tcp_ops_set_rcvlowat() missing from BTF with clang KCSAN

With clang and KCSAN, pahole drops bpf_tcp_ops_set_rcvlowat() from the
vmlinux BTF. This breaks the build when CONFIG_WERROR is set. When it
is not set, __bpf_tcp_ops_init() fails at boot. The patch works around
this by passing the clamped value straight to __tcp_set_rcvlowat()
instead of reassigning the rcvlowat parameter.

> The kfunc came with 5f549c1e49f3 ("bpf: tcp: Add kfunc to adjust
> sk->sk_rcvlowat."), and 201a454df948 ("resolve_btfids: Fix the
> --fatal_warnings exit status") made the missing BTF fatal with
> CONFIG_WERROR. Without CONFIG_WERROR the kernel builds, but bpf_tcp_ops
> is never registered:
>
>   initcall __bpf_tcp_ops_init+0x0/0x70 returned -22 after 302 usecs

[Severity: Low]
Should this carry a Fixes: tag? 5f549c1e49f3 is only named in the prose.
That commit added the "if (rcvlowat < 0) rcvlowat = INT_MAX;"
reassignment in bpf_tcp_ops_set_rcvlowat(), which this patch rewrites.

Users can see this regression. When the kfunc set fails to register,
the chain in __bpf_tcp_ops_init() skips the struct_ops registration:

net/ipv4/bpf_tcp_ops.c:__bpf_tcp_ops_init() {
    ...
	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS,
					       &bpf_tcp_ops_set_rcvlowat_kfunc_id_set);
	ret = ret ?: register_bpf_struct_ops(&bpf_tcp_ops, bpf_tcp_ops);
    ...
}

The sched_ext workaround cited in this message, f2f095c87603, has a
Fixes: tag too. Would something like this work?

Fixes: 5f549c1e49f3 ("bpf: tcp: Add kfunc to adjust sk->sk_rcvlowat.")

[ ... ]

> This seems similar to pahole behaviour sched_ext hit with a bool
> argument, discussed in [2] and worked around in f2f095c87603
> ("sched_ext: Work around pahole 1.32 dropping
> scx_bpf_task_set_lazy_resched() from BTF").

[ ... ]

> Not reassigning the parameter is a workaround for me, with any pahole: clang
> then describes @rcvlowat in w1 from the entry, the kfunc is back in BTF,
> and __bpf_tcp_ops_init() returns 0 (tested in virtme):

[Severity: Medium]
Was the commit message cut off here? It ends with a colon, and the
virtme test output it introduces never appears.

The message also says "discussed in [2]", but there is no [1], no
reference list and no Link: trailer, so the pahole discussion can't be
found from this commit.

There is also no Signed-off-by: line from the author. The DCO in
Documentation/process/submitting-patches.rst requires one, so the patch
can't be applied as posted.

The code change itself looks fine. Passing
"rcvlowat < 0 ? INT_MAX : rcvlowat" behaves the same as the old
reassignment.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/au27Y9m78_3EH2v1%40gmail.com