Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <19a3a2be3c2389e97cacd7e7ab93b317b016ef94.camel@redhat.com>
Subject: Re: [PATCH net] bpf: sockmap: Remove preempt_disable in
 sock_map_sk_acquire
From:   Paolo Abeni <pabeni@redhat.com>
To:     John Fastabend <john.fastabend@gmail.com>,
        Jakub Sitnicki <jakub@cloudflare.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <martin.lau@linux.dev>
Cc:     linux-kernel@vger.kernel.org, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, netdev@vger.kernel.org,
        bpf@vger.kernel.org, tglozar@redhat.com
Date:   Tue, 01 Aug 2023 09:44:23 +0200
In-Reply-To: <64c882fd8c200_a427920843@john.notmuch>
References: <20230728064411.305576-1-tglozar@redhat.com>
         <87ila0fn01.fsf@cloudflare.com> <64c882fd8c200_a427920843@john.notmuch>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Mon, 2023-07-31 at 20:58 -0700, John Fastabend wrote:
> Jakub Sitnicki wrote:
> >=20
> > On Fri, Jul 28, 2023 at 08:44 AM +02, tglozar@redhat.com wrote:
> > > From: Tomas Glozar <tglozar@redhat.com>
> > >=20
> > > 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-exi=
st
> > > patchset notes for details).
> > >=20
> > > This causes calling bpf_map_update_elem on BPF_MAP_TYPE_SOCKMAP maps =
to
> > > BUG (sleeping function called from invalid context) on RT kernels.
> > >=20
> > > preempt_disable was introduced together with lock_sk and rcu_read_loc=
k
> > > in commit 99ba2b5aba24e ("bpf: sockhash, disallow bpf_tcp_close and u=
pdate
> > > in parallel"), probably to match disabled migration of BPF programs, =
and
> > > is no longer necessary.
> > >=20
> > > Remove preempt_disable to fix BUG in sock_map_update_common on RT.
> > >=20
> > > Signed-off-by: Tomas Glozar <tglozar@redhat.com>
> > > ---
> >=20
> > We disable softirq and hold a spin lock when modifying the map/hash in
> > sock_{map,hash}_update_common so this LGTM:
> >=20
>=20
> Agree.
>=20
> > Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>
>=20
> Reviewed-by: John Fastabend <john.fastabend@gmail.com>
>=20
> >=20
> > You might want some extra tags:
> >=20
> > Link: https://lore.kernel.org/all/20200224140131.461979697@linutronix.d=
e/
> > 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