netdev - Re: [PATCH net] bpf: sockmap: Remove preempt_disable in sock_map_sk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 01 Aug 2023 09:44:23 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: John Fastabend <john.fastabend@...il.com>, Jakub Sitnicki
 <jakub@...udflare.com>, Alexei Starovoitov <ast@...nel.org>, Daniel
 Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>,
 Martin KaFai Lau <martin.lau@...ux.dev>
Cc: linux-kernel@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com, 
 kuba@...nel.org, netdev@...r.kernel.org, bpf@...r.kernel.org,
 tglozar@...hat.com
Subject: Re: [PATCH net] bpf: sockmap: Remove preempt_disable in
 sock_map_sk_acquire

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@...hat.com wrote:
> > > From: Tomas Glozar <tglozar@...hat.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@...hat.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@...udflare.com>
> 
> Reviewed-by: John Fastabend <john.fastabend@...il.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