| Message ID | 1666934520-22509-1-git-send-email-wangyufen@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [net,v2] bpf: Fix memory leaks in __check_func_call | expand |
On Thu, Oct 27, 2022 at 10:01 PM Wang Yufen <wangyufen@huawei.com> wrote: > > kmemleak reports this issue: > > unreferenced object 0xffff88817139d000 (size 2048): > comm "test_progs", pid 33246, jiffies 4307381979 (age 45851.820s) > hex dump (first 32 bytes): > 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > backtrace: > [<0000000045f075f0>] kmalloc_trace+0x27/0xa0 > [<0000000098b7c90a>] __check_func_call+0x316/0x1230 > [<00000000b4c3c403>] check_helper_call+0x172e/0x4700 > [<00000000aa3875b7>] do_check+0x21d8/0x45e0 > [<000000001147357b>] do_check_common+0x767/0xaf0 > [<00000000b5a595b4>] bpf_check+0x43e3/0x5bc0 > [<0000000011e391b1>] bpf_prog_load+0xf26/0x1940 > [<0000000007f765c0>] __sys_bpf+0xd2c/0x3650 > [<00000000839815d6>] __x64_sys_bpf+0x75/0xc0 > [<00000000946ee250>] do_syscall_64+0x3b/0x90 > [<0000000000506b7f>] entry_SYSCALL_64_after_hwframe+0x63/0xcd > > The root case here is: In function prepare_func_exit(), the callee is > not released in the abnormal scenario after "state->curframe--;". To > fix, move "state->curframe--;" to the very bottom of the function, > right when we free callee and reset frame[] pointer to NULL, as Andrii > suggested. > > In addition, function __check_func_call() has a similar problem. In > the abnormal scenario before "state->curframe++;", the callee is alse > not released. > > Fixes: 69c087ba6225 ("bpf: Add bpf_for_each_map_elem() helper") > Fixes: fd978bf7fd31 ("bpf: Add reference tracking to verifier") > Signed-off-by: Wang Yufen <wangyufen@huawei.com> > --- This change seems to be breaking BPF selftests quite badly, please check what's going on ([0]): [0] https://github.com/kernel-patches/bpf/actions/runs/3379444311/jobs/5611599540 > kernel/bpf/verifier.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 014ee09..d28d460 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -6736,11 +6736,11 @@ static int __check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn > /* Transfer references to the callee */ > err = copy_reference_state(callee, caller); > if (err) > - return err; > + goto err_out; > > err = set_callee_state_cb(env, caller, callee, *insn_idx); > if (err) > - return err; > + goto err_out; > > clear_caller_saved_regs(env, caller->regs); > > @@ -6757,6 +6757,11 @@ static int __check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn > print_verifier_state(env, callee, true); > } > return 0; > + > +err_out: > + kfree(callee); > + state->frame[state->curframe + 1] = NULL; > + return err; > } > > int map_set_for_each_callback_args(struct bpf_verifier_env *env, > @@ -6969,7 +6974,6 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx) > return -EINVAL; > } > > - state->curframe--; > caller = state->frame[state->curframe]; > if (callee->in_callback_fn) { > /* enforce R0 return value range [0, 1]. */ > @@ -7000,6 +7004,7 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx) > return err; > } > > + state->curframe--; > *insn_idx = callee->callsite + 1; > if (env->log.level & BPF_LOG_LEVEL) { > verbose(env, "returning from callee:\n"); > -- > 1.8.3.1 >
On Wed, Nov 2, 2022 at 12:05 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Thu, Oct 27, 2022 at 10:01 PM Wang Yufen <wangyufen@huawei.com> wrote: > > > > kmemleak reports this issue: > > > > unreferenced object 0xffff88817139d000 (size 2048): > > comm "test_progs", pid 33246, jiffies 4307381979 (age 45851.820s) > > hex dump (first 32 bytes): > > 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > > backtrace: > > [<0000000045f075f0>] kmalloc_trace+0x27/0xa0 > > [<0000000098b7c90a>] __check_func_call+0x316/0x1230 > > [<00000000b4c3c403>] check_helper_call+0x172e/0x4700 > > [<00000000aa3875b7>] do_check+0x21d8/0x45e0 > > [<000000001147357b>] do_check_common+0x767/0xaf0 > > [<00000000b5a595b4>] bpf_check+0x43e3/0x5bc0 > > [<0000000011e391b1>] bpf_prog_load+0xf26/0x1940 > > [<0000000007f765c0>] __sys_bpf+0xd2c/0x3650 > > [<00000000839815d6>] __x64_sys_bpf+0x75/0xc0 > > [<00000000946ee250>] do_syscall_64+0x3b/0x90 > > [<0000000000506b7f>] entry_SYSCALL_64_after_hwframe+0x63/0xcd > > > > The root case here is: In function prepare_func_exit(), the callee is > > not released in the abnormal scenario after "state->curframe--;". To > > fix, move "state->curframe--;" to the very bottom of the function, > > right when we free callee and reset frame[] pointer to NULL, as Andrii > > suggested. > > > > In addition, function __check_func_call() has a similar problem. In > > the abnormal scenario before "state->curframe++;", the callee is alse > > not released. > > > > Fixes: 69c087ba6225 ("bpf: Add bpf_for_each_map_elem() helper") > > Fixes: fd978bf7fd31 ("bpf: Add reference tracking to verifier") > > Signed-off-by: Wang Yufen <wangyufen@huawei.com> > > --- > > This change seems to be breaking BPF selftests quite badly, please > check what's going on ([0]): > > [0] https://github.com/kernel-patches/bpf/actions/runs/3379444311/jobs/5611599540 > And also please target it against bpf tree: [PATCH bpf], not net tree. > > kernel/bpf/verifier.c | 11 ++++++++--- > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 014ee09..d28d460 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -6736,11 +6736,11 @@ static int __check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn > > /* Transfer references to the callee */ > > err = copy_reference_state(callee, caller); > > if (err) > > - return err; > > + goto err_out; > > > > err = set_callee_state_cb(env, caller, callee, *insn_idx); > > if (err) > > - return err; > > + goto err_out; > > > > clear_caller_saved_regs(env, caller->regs); > > > > @@ -6757,6 +6757,11 @@ static int __check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn > > print_verifier_state(env, callee, true); > > } > > return 0; > > + > > +err_out: > > + kfree(callee); > > + state->frame[state->curframe + 1] = NULL; > > + return err; > > } > > > > int map_set_for_each_callback_args(struct bpf_verifier_env *env, > > @@ -6969,7 +6974,6 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx) > > return -EINVAL; > > } > > > > - state->curframe--; > > caller = state->frame[state->curframe]; > > if (callee->in_callback_fn) { > > /* enforce R0 return value range [0, 1]. */ > > @@ -7000,6 +7004,7 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx) > > return err; > > } > > > > + state->curframe--; > > *insn_idx = callee->callsite + 1; > > if (env->log.level & BPF_LOG_LEVEL) { > > verbose(env, "returning from callee:\n"); > > -- > > 1.8.3.1 > >
在 2022/11/3 3:05, Andrii Nakryiko 写道: > On Thu, Oct 27, 2022 at 10:01 PM Wang Yufen <wangyufen@huawei.com> wrote: >> kmemleak reports this issue: >> >> unreferenced object 0xffff88817139d000 (size 2048): >> comm "test_progs", pid 33246, jiffies 4307381979 (age 45851.820s) >> hex dump (first 32 bytes): >> 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ >> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ >> backtrace: >> [<0000000045f075f0>] kmalloc_trace+0x27/0xa0 >> [<0000000098b7c90a>] __check_func_call+0x316/0x1230 >> [<00000000b4c3c403>] check_helper_call+0x172e/0x4700 >> [<00000000aa3875b7>] do_check+0x21d8/0x45e0 >> [<000000001147357b>] do_check_common+0x767/0xaf0 >> [<00000000b5a595b4>] bpf_check+0x43e3/0x5bc0 >> [<0000000011e391b1>] bpf_prog_load+0xf26/0x1940 >> [<0000000007f765c0>] __sys_bpf+0xd2c/0x3650 >> [<00000000839815d6>] __x64_sys_bpf+0x75/0xc0 >> [<00000000946ee250>] do_syscall_64+0x3b/0x90 >> [<0000000000506b7f>] entry_SYSCALL_64_after_hwframe+0x63/0xcd >> >> The root case here is: In function prepare_func_exit(), the callee is >> not released in the abnormal scenario after "state->curframe--;". To >> fix, move "state->curframe--;" to the very bottom of the function, >> right when we free callee and reset frame[] pointer to NULL, as Andrii >> suggested. >> >> In addition, function __check_func_call() has a similar problem. In >> the abnormal scenario before "state->curframe++;", the callee is alse >> not released. >> >> Fixes: 69c087ba6225 ("bpf: Add bpf_for_each_map_elem() helper") >> Fixes: fd978bf7fd31 ("bpf: Add reference tracking to verifier") >> Signed-off-by: Wang Yufen <wangyufen@huawei.com> >> --- > This change seems to be breaking BPF selftests quite badly, please > check what's going on ([0]): > > [0] https://github.com/kernel-patches/bpf/actions/runs/3379444311/jobs/5611599540 > >> kernel/bpf/verifier.c | 11 ++++++++--- >> 1 file changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >> index 014ee09..d28d460 100644 >> --- a/kernel/bpf/verifier.c >> +++ b/kernel/bpf/verifier.c >> @@ -6736,11 +6736,11 @@ static int __check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn >> /* Transfer references to the callee */ >> err = copy_reference_state(callee, caller); >> if (err) >> - return err; >> + goto err_out; >> >> err = set_callee_state_cb(env, caller, callee, *insn_idx); >> if (err) >> - return err; >> + goto err_out; >> >> clear_caller_saved_regs(env, caller->regs); >> >> @@ -6757,6 +6757,11 @@ static int __check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn >> print_verifier_state(env, callee, true); >> } >> return 0; >> + >> +err_out: >> + kfree(callee); >> + state->frame[state->curframe + 1] = NULL; >> + return err; >> } >> >> int map_set_for_each_callback_args(struct bpf_verifier_env *env, >> @@ -6969,7 +6974,6 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx) >> return -EINVAL; >> } >> >> - state->curframe--; >> caller = state->frame[state->curframe]; Got the wrong “caller” here, will change to state->frame[state->curframe - 1] in v3 >> if (callee->in_callback_fn) { >> /* enforce R0 return value range [0, 1]. */ >> @@ -7000,6 +7004,7 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx) >> return err; >> } >> >> + state->curframe--; >> *insn_idx = callee->callsite + 1; >> if (env->log.level & BPF_LOG_LEVEL) { >> verbose(env, "returning from callee:\n"); >> -- >> 1.8.3.1 >>
在 2022/11/3 3:06, Andrii Nakryiko 写道: > On Wed, Nov 2, 2022 at 12:05 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: >> On Thu, Oct 27, 2022 at 10:01 PM Wang Yufen <wangyufen@huawei.com> wrote: >>> kmemleak reports this issue: >>> >>> unreferenced object 0xffff88817139d000 (size 2048): >>> comm "test_progs", pid 33246, jiffies 4307381979 (age 45851.820s) >>> hex dump (first 32 bytes): >>> 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ >>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ >>> backtrace: >>> [<0000000045f075f0>] kmalloc_trace+0x27/0xa0 >>> [<0000000098b7c90a>] __check_func_call+0x316/0x1230 >>> [<00000000b4c3c403>] check_helper_call+0x172e/0x4700 >>> [<00000000aa3875b7>] do_check+0x21d8/0x45e0 >>> [<000000001147357b>] do_check_common+0x767/0xaf0 >>> [<00000000b5a595b4>] bpf_check+0x43e3/0x5bc0 >>> [<0000000011e391b1>] bpf_prog_load+0xf26/0x1940 >>> [<0000000007f765c0>] __sys_bpf+0xd2c/0x3650 >>> [<00000000839815d6>] __x64_sys_bpf+0x75/0xc0 >>> [<00000000946ee250>] do_syscall_64+0x3b/0x90 >>> [<0000000000506b7f>] entry_SYSCALL_64_after_hwframe+0x63/0xcd >>> >>> The root case here is: In function prepare_func_exit(), the callee is >>> not released in the abnormal scenario after "state->curframe--;". To >>> fix, move "state->curframe--;" to the very bottom of the function, >>> right when we free callee and reset frame[] pointer to NULL, as Andrii >>> suggested. >>> >>> In addition, function __check_func_call() has a similar problem. In >>> the abnormal scenario before "state->curframe++;", the callee is alse >>> not released. >>> >>> Fixes: 69c087ba6225 ("bpf: Add bpf_for_each_map_elem() helper") >>> Fixes: fd978bf7fd31 ("bpf: Add reference tracking to verifier") >>> Signed-off-by: Wang Yufen <wangyufen@huawei.com> >>> --- >> This change seems to be breaking BPF selftests quite badly, please >> check what's going on ([0]): >> >> [0] https://github.com/kernel-patches/bpf/actions/runs/3379444311/jobs/5611599540 >> > And also please target it against bpf tree: [PATCH bpf], not net tree. OK， thanks！ > >>> kernel/bpf/verifier.c | 11 ++++++++--- >>> 1 file changed, 8 insertions(+), 3 deletions(-) >>> >>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >>> index 014ee09..d28d460 100644 >>> --- a/kernel/bpf/verifier.c >>> +++ b/kernel/bpf/verifier.c >>> @@ -6736,11 +6736,11 @@ static int __check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn >>> /* Transfer references to the callee */ >>> err = copy_reference_state(callee, caller); >>> if (err) >>> - return err; >>> + goto err_out; >>> >>> err = set_callee_state_cb(env, caller, callee, *insn_idx); >>> if (err) >>> - return err; >>> + goto err_out; >>> >>> clear_caller_saved_regs(env, caller->regs); >>> >>> @@ -6757,6 +6757,11 @@ static int __check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn >>> print_verifier_state(env, callee, true); >>> } >>> return 0; >>> + >>> +err_out: >>> + kfree(callee); >>> + state->frame[state->curframe + 1] = NULL; >>> + return err; >>> } >>> >>> int map_set_for_each_callback_args(struct bpf_verifier_env *env, >>> @@ -6969,7 +6974,6 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx) >>> return -EINVAL; >>> } >>> >>> - state->curframe--; >>> caller = state->frame[state->curframe]; >>> if (callee->in_callback_fn) { >>> /* enforce R0 return value range [0, 1]. */ >>> @@ -7000,6 +7004,7 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx) >>> return err; >>> } >>> >>> + state->curframe--; >>> *insn_idx = callee->callsite + 1; >>> if (env->log.level & BPF_LOG_LEVEL) { >>> verbose(env, "returning from callee:\n"); >>> -- >>> 1.8.3.1 >>>
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 014ee09..d28d460 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -6736,11 +6736,11 @@ static int __check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn /* Transfer references to the callee */ err = copy_reference_state(callee, caller); if (err) - return err; + goto err_out; err = set_callee_state_cb(env, caller, callee, *insn_idx); if (err) - return err; + goto err_out; clear_caller_saved_regs(env, caller->regs); @@ -6757,6 +6757,11 @@ static int __check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn print_verifier_state(env, callee, true); } return 0; + +err_out: + kfree(callee); + state->frame[state->curframe + 1] = NULL; + return err; } int map_set_for_each_callback_args(struct bpf_verifier_env *env, @@ -6969,7 +6974,6 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx) return -EINVAL; } - state->curframe--; caller = state->frame[state->curframe]; if (callee->in_callback_fn) { /* enforce R0 return value range [0, 1]. */ @@ -7000,6 +7004,7 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx) return err; } + state->curframe--; *insn_idx = callee->callsite + 1; if (env->log.level & BPF_LOG_LEVEL) { verbose(env, "returning from callee:\n");
kmemleak reports this issue: unreferenced object 0xffff88817139d000 (size 2048): comm "test_progs", pid 33246, jiffies 4307381979 (age 45851.820s) hex dump (first 32 bytes): 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [<0000000045f075f0>] kmalloc_trace+0x27/0xa0 [<0000000098b7c90a>] __check_func_call+0x316/0x1230 [<00000000b4c3c403>] check_helper_call+0x172e/0x4700 [<00000000aa3875b7>] do_check+0x21d8/0x45e0 [<000000001147357b>] do_check_common+0x767/0xaf0 [<00000000b5a595b4>] bpf_check+0x43e3/0x5bc0 [<0000000011e391b1>] bpf_prog_load+0xf26/0x1940 [<0000000007f765c0>] __sys_bpf+0xd2c/0x3650 [<00000000839815d6>] __x64_sys_bpf+0x75/0xc0 [<00000000946ee250>] do_syscall_64+0x3b/0x90 [<0000000000506b7f>] entry_SYSCALL_64_after_hwframe+0x63/0xcd The root case here is: In function prepare_func_exit(), the callee is not released in the abnormal scenario after "state->curframe--;". To fix, move "state->curframe--;" to the very bottom of the function, right when we free callee and reset frame[] pointer to NULL, as Andrii suggested. In addition, function __check_func_call() has a similar problem. In the abnormal scenario before "state->curframe++;", the callee is alse not released. Fixes: 69c087ba6225 ("bpf: Add bpf_for_each_map_elem() helper") Fixes: fd978bf7fd31 ("bpf: Add reference tracking to verifier") Signed-off-by: Wang Yufen <wangyufen@huawei.com> --- kernel/bpf/verifier.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)