Re: [PATCH net] bpf: sockmap: Remove preempt_disable in sock_map_sk_acquire

[Paolo Abeni <pabeni@redhat.com>]

On Mon, 2023-07-31 at 20:58 -0700, John Fastabend wrote:
> Jakub Sitnicki wrote:
> > 
> > On Fri, Jul 28, 2023 at 08:44 AM +02, tglozar@redhat.com wrote:
> > > From: Tomas Glozar <tglozar@redhat.com>
> > > 
> > > Disabling preemption in sock_map_sk_acquire conflicts with GFP_ATOMIC
> > > allocation later in sk_psock_init_link on PREEMPT_RT kernels, since
> > > GFP_ATOMIC might sleep on RT (see bpf: Make BPF and PREEMPT_RT co-exist
> > > patchset notes for details).
> > > 
> > > This causes calling bpf_map_update_elem on BPF_MAP_TYPE_SOCKMAP maps to
> > > BUG (sleeping function called from invalid context) on RT kernels.
> > > 
> > > preempt_disable was introduced together with lock_sk and rcu_read_lock
> > > in commit 99ba2b5aba24e ("bpf: sockhash, disallow bpf_tcp_close and update
> > > in parallel"), probably to match disabled migration of BPF programs, and
> > > is no longer necessary.
> > > 
> > > Remove preempt_disable to fix BUG in sock_map_update_common on RT.
> > > 
> > > Signed-off-by: Tomas Glozar <tglozar@redhat.com>
> > > ---
> > 
> > We disable softirq and hold a spin lock when modifying the map/hash in
> > sock_{map,hash}_update_common so this LGTM:
> > 
> 
> Agree.
> 
> > Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>
> 
> Reviewed-by: John Fastabend <john.fastabend@gmail.com>
> 
> > 
> > You might want some extra tags:
> > 
> > Link: https://lore.kernel.org/all/20200224140131.461979697@linutronix.de/
> > Fixes: 99ba2b5aba24 ("bpf: sockhash, disallow bpf_tcp_close and update in parallel")

ENOCOFFEE here (which is never an excuse, but at least today is really
true), but I considered you may want this patch via the ebpf tree only
after applying it to net.

Hopefully should not be tragic, but please let me know if you prefer
the change reverted from net and going via the other path.

Thanks!

Paolo