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

[Michal Luczaj]

On 8/5/26 16:56, Jakub Sitnicki wrote:
> 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

Patchwork does not list this series anymore, so I've picked only the
cleanup/optimization patches and re-sent them targeting bpf-next:
https://lore.kernel.org/bpf/20260813-sockmap-lookup-get-ref-v1-0-31f5d55f44ac@xxxxxxx/

thanks,
Michal