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

[Martin KaFai Lau <martin.lau@linux.dev>]

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()?

> 
>>>> The only path left is bpf_iter, which I believe was the primary use case
>>>> when adding bpf_map_update_elem(sockmap) support [1]. It would be nice
>>>> to avoid bh_lock_sock() when calling from all bpf_iter (tcp/udp/unix)
>>>> where lock_sock() has already been done. It is more for
>>>> reading-correctness though. This just came to my mind.
>>>> has_current_bpf_ctx() can be used to check this. sockopt_lock_sock() has
>>>> been using it to conditionally take lock_sock() or not.
>>>
>>> [ One clarification: bh_lock_sock() is a sock_map_update_elem() thing,
>>> which can only be called by a bpf prog. IOW, has_current_bpf_ctx() is
>>> always `true` in sock_map_update_elem(), right? ]
>>
>> For all the bpf prog types allowed by may_update_sockmap() to do
>> bpf_map_update_elem(sockmap), only BPF_TRACE_ITER should have
>> has_current_bpf_ctx() == true. The tc prog (and others allowed in
>> may_update_sockmap()) will have has_current_bpf_ctx() == false when
>> calling sock_map_update_elem().
> 
> OK, so let's take test_sockmap_update.c:copy_sock_map(). It is a tc prog
> and it calls bpf_map_update_elem() -> sock_map_update_elem(), right? But
> running `test_progs -t "sockmap_basic/sockmap update"` shows (pr_warn() in
> sock_map_update_elem()) that has_current_bpf_ctx() == true. That's expected

I think it is because of the bpf_prog_test_run_skb() code path used by 
the test_sockmap_update() test. This would need to be addressed if 
has_current_bpf_ctx() was used in sock_map_update_elem().

> and has_current_bpf_ctx() would be false if sock_map_update_elem() was ran
> via a hook?

It should be false when the bpf prog is run by tc instead of 
bpf_prog_test_run_skb().

>>> Let me know if I'm correctly rephrasing your idea: assume all bpf-context
>>> callers hold the socket locked or keep it "stable" (meaning: "sk won't
>>> surprise sockmap update by some breaking state change coming from another
>>> thread"). As you said, most bpf iters already take the sock_lock(), and I
>>
>> Right, all bpf iter (udp, tcp, unix) has acquired the lock_sock() before
>> running the bpf iter prog. afaik, the only exception is netlink bpf iter
>> but it cannot be added to sock_map afaik.
> 
> And sock_{map,hash}_seq_show() (being a part of bpf iter machinery) needs
> to take lock_sock() just as well? Would that require a special-casing
> (unix_state_lock()) for af_unix?

I would think so for lock_sock() considering the current bh_lock_sock 
without !sock_owned_by_user() usage is incorrect in 
sock_map_update_elem(). [ this probably should be a separate issue for 
another patch ]

Some more side-tracking... from looking at the code, the bpf_iter of 
sock_{map,hash} can do bpf_map_lookup_elem(&sock_map, ...). This 
bpr_iter program probably will be failed to load because the 
bpf_sk_release() is not available.

I still don't have good idea what to do with the tc's prog calling 
sock_map_update_elem().