2023-07-26 09:53:17

by Tomas Glozar

Subject: [PATCH RFC net] bpf: sockmap: Remove preempt_disable in sock_map_sk_acquire

From: Tomas Glozar <[email protected]>

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 <[email protected]>
---
net/core/sock_map.c | 2 --
1 file changed, 2 deletions(-)

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

2023-07-27 20:49:07

by John Fastabend

Subject: RE: [PATCH RFC net] bpf: sockmap: Remove preempt_disable in sock_map_sk_acquire

tglozar@ wrote:
> From: Tomas Glozar <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
>