[PATCH net] flow_dissector: disable preemption around BPF calls

[PATCH net] flow_dissector: disable preemption around BPF calls

[Eric Dumazet]

Various things in eBPF really require us to disable preemption
before running an eBPF program.

syzbot reported :

BUG: assuming atomic context at net/core/flow_dissector.c:737
in_atomic(): 0, irqs_disabled(): 0, pid: 24710, name: syz-executor.3
2 locks held by syz-executor.3/24710:
 #0: 00000000e81a4bf1 (&tfile->napi_mutex){+.+.}, at: tun_get_user+0x168e/0x3ff0 drivers/net/tun.c:1850
 #1: 00000000254afebd (rcu_read_lock){....}, at: __skb_flow_dissect+0x1e1/0x4bb0 net/core/flow_dissector.c:822
CPU: 1 PID: 24710 Comm: syz-executor.3 Not tainted 5.1.0+ #6
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x172/0x1f0 lib/dump_stack.c:113
 __cant_sleep kernel/sched/core.c:6165 [inline]
 __cant_sleep.cold+0xa3/0xbb kernel/sched/core.c:6142
 bpf_flow_dissect+0xfe/0x390 net/core/flow_dissector.c:737
 __skb_flow_dissect+0x362/0x4bb0 net/core/flow_dissector.c:853
 skb_flow_dissect_flow_keys_basic include/linux/skbuff.h:1322 [inline]
 skb_probe_transport_header include/linux/skbuff.h:2500 [inline]
 skb_probe_transport_header include/linux/skbuff.h:2493 [inline]
 tun_get_user+0x2cfe/0x3ff0 drivers/net/tun.c:1940
 tun_chr_write_iter+0xbd/0x156 drivers/net/tun.c:2037
 call_write_iter include/linux/fs.h:1872 [inline]
 do_iter_readv_writev+0x5fd/0x900 fs/read_write.c:693
 do_iter_write fs/read_write.c:970 [inline]
 do_iter_write+0x184/0x610 fs/read_write.c:951
 vfs_writev+0x1b3/0x2f0 fs/read_write.c:1015
 do_writev+0x15b/0x330 fs/read_write.c:1058
 __do_sys_writev fs/read_write.c:1131 [inline]
 __se_sys_writev fs/read_write.c:1128 [inline]
 __x64_sys_writev+0x75/0xb0 fs/read_write.c:1128
 do_syscall_64+0x103/0x670 arch/x86/entry/common.c:298
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

Fixes: d58e468b1112 ("flow_dissector: implements flow dissector BPF hook")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: syzbot <syzkaller@googlegroups.com>
Cc: Petar Penkov <ppenkov@google.com>
Cc: Stanislav Fomichev <sdf@google.com>
---
 net/core/flow_dissector.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 9ca784c592ac8c9c58282289a81889fbe4658a9e..548f39dde30711ac5be9e921993a6d8e53f74161 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -734,7 +734,9 @@ bool bpf_flow_dissect(struct bpf_prog *prog, struct bpf_flow_dissector *ctx,
 	flow_keys->nhoff = nhoff;
 	flow_keys->thoff = flow_keys->nhoff;
 
+	preempt_disable();
 	result = BPF_PROG_RUN(prog, ctx);
+	preempt_enable();
 
 	flow_keys->nhoff = clamp_t(u16, flow_keys->nhoff, nhoff, hlen);
 	flow_keys->thoff = clamp_t(u16, flow_keys->thoff,
-- 
2.21.0.1020.gf2820cf01a-goog

Re: [PATCH net] flow_dissector: disable preemption around BPF calls

[David Miller]

From: Eric Dumazet <edumazet@google.com>
Date: Mon, 13 May 2019 09:38:55 -0700

> Various things in eBPF really require us to disable preemption
> before running an eBPF program.
> 
> syzbot reported :
. ..
> Fixes: d58e468b1112 ("flow_dissector: implements flow dissector BPF hook")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: syzbot <syzkaller@googlegroups.com>

Applied and queued up for -stable.

Re: [PATCH net] flow_dissector: disable preemption around BPF calls

[Stanislav Fomichev]

On 05/13, Eric Dumazet wrote:
> Various things in eBPF really require us to disable preemption
> before running an eBPF program.
> 
> syzbot reported :
> 
> BUG: assuming atomic context at net/core/flow_dissector.c:737
> in_atomic(): 0, irqs_disabled(): 0, pid: 24710, name: syz-executor.3
> 2 locks held by syz-executor.3/24710:
>  #0: 00000000e81a4bf1 (&tfile->napi_mutex){+.+.}, at: tun_get_user+0x168e/0x3ff0 drivers/net/tun.c:1850
>  #1: 00000000254afebd (rcu_read_lock){....}, at: __skb_flow_dissect+0x1e1/0x4bb0 net/core/flow_dissector.c:822
> CPU: 1 PID: 24710 Comm: syz-executor.3 Not tainted 5.1.0+ #6
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Call Trace:
>  __dump_stack lib/dump_stack.c:77 [inline]
>  dump_stack+0x172/0x1f0 lib/dump_stack.c:113
>  __cant_sleep kernel/sched/core.c:6165 [inline]
>  __cant_sleep.cold+0xa3/0xbb kernel/sched/core.c:6142
>  bpf_flow_dissect+0xfe/0x390 net/core/flow_dissector.c:737
>  __skb_flow_dissect+0x362/0x4bb0 net/core/flow_dissector.c:853
>  skb_flow_dissect_flow_keys_basic include/linux/skbuff.h:1322 [inline]
>  skb_probe_transport_header include/linux/skbuff.h:2500 [inline]
>  skb_probe_transport_header include/linux/skbuff.h:2493 [inline]
>  tun_get_user+0x2cfe/0x3ff0 drivers/net/tun.c:1940
>  tun_chr_write_iter+0xbd/0x156 drivers/net/tun.c:2037
>  call_write_iter include/linux/fs.h:1872 [inline]
>  do_iter_readv_writev+0x5fd/0x900 fs/read_write.c:693
>  do_iter_write fs/read_write.c:970 [inline]
>  do_iter_write+0x184/0x610 fs/read_write.c:951
>  vfs_writev+0x1b3/0x2f0 fs/read_write.c:1015
>  do_writev+0x15b/0x330 fs/read_write.c:1058
>  __do_sys_writev fs/read_write.c:1131 [inline]
>  __se_sys_writev fs/read_write.c:1128 [inline]
>  __x64_sys_writev+0x75/0xb0 fs/read_write.c:1128
>  do_syscall_64+0x103/0x670 arch/x86/entry/common.c:298
>  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> 
> Fixes: d58e468b1112 ("flow_dissector: implements flow dissector BPF hook")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Cc: Petar Penkov <ppenkov@google.com>
> Cc: Stanislav Fomichev <sdf@google.com>
> ---
>  net/core/flow_dissector.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 9ca784c592ac8c9c58282289a81889fbe4658a9e..548f39dde30711ac5be9e921993a6d8e53f74161 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -734,7 +734,9 @@ bool bpf_flow_dissect(struct bpf_prog *prog, struct bpf_flow_dissector *ctx,
>  	flow_keys->nhoff = nhoff;
>  	flow_keys->thoff = flow_keys->nhoff;
>  
> +	preempt_disable();
>  	result = BPF_PROG_RUN(prog, ctx);
> +	preempt_enable();
>  
>  	flow_keys->nhoff = clamp_t(u16, flow_keys->nhoff, nhoff, hlen);
>  	flow_keys->thoff = clamp_t(u16, flow_keys->thoff,
> -- 
> 2.21.0.1020.gf2820cf01a-goog
> 
Reviewed-by: Stanislav Fomichev <sdf@google.com>

Thanks!

Re: [PATCH net] flow_dissector: disable preemption around BPF calls

[Mark Rutland]

On Mon, May 13, 2019 at 09:38:55AM -0700, 'Eric Dumazet' via syzkaller wrote:
> Various things in eBPF really require us to disable preemption
> before running an eBPF program.

Is that true for all eBPF uses? I note that we don't disable preemption
in the lib/test_bpf.c module, for example.

If it's a general requirement, perhaps it's worth an assertion within
BPF_PROG_RUN()?

Thanks,
Mark.

> 
> syzbot reported :
> 
> BUG: assuming atomic context at net/core/flow_dissector.c:737
> in_atomic(): 0, irqs_disabled(): 0, pid: 24710, name: syz-executor.3
> 2 locks held by syz-executor.3/24710:
>  #0: 00000000e81a4bf1 (&tfile->napi_mutex){+.+.}, at: tun_get_user+0x168e/0x3ff0 drivers/net/tun.c:1850
>  #1: 00000000254afebd (rcu_read_lock){....}, at: __skb_flow_dissect+0x1e1/0x4bb0 net/core/flow_dissector.c:822
> CPU: 1 PID: 24710 Comm: syz-executor.3 Not tainted 5.1.0+ #6
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Call Trace:
>  __dump_stack lib/dump_stack.c:77 [inline]
>  dump_stack+0x172/0x1f0 lib/dump_stack.c:113
>  __cant_sleep kernel/sched/core.c:6165 [inline]
>  __cant_sleep.cold+0xa3/0xbb kernel/sched/core.c:6142
>  bpf_flow_dissect+0xfe/0x390 net/core/flow_dissector.c:737
>  __skb_flow_dissect+0x362/0x4bb0 net/core/flow_dissector.c:853
>  skb_flow_dissect_flow_keys_basic include/linux/skbuff.h:1322 [inline]
>  skb_probe_transport_header include/linux/skbuff.h:2500 [inline]
>  skb_probe_transport_header include/linux/skbuff.h:2493 [inline]
>  tun_get_user+0x2cfe/0x3ff0 drivers/net/tun.c:1940
>  tun_chr_write_iter+0xbd/0x156 drivers/net/tun.c:2037
>  call_write_iter include/linux/fs.h:1872 [inline]
>  do_iter_readv_writev+0x5fd/0x900 fs/read_write.c:693
>  do_iter_write fs/read_write.c:970 [inline]
>  do_iter_write+0x184/0x610 fs/read_write.c:951
>  vfs_writev+0x1b3/0x2f0 fs/read_write.c:1015
>  do_writev+0x15b/0x330 fs/read_write.c:1058
>  __do_sys_writev fs/read_write.c:1131 [inline]
>  __se_sys_writev fs/read_write.c:1128 [inline]
>  __x64_sys_writev+0x75/0xb0 fs/read_write.c:1128
>  do_syscall_64+0x103/0x670 arch/x86/entry/common.c:298
>  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> 
> Fixes: d58e468b1112 ("flow_dissector: implements flow dissector BPF hook")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Cc: Petar Penkov <ppenkov@google.com>
> Cc: Stanislav Fomichev <sdf@google.com>
> ---
>  net/core/flow_dissector.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 9ca784c592ac8c9c58282289a81889fbe4658a9e..548f39dde30711ac5be9e921993a6d8e53f74161 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -734,7 +734,9 @@ bool bpf_flow_dissect(struct bpf_prog *prog, struct bpf_flow_dissector *ctx,
>  	flow_keys->nhoff = nhoff;
>  	flow_keys->thoff = flow_keys->nhoff;
>  
> +	preempt_disable();
>  	result = BPF_PROG_RUN(prog, ctx);
> +	preempt_enable();
>  
>  	flow_keys->nhoff = clamp_t(u16, flow_keys->nhoff, nhoff, hlen);
>  	flow_keys->thoff = clamp_t(u16, flow_keys->thoff,
> -- 
> 2.21.0.1020.gf2820cf01a-goog
> 
> -- 
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/20190513163855.225489-1-edumazet%40google.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH net] flow_dissector: disable preemption around BPF calls

[Eric Dumazet]

On Mon, May 13, 2019 at 10:17 AM Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Mon, May 13, 2019 at 09:38:55AM -0700, 'Eric Dumazet' via syzkaller wrote:
> > Various things in eBPF really require us to disable preemption
> > before running an eBPF program.
>
> Is that true for all eBPF uses? I note that we don't disable preemption
> in the lib/test_bpf.c module, for example.
>
> If it's a general requirement, perhaps it's worth an assertion within
> BPF_PROG_RUN()?


The assertion is already there :)

This is how syzbot triggered the report.

Re: [PATCH net] flow_dissector: disable preemption around BPF calls

[Mark Rutland]

On Mon, May 13, 2019 at 10:20:19AM -0700, 'Eric Dumazet' via syzkaller wrote:
> On Mon, May 13, 2019 at 10:17 AM Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > On Mon, May 13, 2019 at 09:38:55AM -0700, 'Eric Dumazet' via syzkaller wrote:
> > > Various things in eBPF really require us to disable preemption
> > > before running an eBPF program.
> >
> > Is that true for all eBPF uses? I note that we don't disable preemption
> > in the lib/test_bpf.c module, for example.
> >
> > If it's a general requirement, perhaps it's worth an assertion within
> > BPF_PROG_RUN()?
> 
> The assertion is already there :)
> 
> This is how syzbot triggered the report.

Ah! :)

I also see I'm wrong about test_bpf.c, so sorry for the noise on both
counts!

Thanks,
Mark.

Re: [PATCH net] flow_dissector: disable preemption around BPF calls

[Eric Dumazet]

On 5/13/19 10:25 AM, Mark Rutland wrote:
> On Mon, May 13, 2019 at 10:20:19AM -0700, 'Eric Dumazet' via syzkaller wrote:
>> On Mon, May 13, 2019 at 10:17 AM Mark Rutland <mark.rutland@arm.com> wrote:
>>>
>>> On Mon, May 13, 2019 at 09:38:55AM -0700, 'Eric Dumazet' via syzkaller wrote:
>>>> Various things in eBPF really require us to disable preemption
>>>> before running an eBPF program.
>>>
>>> Is that true for all eBPF uses? I note that we don't disable preemption
>>> in the lib/test_bpf.c module, for example.
>>>
>>> If it's a general requirement, perhaps it's worth an assertion within
>>> BPF_PROG_RUN()?
>>
>> The assertion is already there :)
>>
>> This is how syzbot triggered the report.
> 
> Ah! :)
> 
> I also see I'm wrong about test_bpf.c, so sorry for the noise on both
> counts!

No worries, thanks for reviewing !