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

On Wed, Aug 05, 2026 at 05:00 PM +02, Michal Luczaj wrote:
> On 8/4/26 12:14, Jakub Sitnicki 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 <[email protected]>
>>> Closes: https://lore.kernel.org/bpf/[email protected]/
>>> Signed-off-by: Michal Luczaj <[email protected]>
>>> Reviewed-by: Emil Tsalapatis <[email protected]>
>>> ---
>>> 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'm not sure I understand. Isn't a freshly created socket already
> refcounted?

Yeah, it's just me not thinking this through. We don't have any state
today to check if a socket was an established socket before.