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

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

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? Also,
please explain in detail in the commit message why testing for NULL
without unix_state_lock() is enough. 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.

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.

> 
> >> ...
> >> 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 ]
> 
> All right, leaving that for later.
> 
> > 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 think ability to bpf_map_lookup_elem(sockmap) was added way before bpf
> iter in
> https://lore.kernel.org/bpf/20200429181154.479310-2-jakub@cloudflare.com/
> and iter "allows" it somewhat accidentally. Would you rather have it
> explicitly dropped?

It is fine to leave it as is. It would be a bit nicer to reject map_lookup
on sockmap instead of giving the confusing error that bpf_sk_release
is not available. That is for another day. I was just wondering what else
could go wrong with map_lookup.