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-2af6fe979...@rbox.co/
