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;
Date:   Thu, 27 Jul 2023 13:15:22 -0700
From:   John Fastabend <john.fastabend@gmail.com>
To:     tglozar@redhat.com, linux-kernel@vger.kernel.org
Cc:     john.fastabend@gmail.com, jakub@cloudflare.com,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, netdev@vger.kernel.org, bpf@vger.kernel.org,
        Tomas Glozar <tglozar@redhat.com>
Message-ID: <64c2d05a894be_831d20896@john.notmuch>
In-Reply-To: <20230726085003.261112-1-tglozar@redhat.com>
References: <20230726085003.261112-1-tglozar@redhat.com>
Subject: RE: [PATCH RFC net] bpf: sockmap: Remove preempt_disable in
 sock_map_sk_acquire
Mime-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

tglozar@ 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") with no comment on why it is necessary.
> 
> Remove preempt_disable to fix BUG in sock_map_update_common on RT.
> 
> Signed-off-by: Tomas Glozar <tglozar@redhat.com>
> ---
>  net/core/sock_map.c | 2 --
>  1 file changed, 2 deletions(-)

Hi Thomas,

I looked at this and don't see the need for the preempt_disable there
at the moment. I believe it was there simply to match the BPF caller
case where it was preempt_disable at the time and now is migrate
disable. From the BPF side we don't want the migrate, but from the
syscall interface it should be OK.

Can you submit without RFC tag I think this should be OK.

Thanks,
John

> 
> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index 19538d628714..08ab108206bf 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -115,7 +115,6 @@ static void sock_map_sk_acquire(struct sock *sk)
>  	__acquires(&sk->sk_lock.slock)
>  {
>  	lock_sock(sk);
> -	preempt_disable();
>  	rcu_read_lock();
>  }
>  
> @@ -123,7 +122,6 @@ static void sock_map_sk_release(struct sock *sk)
>  	__releases(&sk->sk_lock.slock)
>  {
>  	rcu_read_unlock();
> -	preempt_enable();
>  	release_sock(sk);
>  }
>  
> 
> base-commit: 22117b3ae6e37d07225653d9ae5ae86b3a54f99c
> -- 
> 2.39.3
>