Re: [PATCH bpf v2 2/2] bpf: Unconditionally take socket references in lookup helpers

[Jakub Sitnicki]

On Tue, Aug 04, 2026 at 09:01 PM -07, Kuniyuki Iwashima wrote:
> On Tue, Aug 4, 2026 at 3:14 AM Jakub Sitnicki <jakub@xxxxxxxxxxxxxx> wrote:
>>
>> On Mon, Aug 03, 2026 at 11:00 AM +02, Michal Luczaj wrote:
>> > Lookup helpers gate whether to acquire a socket reference on
>> > sk_is_refcounted(), a check re-evaluated at release. An established socket
>> > refcounted at acquire time can gain SOCK_RCU_FREE via
>> > connect(AF_UNSPEC)+listen() before release runs; the release-side re-check
>> > then reads sk_is_refcounted() == false and skips the put. The reference
>> > leaks.
>> >
>> > Make acquire and release unconditional and symmetric: always take a
>> > reference, always put it. Adapt sk_select_reuseport().
>> >
>> > Fixes: 6acc9b432e67 ("bpf: Add helper to retrieve socket in BPF")
>> > Fixes: 64d85290d79c ("bpf: Allow bpf_map_lookup_elem for SOCKMAP and SOCKHASH")
>> > Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
>> > Closes: https://lore.kernel.org/bpf/20260701235552.2B0AA1F00A3F@xxxxxxxxxxxxxxx/
>> > Signed-off-by: Michal Luczaj <mhal@xxxxxxx>
>> > Reviewed-by: Emil Tsalapatis <emil@xxxxxxxxxxxxxxx>
>> > ---
>> > TC bpf_sk_assign() has the same issue; it takes a reference only when
>> > sk_is_refcounted() is true at assign time, but sock_pfree() (the skb
>> > destructor it installs) re-checks sk_is_refcounted() independently at
>> > release time. The same connect(AF_UNSPEC)+listen() transition leaks the
>> > socket here too. I'd welcome suggestions on the right way to handle this.
>>
>> Can we make this scenario unsupported?
>>
>> listen() could return EBUSY if called on a socket that is refcounted.
>>
>> WDYT?
>
> I discussed this kind of buggy rehash with Eric today.
>
> We can't make it unsupported although it's super unlikely
> that this is used by a real application.

I'm just wondering why not?

First I thought is was due to POSIX compatibility but POSIX seems to
define connect(AF_UNSPEC) only for connection-less sockets [1]:

"""
If he initiating socket is not connection-mode, then connect() shall set
the socket's peer address [...] If the sa_family member of address is
AF_UNSPEC, the socket's peer address shall be reset.
"""

So if this is Linux-specific behavior (?) and we don't exect any users
to rely on it, why not change it and see if anyone complains?

Seems like wasted effort to try to make it work properly.

[1] https://man.archlinux.org/man/connect.3p