Re: [PATCH bpf v3 5/5] bpf, sockmap: Adapt for af_unix-specific lock

[Michal Luczaj <mhal@rbox.co>]

On 4/2/26 03:34, Martin KaFai Lau wrote:
> On Wed, 01 Apr 2026 00:43:58 +0200, Michal Luczaj wrote:
>> On 3/31/26 02:20, Martin KaFai Lau wrote:
>>> On 3/30/26 4:03 PM, Michal Luczaj wrote:
>>>> On 3/26/26 07:26, Martin KaFai Lau wrote:
>>>>> On 3/15/26 4:58 PM, Michal Luczaj wrote:
>>>>>>> Beside, from looking at the may_update_sockmap(), I don't know if it is
>>>>>>> even doable (or useful) to bpf_map_update_elem(unix_sk) in
>>>>>>> tc/flow_dissector/xdp. One possible path is the SOCK_FILTER when looking
>>>>>>> at unix_dgram_sendmsg() => sk_filter(). It was not the original use case
>>>>>>> when the bpf_map_update_elem(sockmap) support was added iirc.
>>>>>>
>>>>>> What about a situation when unix_sk is stored in a sockmap, then tc prog
>>>>>> looks it up and invokes bpf_map_update_elem(unix_sk)? I'm not sure it's
>>>>>> useful, but seems doable.
>>>>>
>>>>> [ Sorry for the late reply ]
>>>>>
>>>>> It is a bummer that the bpf_map_update_elem(unix_sk) path is possible
>>>>> from tc :(
>>>>>
>>>>> Then unix_state_lock() in its current form cannot be safely acquired in
>>>>> sock_map_update_elem(). It is currently a spin_lock() instead of
>>>>> spin_lock_bh().
>>>>
>>>> Is there a specific deadlock you have in your mind?
>>>
>>> e.g. unix_stream_connect() is taking unix_state_lock(). Can a tc's 
>>> ingress bpf prog call unix_state_lock()?
>>
>> Ah, right, that's the problem, thanks for explaining.
>>
>> But, as I've asked in the parallel thread, do we really need to take the
>> unix_state_lock() in sock_map_update_elem()? Taking it in
>> sock_map_update_elem_sys() fixes the null-ptr-deref and does not lead to a
>> deadlock. Taking unix_state_lock() in sock_map_update_elem() seems
>> unnecessary. Well, at least under the assumption progs can only access
>> unix_sk via the sockmap lookup.
> 
> right, sock_map_update_elem_sys() should be safe to take
> unix_state_lock().
> 
> If it is fixed by testing unix_peer(), is the TCPF_ESTABLISHED test
> in sock_map_sk_state_allowed() still useful and needed?

I don't think it's necessary. Although removing it may slightly mask the
fact that we're interested in TCP_ESTABLISHED sockets (we watch the sock's
life cycle and invoke sock_map_close() as it transitions to TCP_CLOSE).
Removing this check will also mean listening socks will be rejected not
early in sock_map_sk_state_allowed(), but deeper in
unix_stream_bpf_update_proto() (and with a different error code?).

> Also,
> please explain in detail in the commit message why testing for NULL
> without unix_state_lock() is enough.

OK, will do.

> For example, for the BPF iterator on
> sock_map, my understanding is that unix_release_sock() can still happen
> while the BPF iterator is iterating over a unix_sock. I guess a future
> unix_state_lock() in the iterator's seq_show() should be useful.

That's right. That's also why, I think, Kuniyuki was asking for
"lock_sock() + unix_state_lock() + SOCK_DEAD check" in a parallel thread.

> It will also be useful to mention what was discovered about TC + lookup
> + update_elem(&sock_map, ...) and why it is not safe to take
> unix_state_lock() in that path. Thanks.

The softirq vs. process context? Sure, I'll mention that.

Took a while (sorry), but here's v4:
https://lore.kernel.org/netdev/20260414-unix-proto-update-null-ptr-deref-v4-0-2af6fe97918e@rbox.co/