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

[Michal Luczaj]

On 8/5/26 17:00, Michal Luczaj wrote:
> On 8/4/26 03:58, Kuniyuki Iwashima wrote:
>> On Mon, Aug 3, 2026 at 2:01 AM Michal Luczaj <mhal@xxxxxxx> 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.
>>
>> The same class of issue was reported by listen() + shutdown() + connect().
>> https://lore.kernel.org/netdev/20260804015349.2353056-1-kuniyu@xxxxxxxxxx/
>>
>> Can you test the diff in the thread ?
> 
> Yeah, it does fix it for sk/sockmap lookups. But bpf_sk_assign() still
> leaks; LLM devised a testcase that puts skb into NFQUEUE. Another way of
> escaping the RCU section could probably involve bpf_skb_set_tstamp().

Ugh, I was wrong. It doesn't fix sockmap lookups, it just made the race
window smaller:

listen():
  inet_hash()
    unhashed_state != TCP_LISTEN
      synchronize_rcu()
                                     sock_map_lookup():
                                       rcu_read_lock()
                                       sock_map_lookup()
                                         sk_is_refcounted() == true
                                         refcount_inc_not_zero()
    sock_set_flag(SOCK_RCU_FREE)
                                       bpf_sk_release()
                                         sk_is_refcounted() == false
                                         put skipped, sock leaked