All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH bpf-next] bpf: extend BTF_ID_LIST_GLOBAL with parameter for number of IDs
@ 2021-11-10 17:44 Song Liu
  2021-11-10 17:48 ` Eric Dumazet
  2021-11-10 22:02 ` Alexei Starovoitov
  0 siblings, 2 replies; 6+ messages in thread
From: Song Liu @ 2021-11-10 17:44 UTC (permalink / raw)
  To: bpf, netdev
  Cc: ast, daniel, andrii, kernel-team, Song Liu,
	syzbot+e0d81ec552a21d9071aa, Eric Dumazet

syzbot reported the following BUG w/o CONFIG_DEBUG_INFO_BTF

BUG: KASAN: global-out-of-bounds in task_iter_init+0x212/0x2e7 kernel/bpf/task_iter.c:661
Read of size 4 at addr ffffffff90297404 by task swapper/0/1

CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.15.0-syzkaller #0
Hardware name: ... Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
print_address_description.constprop.0.cold+0xf/0x309 mm/kasan/report.c:256
__kasan_report mm/kasan/report.c:442 [inline]
kasan_report.cold+0x83/0xdf mm/kasan/report.c:459
task_iter_init+0x212/0x2e7 kernel/bpf/task_iter.c:661
do_one_initcall+0x103/0x650 init/main.c:1295
do_initcall_level init/main.c:1368 [inline]
do_initcalls init/main.c:1384 [inline]
do_basic_setup init/main.c:1403 [inline]
kernel_init_freeable+0x6b1/0x73a init/main.c:1606
kernel_init+0x1a/0x1d0 init/main.c:1497
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295
</TASK>

This is caused by hard-coded name[1] in BTF_ID_LIST_GLOBAL (w/o
CONFIG_DEBUG_INFO_BTF). Fix this by adding a parameter n to
BTF_ID_LIST_GLOBAL. This avoids ifdef CONFIG_DEBUG_INFO_BTF in btf.c and
filter.c.

Fixes: 7c7e3d31e785 ("bpf: Introduce helper bpf_find_vma")
Reported-by: syzbot+e0d81ec552a21d9071aa@syzkaller.appspotmail.com
Suggested-by: Eric Dumazet <edumazet@google.com>
Reported-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Song Liu <songliubraving@fb.com>
---
 include/linux/btf_ids.h | 6 +++---
 kernel/bpf/btf.c        | 2 +-
 net/core/filter.c       | 6 +-----
 3 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
index 47d9abfbdb556..6bb42b785293a 100644
--- a/include/linux/btf_ids.h
+++ b/include/linux/btf_ids.h
@@ -73,7 +73,7 @@ asm(							\
 __BTF_ID_LIST(name, local)				\
 extern u32 name[];
 
-#define BTF_ID_LIST_GLOBAL(name)			\
+#define BTF_ID_LIST_GLOBAL(name, n)			\
 __BTF_ID_LIST(name, globl)
 
 /* The BTF_ID_LIST_SINGLE macro defines a BTF_ID_LIST with
@@ -83,7 +83,7 @@ __BTF_ID_LIST(name, globl)
 	BTF_ID_LIST(name) \
 	BTF_ID(prefix, typename)
 #define BTF_ID_LIST_GLOBAL_SINGLE(name, prefix, typename) \
-	BTF_ID_LIST_GLOBAL(name) \
+	BTF_ID_LIST_GLOBAL(name, 1)			  \
 	BTF_ID(prefix, typename)
 
 /*
@@ -149,7 +149,7 @@ extern struct btf_id_set name;
 #define BTF_ID_LIST(name) static u32 name[5];
 #define BTF_ID(prefix, name)
 #define BTF_ID_UNUSED
-#define BTF_ID_LIST_GLOBAL(name) u32 name[1];
+#define BTF_ID_LIST_GLOBAL(name, n) u32 name[n];
 #define BTF_ID_LIST_SINGLE(name, prefix, typename) static u32 name[1];
 #define BTF_ID_LIST_GLOBAL_SINGLE(name, prefix, typename) u32 name[1];
 #define BTF_SET_START(name) static struct btf_id_set name = { 0 };
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index cdb0fba656006..1cd2750b4c88e 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6342,7 +6342,7 @@ const struct bpf_func_proto bpf_btf_find_by_name_kind_proto = {
 	.arg4_type	= ARG_ANYTHING,
 };
 
-BTF_ID_LIST_GLOBAL(btf_task_struct_ids)
+BTF_ID_LIST_GLOBAL(btf_task_struct_ids, 3)
 BTF_ID(struct, task_struct)
 BTF_ID(struct, file)
 BTF_ID(struct, vm_area_struct)
diff --git a/net/core/filter.c b/net/core/filter.c
index 8e8d3b49c2976..cf3253a0e43af 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -10604,14 +10604,10 @@ void bpf_prog_change_xdp(struct bpf_prog *prev_prog, struct bpf_prog *prog)
 	bpf_dispatcher_change_prog(BPF_DISPATCHER_PTR(xdp), prev_prog, prog);
 }
 
-#ifdef CONFIG_DEBUG_INFO_BTF
-BTF_ID_LIST_GLOBAL(btf_sock_ids)
+BTF_ID_LIST_GLOBAL(btf_sock_ids, MAX_BTF_SOCK_TYPE)
 #define BTF_SOCK_TYPE(name, type) BTF_ID(struct, type)
 BTF_SOCK_TYPE_xxx
 #undef BTF_SOCK_TYPE
-#else
-u32 btf_sock_ids[MAX_BTF_SOCK_TYPE];
-#endif
 
 BPF_CALL_1(bpf_skc_to_tcp6_sock, struct sock *, sk)
 {
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next] bpf: extend BTF_ID_LIST_GLOBAL with parameter for number of IDs
  2021-11-10 17:44 [PATCH bpf-next] bpf: extend BTF_ID_LIST_GLOBAL with parameter for number of IDs Song Liu
@ 2021-11-10 17:48 ` Eric Dumazet
  2021-11-10 22:02 ` Alexei Starovoitov
  1 sibling, 0 replies; 6+ messages in thread
From: Eric Dumazet @ 2021-11-10 17:48 UTC (permalink / raw)
  To: Song Liu
  Cc: bpf, netdev, ast, daniel, andrii, Kernel-team,
	syzbot+e0d81ec552a21d9071aa

On Wed, Nov 10, 2021 at 9:45 AM Song Liu <songliubraving@fb.com> wrote:
>
> syzbot reported the following BUG w/o CONFIG_DEBUG_INFO_BTF
>
> BUG: KASAN: global-out-of-bounds in task_iter_init+0x212/0x2e7 kernel/bpf/task_iter.c:661
> Read of size 4 at addr ffffffff90297404 by task swapper/0/1
>
...
>
> This is caused by hard-coded name[1] in BTF_ID_LIST_GLOBAL (w/o
> CONFIG_DEBUG_INFO_BTF). Fix this by adding a parameter n to
> BTF_ID_LIST_GLOBAL. This avoids ifdef CONFIG_DEBUG_INFO_BTF in btf.c and
> filter.c.
>
> Fixes: 7c7e3d31e785 ("bpf: Introduce helper bpf_find_vma")
> Reported-by: syzbot+e0d81ec552a21d9071aa@syzkaller.appspotmail.com
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---
>  include/linux/btf_ids.h | 6 +++---
>  kernel/bpf/btf.c        | 2 +-
>  net/core/filter.c       | 6 +-----
>  3 files changed, 5 insertions(+), 9 deletions(-)

SGTM, thanks !

Reviewed-by: Eric Dumazet <edumazet@google.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next] bpf: extend BTF_ID_LIST_GLOBAL with parameter for number of IDs
  2021-11-10 17:44 [PATCH bpf-next] bpf: extend BTF_ID_LIST_GLOBAL with parameter for number of IDs Song Liu
  2021-11-10 17:48 ` Eric Dumazet
@ 2021-11-10 22:02 ` Alexei Starovoitov
  2021-11-10 22:11   ` Song Liu
  1 sibling, 1 reply; 6+ messages in thread
From: Alexei Starovoitov @ 2021-11-10 22:02 UTC (permalink / raw)
  To: Song Liu
  Cc: bpf, Network Development, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Kernel Team, syzbot+e0d81ec552a21d9071aa,
	Eric Dumazet

On Wed, Nov 10, 2021 at 9:47 AM Song Liu <songliubraving@fb.com> wrote:
>
> -#ifdef CONFIG_DEBUG_INFO_BTF
> -BTF_ID_LIST_GLOBAL(btf_sock_ids)
> +BTF_ID_LIST_GLOBAL(btf_sock_ids, MAX_BTF_SOCK_TYPE)
>  #define BTF_SOCK_TYPE(name, type) BTF_ID(struct, type)
>  BTF_SOCK_TYPE_xxx
>  #undef BTF_SOCK_TYPE
> -#else
> -u32 btf_sock_ids[MAX_BTF_SOCK_TYPE];
> -#endif

If we're trying to future proof it I think it would be better
to combine it with MAX_BTF_SOCK_TYPE and BTF_SOCK_TYPE_xxx macro.
(or have another macro that is tracing specific).
That will help avoid cryptic btf_task_struct_ids[0|1|2]
references in the code.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next] bpf: extend BTF_ID_LIST_GLOBAL with parameter for number of IDs
  2021-11-10 22:02 ` Alexei Starovoitov
@ 2021-11-10 22:11   ` Song Liu
  2021-11-10 22:13     ` Alexei Starovoitov
  0 siblings, 1 reply; 6+ messages in thread
From: Song Liu @ 2021-11-10 22:11 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: bpf, Network Development, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Kernel Team, syzbot+e0d81ec552a21d9071aa,
	Eric Dumazet



> On Nov 10, 2021, at 2:02 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> 
> On Wed, Nov 10, 2021 at 9:47 AM Song Liu <songliubraving@fb.com> wrote:
>> 
>> -#ifdef CONFIG_DEBUG_INFO_BTF
>> -BTF_ID_LIST_GLOBAL(btf_sock_ids)
>> +BTF_ID_LIST_GLOBAL(btf_sock_ids, MAX_BTF_SOCK_TYPE)
>> #define BTF_SOCK_TYPE(name, type) BTF_ID(struct, type)
>> BTF_SOCK_TYPE_xxx
>> #undef BTF_SOCK_TYPE
>> -#else
>> -u32 btf_sock_ids[MAX_BTF_SOCK_TYPE];
>> -#endif
> 
> If we're trying to future proof it I think it would be better
> to combine it with MAX_BTF_SOCK_TYPE and BTF_SOCK_TYPE_xxx macro.
> (or have another macro that is tracing specific).
> That will help avoid cryptic btf_task_struct_ids[0|1|2]
> references in the code.

Yeah, this makes sense. 

I am taking time off for tomorrow and Friday, so I probably won't 
have time to implement this before 5.16-rc1. How about we ship 
this fix as-is, and improve it later?

Thanks,
Song

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next] bpf: extend BTF_ID_LIST_GLOBAL with parameter for number of IDs
  2021-11-10 22:11   ` Song Liu
@ 2021-11-10 22:13     ` Alexei Starovoitov
  2021-11-10 22:18       ` Song Liu
  0 siblings, 1 reply; 6+ messages in thread
From: Alexei Starovoitov @ 2021-11-10 22:13 UTC (permalink / raw)
  To: Song Liu
  Cc: bpf, Network Development, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Kernel Team, syzbot+e0d81ec552a21d9071aa,
	Eric Dumazet

On Wed, Nov 10, 2021 at 2:11 PM Song Liu <songliubraving@fb.com> wrote:
>
>
>
> > On Nov 10, 2021, at 2:02 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> >
> > On Wed, Nov 10, 2021 at 9:47 AM Song Liu <songliubraving@fb.com> wrote:
> >>
> >> -#ifdef CONFIG_DEBUG_INFO_BTF
> >> -BTF_ID_LIST_GLOBAL(btf_sock_ids)
> >> +BTF_ID_LIST_GLOBAL(btf_sock_ids, MAX_BTF_SOCK_TYPE)
> >> #define BTF_SOCK_TYPE(name, type) BTF_ID(struct, type)
> >> BTF_SOCK_TYPE_xxx
> >> #undef BTF_SOCK_TYPE
> >> -#else
> >> -u32 btf_sock_ids[MAX_BTF_SOCK_TYPE];
> >> -#endif
> >
> > If we're trying to future proof it I think it would be better
> > to combine it with MAX_BTF_SOCK_TYPE and BTF_SOCK_TYPE_xxx macro.
> > (or have another macro that is tracing specific).
> > That will help avoid cryptic btf_task_struct_ids[0|1|2]
> > references in the code.
>
> Yeah, this makes sense.
>
> I am taking time off for tomorrow and Friday, so I probably won't
> have time to implement this before 5.16-rc1. How about we ship
> this fix as-is, and improve it later?

It's not rc1 material. It's in bpf-next only. There is no rush, I think.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next] bpf: extend BTF_ID_LIST_GLOBAL with parameter for number of IDs
  2021-11-10 22:13     ` Alexei Starovoitov
@ 2021-11-10 22:18       ` Song Liu
  0 siblings, 0 replies; 6+ messages in thread
From: Song Liu @ 2021-11-10 22:18 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: bpf, Network Development, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Kernel Team, syzbot+e0d81ec552a21d9071aa,
	Eric Dumazet



> On Nov 10, 2021, at 2:13 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> 
> On Wed, Nov 10, 2021 at 2:11 PM Song Liu <songliubraving@fb.com> wrote:
>> 
>> 
>> 
>>> On Nov 10, 2021, at 2:02 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>>> 
>>> On Wed, Nov 10, 2021 at 9:47 AM Song Liu <songliubraving@fb.com> wrote:
>>>> 
>>>> -#ifdef CONFIG_DEBUG_INFO_BTF
>>>> -BTF_ID_LIST_GLOBAL(btf_sock_ids)
>>>> +BTF_ID_LIST_GLOBAL(btf_sock_ids, MAX_BTF_SOCK_TYPE)
>>>> #define BTF_SOCK_TYPE(name, type) BTF_ID(struct, type)
>>>> BTF_SOCK_TYPE_xxx
>>>> #undef BTF_SOCK_TYPE
>>>> -#else
>>>> -u32 btf_sock_ids[MAX_BTF_SOCK_TYPE];
>>>> -#endif
>>> 
>>> If we're trying to future proof it I think it would be better
>>> to combine it with MAX_BTF_SOCK_TYPE and BTF_SOCK_TYPE_xxx macro.
>>> (or have another macro that is tracing specific).
>>> That will help avoid cryptic btf_task_struct_ids[0|1|2]
>>> references in the code.
>> 
>> Yeah, this makes sense.
>> 
>> I am taking time off for tomorrow and Friday, so I probably won't
>> have time to implement this before 5.16-rc1. How about we ship
>> this fix as-is, and improve it later?
> 
> It's not rc1 material. It's in bpf-next only. There is no rush, I think.

Aha, I guess I messed up the branches.

Song 

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-11-10 22:18 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-10 17:44 [PATCH bpf-next] bpf: extend BTF_ID_LIST_GLOBAL with parameter for number of IDs Song Liu
2021-11-10 17:48 ` Eric Dumazet
2021-11-10 22:02 ` Alexei Starovoitov
2021-11-10 22:11   ` Song Liu
2021-11-10 22:13     ` Alexei Starovoitov
2021-11-10 22:18       ` Song Liu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.