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

[Michal Luczaj <mhal@rbox.co>]

On 8/4/26 03:58, Kuniyuki Iwashima wrote:
> On Mon, Aug 3, 2026 at 2:01 AM Michal Luczaj <mhal@rbox.co> 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@kernel.org>
>> Closes: https://lore.kernel.org/bpf/20260701235552.2B0AA1F00A3F@smtp.kernel.org/
>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>> Reviewed-by: Emil Tsalapatis <emil@etsalapatis.com>
>> ---
>> 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@google.com/
> 
> 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().