netdev - Re: [PATCH bpf-next v6] bpf, sockmap: avoid using sk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <dbd4b59b-4183-47fe-9287-c0a438df348c@linux.dev>
Date: Thu, 22 May 2025 16:23:10 -0700
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Jiayuan Chen <jiayuan.chen@...ux.dev>
Cc: bpf@...r.kernel.org, Michal Luczaj <mhal@...x.co>,
 John Fastabend <john.fastabend@...il.com>,
 Jakub Sitnicki <jakub@...udflare.com>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Simon Horman <horms@...nel.org>,
 Thadeu Lima de Souza Cascardo <cascardo@...lia.com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v6] bpf, sockmap: avoid using sk_socket after
 free when sending

On 5/22/25 3:56 PM, Jiayuan Chen wrote:
>>>   @@ -656,6 +656,13 @@ static void sk_psock_backlog(struct work_struct *work)
>>>   bool ingress;
>>>   int ret;
>>>   > + /* Increment the psock refcnt to synchronize with close(fd) path in
>>>   + * sock_map_close(), ensuring we wait for backlog thread completion
>>>   + * before sk_socket freed. If refcnt increment fails, it indicates
>>>   + * sock_map_close() completed with sk_socket potentially already freed.
>>>   + */
>>>   + if (!sk_psock_get(psock->sk))
>>
>> This seems to be the first use case to pass "psock->sk" to "sk_psock_get()".
>>
>> I could have missed the sock_map details here. Considering it is racing with sock_map_close() which should also do a sock_put(sk) [?],
>>
>> could you help to explain what makes it safe to access the psock->sk here?
>>

> 
> Using 'sk_psock_get(psock->sk)' in the workqueue is safe because
> sock_map_close() only reduces the reference count of psock to zero, while
> the actual memory release is fully handled by the RCU callback: sk_psock_destroy().
> 
> In sk_psock_destroy(), we first cancel_delayed_work_sync() to wait for the
> workqueue to complete, and then perform sock_put(psock->sk). This means we

Got it. The sock_put(psock->sk) done after a rcu gp is the part that I was missing.

Applied. Thanks.