* [PATCH bpf-next v2 0/2] Relax allowlist for open-coded css_task iter @ 2023-10-24 2:42 Chuyi Zhou 2023-10-24 2:42 ` [PATCH bpf-next v2 1/2] bpf: Relax allowlist for " Chuyi Zhou 2023-10-24 2:42 ` [PATCH bpf-next v2 2/2] selftests/bpf: Add test for css_task iter combining with cgroup iter Chuyi Zhou 0 siblings, 2 replies; 9+ messages in thread From: Chuyi Zhou @ 2023-10-24 2:42 UTC (permalink / raw) To: bpf; +Cc: ast, daniel, andrii, martin.lau, Chuyi Zhou Hi, The patchset aims to relax the allowlist for open-coded css_task iter suggested by Alexei[1]. Please see individual patches for more details. And comments are always welcome. Patch summary: * Patch #1: Relax the allowlist and let css_task iter can be used in bpf iters and any sleepable progs. * Patch #2: Add a test in cgroup_iters.c which demonstrates how css_task iters can be combined with cgroup iter. link[1]:https://lore.kernel.org/lkml/CAADnVQKafk_junRyE=-FVAik4hjTRDtThymYGEL8hGTuYoOGpA@mail.gmail.com/ --- Changes in v2: * Fix the incorrect logic in check_css_task_iter_allowlist. Use expected_attach_type to check whether we are using bpf_iters. * Link to v1:https://lore.kernel.org/bpf/20231022154527.229117-1-zhouchuyi@bytedance.com/T/#m946f9cde86b44a13265d9a44c5738a711eb578fd --- Chuyi Zhou (2): bpf: Relax allowlist for css_task iter selftests/bpf: Add test for css_task iter combining with cgroup iter kernel/bpf/verifier.c | 21 ++++++---- .../selftests/bpf/prog_tests/cgroup_iter.c | 33 +++++++++++++++ .../selftests/bpf/progs/iters_css_task.c | 41 +++++++++++++++++++ .../selftests/bpf/progs/iters_task_failure.c | 4 +- 4 files changed, 89 insertions(+), 10 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH bpf-next v2 1/2] bpf: Relax allowlist for css_task iter 2023-10-24 2:42 [PATCH bpf-next v2 0/2] Relax allowlist for open-coded css_task iter Chuyi Zhou @ 2023-10-24 2:42 ` Chuyi Zhou 2023-10-24 4:57 ` Alexei Starovoitov 2023-10-24 2:42 ` [PATCH bpf-next v2 2/2] selftests/bpf: Add test for css_task iter combining with cgroup iter Chuyi Zhou 1 sibling, 1 reply; 9+ messages in thread From: Chuyi Zhou @ 2023-10-24 2:42 UTC (permalink / raw) To: bpf; +Cc: ast, daniel, andrii, martin.lau, Chuyi Zhou The newly added open-coded css_task iter would try to hold the global css_set_lock in bpf_iter_css_task_new, so the bpf side has to be careful in where it allows to use this iter. The mainly concern is dead locking on css_set_lock. check_css_task_iter_allowlist() in verifier enforced css_task can only be used in bpf_lsm hooks and sleepable bpf_iter. This patch relax the allowlist for css_task iter. Any lsm and any iter (even non-sleepable) and any sleepable are safe since they would not hold the css_set_lock before entering BPF progs context. This patch also fixes the misused BPF_TRACE_ITER in check_css_task_iter_allowlist which compared bpf_prog_type with bpf_attach_type. Fixes: 9c66dc94b62ae ("bpf: Introduce css_task open-coded iterator kfuncs") Signed-off-by: Chuyi Zhou <zhouchuyi@bytedance.com> --- kernel/bpf/verifier.c | 21 ++++++++++++------- .../selftests/bpf/progs/iters_task_failure.c | 4 ++-- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index e9bc5d4a25a1..9f209adc4ccb 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -11088,18 +11088,23 @@ static int process_kf_arg_ptr_to_rbtree_node(struct bpf_verifier_env *env, &meta->arg_rbtree_root.field); } +/* + * css_task iter allowlist is needed to avoid dead locking on css_set_lock. + * LSM hooks and iters (both sleepable and non-sleepable) are safe. + * Any sleepable progs are also safe since bpf_check_attach_target() enforce + * them can only be attached to some specific hook points. + */ static bool check_css_task_iter_allowlist(struct bpf_verifier_env *env) { enum bpf_prog_type prog_type = resolve_prog_type(env->prog); - switch (prog_type) { - case BPF_PROG_TYPE_LSM: + if (prog_type == BPF_PROG_TYPE_LSM) return true; - case BPF_TRACE_ITER: - return env->prog->aux->sleepable; - default: - return false; - } + + if (env->prog->expected_attach_type == BPF_TRACE_ITER) + return true; + + return env->prog->aux->sleepable; } static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_arg_meta *meta, @@ -11357,7 +11362,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ case KF_ARG_PTR_TO_ITER: if (meta->func_id == special_kfunc_list[KF_bpf_iter_css_task_new]) { if (!check_css_task_iter_allowlist(env)) { - verbose(env, "css_task_iter is only allowed in bpf_lsm and bpf iter-s\n"); + verbose(env, "css_task_iter is only allowed in bpf_lsm, bpf_iter and sleepable progs\n"); return -EINVAL; } } diff --git a/tools/testing/selftests/bpf/progs/iters_task_failure.c b/tools/testing/selftests/bpf/progs/iters_task_failure.c index c3bf96a67dba..6b1588d70652 100644 --- a/tools/testing/selftests/bpf/progs/iters_task_failure.c +++ b/tools/testing/selftests/bpf/progs/iters_task_failure.c @@ -84,8 +84,8 @@ int BPF_PROG(iter_css_lock_and_unlock) return 0; } -SEC("?fentry.s/" SYS_PREFIX "sys_getpgid") -__failure __msg("css_task_iter is only allowed in bpf_lsm and bpf iter-s") +SEC("?fentry/" SYS_PREFIX "sys_getpgid") +__failure __msg("css_task_iter is only allowed in bpf_lsm, bpf_iter and sleepable progs") int BPF_PROG(iter_css_task_for_each) { u64 cg_id = bpf_get_current_cgroup_id(); -- 2.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next v2 1/2] bpf: Relax allowlist for css_task iter 2023-10-24 2:42 ` [PATCH bpf-next v2 1/2] bpf: Relax allowlist for " Chuyi Zhou @ 2023-10-24 4:57 ` Alexei Starovoitov 2023-10-24 5:52 ` Chuyi Zhou 0 siblings, 1 reply; 9+ messages in thread From: Alexei Starovoitov @ 2023-10-24 4:57 UTC (permalink / raw) To: Chuyi Zhou Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau On Mon, Oct 23, 2023 at 7:42 PM Chuyi Zhou <zhouchuyi@bytedance.com> wrote: > > The newly added open-coded css_task iter would try to hold the global > css_set_lock in bpf_iter_css_task_new, so the bpf side has to be careful in > where it allows to use this iter. The mainly concern is dead locking on > css_set_lock. check_css_task_iter_allowlist() in verifier enforced css_task > can only be used in bpf_lsm hooks and sleepable bpf_iter. > > This patch relax the allowlist for css_task iter. Any lsm and any iter > (even non-sleepable) and any sleepable are safe since they would not hold > the css_set_lock before entering BPF progs context. > > This patch also fixes the misused BPF_TRACE_ITER in > check_css_task_iter_allowlist which compared bpf_prog_type with > bpf_attach_type. > > Fixes: 9c66dc94b62ae ("bpf: Introduce css_task open-coded iterator kfuncs") > Signed-off-by: Chuyi Zhou <zhouchuyi@bytedance.com> > --- > kernel/bpf/verifier.c | 21 ++++++++++++------- > .../selftests/bpf/progs/iters_task_failure.c | 4 ++-- > 2 files changed, 15 insertions(+), 10 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index e9bc5d4a25a1..9f209adc4ccb 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -11088,18 +11088,23 @@ static int process_kf_arg_ptr_to_rbtree_node(struct bpf_verifier_env *env, > &meta->arg_rbtree_root.field); > } > > +/* > + * css_task iter allowlist is needed to avoid dead locking on css_set_lock. > + * LSM hooks and iters (both sleepable and non-sleepable) are safe. > + * Any sleepable progs are also safe since bpf_check_attach_target() enforce > + * them can only be attached to some specific hook points. > + */ > static bool check_css_task_iter_allowlist(struct bpf_verifier_env *env) > { > enum bpf_prog_type prog_type = resolve_prog_type(env->prog); > > - switch (prog_type) { > - case BPF_PROG_TYPE_LSM: > + if (prog_type == BPF_PROG_TYPE_LSM) > return true; > - case BPF_TRACE_ITER: > - return env->prog->aux->sleepable; > - default: > - return false; > - } > + > + if (env->prog->expected_attach_type == BPF_TRACE_ITER) > + return true; I think the switch by prog_type has to stay. Checking attach_type == BPF_TRACE_ITER without considering prog_type is fragile. It likely works, but we don't do it anywhere else. Let's stick to what is known to work. > -SEC("?fentry.s/" SYS_PREFIX "sys_getpgid") > -__failure __msg("css_task_iter is only allowed in bpf_lsm and bpf iter-s") > +SEC("?fentry/" SYS_PREFIX "sys_getpgid") > +__failure __msg("css_task_iter is only allowed in bpf_lsm, bpf_iter and sleepable progs") Please add both. fentry that is rejected and fentry.s that is accepted. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next v2 1/2] bpf: Relax allowlist for css_task iter 2023-10-24 4:57 ` Alexei Starovoitov @ 2023-10-24 5:52 ` Chuyi Zhou 2023-10-24 6:08 ` Yonghong Song 0 siblings, 1 reply; 9+ messages in thread From: Chuyi Zhou @ 2023-10-24 5:52 UTC (permalink / raw) To: Alexei Starovoitov Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau Hello, 在 2023/10/24 12:57, Alexei Starovoitov 写道: > On Mon, Oct 23, 2023 at 7:42 PM Chuyi Zhou <zhouchuyi@bytedance.com> wrote: >> >> The newly added open-coded css_task iter would try to hold the global >> css_set_lock in bpf_iter_css_task_new, so the bpf side has to be careful in >> where it allows to use this iter. The mainly concern is dead locking on >> css_set_lock. check_css_task_iter_allowlist() in verifier enforced css_task >> can only be used in bpf_lsm hooks and sleepable bpf_iter. >> >> This patch relax the allowlist for css_task iter. Any lsm and any iter >> (even non-sleepable) and any sleepable are safe since they would not hold >> the css_set_lock before entering BPF progs context. >> >> This patch also fixes the misused BPF_TRACE_ITER in >> check_css_task_iter_allowlist which compared bpf_prog_type with >> bpf_attach_type. >> >> Fixes: 9c66dc94b62ae ("bpf: Introduce css_task open-coded iterator kfuncs") >> Signed-off-by: Chuyi Zhou <zhouchuyi@bytedance.com> >> --- >> kernel/bpf/verifier.c | 21 ++++++++++++------- >> .../selftests/bpf/progs/iters_task_failure.c | 4 ++-- >> 2 files changed, 15 insertions(+), 10 deletions(-) >> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >> index e9bc5d4a25a1..9f209adc4ccb 100644 >> --- a/kernel/bpf/verifier.c >> +++ b/kernel/bpf/verifier.c >> @@ -11088,18 +11088,23 @@ static int process_kf_arg_ptr_to_rbtree_node(struct bpf_verifier_env *env, >> &meta->arg_rbtree_root.field); >> } >> >> +/* >> + * css_task iter allowlist is needed to avoid dead locking on css_set_lock. >> + * LSM hooks and iters (both sleepable and non-sleepable) are safe. >> + * Any sleepable progs are also safe since bpf_check_attach_target() enforce >> + * them can only be attached to some specific hook points. >> + */ >> static bool check_css_task_iter_allowlist(struct bpf_verifier_env *env) >> { >> enum bpf_prog_type prog_type = resolve_prog_type(env->prog); >> >> - switch (prog_type) { >> - case BPF_PROG_TYPE_LSM: >> + if (prog_type == BPF_PROG_TYPE_LSM) >> return true; >> - case BPF_TRACE_ITER: >> - return env->prog->aux->sleepable; >> - default: >> - return false; >> - } >> + >> + if (env->prog->expected_attach_type == BPF_TRACE_ITER) >> + return true; > > I think the switch by prog_type has to stay. > Checking attach_type == BPF_TRACE_ITER without considering prog_type > is fragile. It likely works, but we don't do it anywhere else. > Let's stick to what is known to work. > IIUC, do you mean: static bool check_css_task_iter_allowlist(struct bpf_verifier_env *env) { enum bpf_prog_type prog_type = resolve_prog_type(env->prog); switch (prog_type) { case BPF_PROG_TYPE_LSM: return true; case BPF_PROG_TYPE_TRACING: if (env->prog->expected_attach_type == BPF_TRACE_ITER) return true; return env->prog->aux->sleepable; default: return env->prog->aux->sleepable; } } >> -SEC("?fentry.s/" SYS_PREFIX "sys_getpgid") >> -__failure __msg("css_task_iter is only allowed in bpf_lsm and bpf iter-s") >> +SEC("?fentry/" SYS_PREFIX "sys_getpgid") >> +__failure __msg("css_task_iter is only allowed in bpf_lsm, bpf_iter and sleepable progs") > > Please add both. fentry that is rejected and fentry.s that is accepted. Sure. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next v2 1/2] bpf: Relax allowlist for css_task iter 2023-10-24 5:52 ` Chuyi Zhou @ 2023-10-24 6:08 ` Yonghong Song 2023-10-24 6:23 ` Chuyi Zhou 0 siblings, 1 reply; 9+ messages in thread From: Yonghong Song @ 2023-10-24 6:08 UTC (permalink / raw) To: Chuyi Zhou, Alexei Starovoitov Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau On 10/23/23 10:52 PM, Chuyi Zhou wrote: > Hello, > > 在 2023/10/24 12:57, Alexei Starovoitov 写道: >> On Mon, Oct 23, 2023 at 7:42 PM Chuyi Zhou <zhouchuyi@bytedance.com> >> wrote: >>> >>> The newly added open-coded css_task iter would try to hold the global >>> css_set_lock in bpf_iter_css_task_new, so the bpf side has to be >>> careful in >>> where it allows to use this iter. The mainly concern is dead locking on >>> css_set_lock. check_css_task_iter_allowlist() in verifier enforced >>> css_task >>> can only be used in bpf_lsm hooks and sleepable bpf_iter. >>> >>> This patch relax the allowlist for css_task iter. Any lsm and any iter >>> (even non-sleepable) and any sleepable are safe since they would not >>> hold >>> the css_set_lock before entering BPF progs context. >>> >>> This patch also fixes the misused BPF_TRACE_ITER in >>> check_css_task_iter_allowlist which compared bpf_prog_type with >>> bpf_attach_type. >>> >>> Fixes: 9c66dc94b62ae ("bpf: Introduce css_task open-coded iterator >>> kfuncs") >>> Signed-off-by: Chuyi Zhou <zhouchuyi@bytedance.com> >>> --- >>> kernel/bpf/verifier.c | 21 >>> ++++++++++++------- >>> .../selftests/bpf/progs/iters_task_failure.c | 4 ++-- >>> 2 files changed, 15 insertions(+), 10 deletions(-) >>> >>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >>> index e9bc5d4a25a1..9f209adc4ccb 100644 >>> --- a/kernel/bpf/verifier.c >>> +++ b/kernel/bpf/verifier.c >>> @@ -11088,18 +11088,23 @@ static int >>> process_kf_arg_ptr_to_rbtree_node(struct bpf_verifier_env *env, >>> &meta->arg_rbtree_root.field); >>> } >>> >>> +/* >>> + * css_task iter allowlist is needed to avoid dead locking on >>> css_set_lock. >>> + * LSM hooks and iters (both sleepable and non-sleepable) are safe. >>> + * Any sleepable progs are also safe since >>> bpf_check_attach_target() enforce >>> + * them can only be attached to some specific hook points. >>> + */ >>> static bool check_css_task_iter_allowlist(struct bpf_verifier_env >>> *env) >>> { >>> enum bpf_prog_type prog_type = resolve_prog_type(env->prog); >>> >>> - switch (prog_type) { >>> - case BPF_PROG_TYPE_LSM: >>> + if (prog_type == BPF_PROG_TYPE_LSM) >>> return true; >>> - case BPF_TRACE_ITER: >>> - return env->prog->aux->sleepable; >>> - default: >>> - return false; >>> - } >>> + >>> + if (env->prog->expected_attach_type == BPF_TRACE_ITER) >>> + return true; >> >> I think the switch by prog_type has to stay. >> Checking attach_type == BPF_TRACE_ITER without considering prog_type >> is fragile. It likely works, but we don't do it anywhere else. >> Let's stick to what is known to work. >> > > IIUC, do you mean: > > static bool check_css_task_iter_allowlist(struct bpf_verifier_env *env) > { > enum bpf_prog_type prog_type = resolve_prog_type(env->prog); > > switch (prog_type) { > case BPF_PROG_TYPE_LSM: > return true; > case BPF_PROG_TYPE_TRACING: > if (env->prog->expected_attach_type == BPF_TRACE_ITER) > return true; > return env->prog->aux->sleepable; The above can be a fullthrough instead. > default: > return env->prog->aux->sleepable; > } > } > >>> -SEC("?fentry.s/" SYS_PREFIX "sys_getpgid") >>> -__failure __msg("css_task_iter is only allowed in bpf_lsm and bpf >>> iter-s") >>> +SEC("?fentry/" SYS_PREFIX "sys_getpgid") >>> +__failure __msg("css_task_iter is only allowed in bpf_lsm, bpf_iter >>> and sleepable progs") >> >> Please add both. fentry that is rejected and fentry.s that is accepted. > > Sure. > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next v2 1/2] bpf: Relax allowlist for css_task iter 2023-10-24 6:08 ` Yonghong Song @ 2023-10-24 6:23 ` Chuyi Zhou 2023-10-24 11:43 ` Chuyi Zhou 0 siblings, 1 reply; 9+ messages in thread From: Chuyi Zhou @ 2023-10-24 6:23 UTC (permalink / raw) To: Yonghong Song, Alexei Starovoitov Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau Hello, 在 2023/10/24 14:08, Yonghong Song 写道: > > On 10/23/23 10:52 PM, Chuyi Zhou wrote: >> Hello, >> >> 在 2023/10/24 12:57, Alexei Starovoitov 写道: >>> On Mon, Oct 23, 2023 at 7:42 PM Chuyi Zhou <zhouchuyi@bytedance.com> >>> wrote: >>>> >>>> The newly added open-coded css_task iter would try to hold the global >>>> css_set_lock in bpf_iter_css_task_new, so the bpf side has to be >>>> careful in >>>> where it allows to use this iter. The mainly concern is dead locking on >>>> css_set_lock. check_css_task_iter_allowlist() in verifier enforced >>>> css_task >>>> can only be used in bpf_lsm hooks and sleepable bpf_iter. >>>> >>>> This patch relax the allowlist for css_task iter. Any lsm and any iter >>>> (even non-sleepable) and any sleepable are safe since they would not >>>> hold >>>> the css_set_lock before entering BPF progs context. >>>> >>>> This patch also fixes the misused BPF_TRACE_ITER in >>>> check_css_task_iter_allowlist which compared bpf_prog_type with >>>> bpf_attach_type. >>>> >>>> Fixes: 9c66dc94b62ae ("bpf: Introduce css_task open-coded iterator >>>> kfuncs") >>>> Signed-off-by: Chuyi Zhou <zhouchuyi@bytedance.com> >>>> --- >>>> kernel/bpf/verifier.c | 21 >>>> ++++++++++++------- >>>> .../selftests/bpf/progs/iters_task_failure.c | 4 ++-- >>>> 2 files changed, 15 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >>>> index e9bc5d4a25a1..9f209adc4ccb 100644 >>>> --- a/kernel/bpf/verifier.c >>>> +++ b/kernel/bpf/verifier.c >>>> @@ -11088,18 +11088,23 @@ static int >>>> process_kf_arg_ptr_to_rbtree_node(struct bpf_verifier_env *env, >>>> &meta->arg_rbtree_root.field); >>>> } >>>> >>>> +/* >>>> + * css_task iter allowlist is needed to avoid dead locking on >>>> css_set_lock. >>>> + * LSM hooks and iters (both sleepable and non-sleepable) are safe. >>>> + * Any sleepable progs are also safe since >>>> bpf_check_attach_target() enforce >>>> + * them can only be attached to some specific hook points. >>>> + */ >>>> static bool check_css_task_iter_allowlist(struct bpf_verifier_env >>>> *env) >>>> { >>>> enum bpf_prog_type prog_type = resolve_prog_type(env->prog); >>>> >>>> - switch (prog_type) { >>>> - case BPF_PROG_TYPE_LSM: >>>> + if (prog_type == BPF_PROG_TYPE_LSM) >>>> return true; >>>> - case BPF_TRACE_ITER: >>>> - return env->prog->aux->sleepable; >>>> - default: >>>> - return false; >>>> - } >>>> + >>>> + if (env->prog->expected_attach_type == BPF_TRACE_ITER) >>>> + return true; >>> >>> I think the switch by prog_type has to stay. >>> Checking attach_type == BPF_TRACE_ITER without considering prog_type >>> is fragile. It likely works, but we don't do it anywhere else. >>> Let's stick to what is known to work. >>> >> >> IIUC, do you mean: >> >> static bool check_css_task_iter_allowlist(struct bpf_verifier_env *env) >> { >> enum bpf_prog_type prog_type = resolve_prog_type(env->prog); >> >> switch (prog_type) { >> case BPF_PROG_TYPE_LSM: >> return true; >> case BPF_PROG_TYPE_TRACING: >> if (env->prog->expected_attach_type == BPF_TRACE_ITER) >> return true; >> return env->prog->aux->sleepable; > > > The above can be a fullthrough instead. > Sorry, what do you mean 'a fullthrough' ? Do you mean we can check env->prog->aux->sleepable first and then fall back to check prog/attach type ? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next v2 1/2] bpf: Relax allowlist for css_task iter 2023-10-24 6:23 ` Chuyi Zhou @ 2023-10-24 11:43 ` Chuyi Zhou 2023-10-24 15:28 ` Yonghong Song 0 siblings, 1 reply; 9+ messages in thread From: Chuyi Zhou @ 2023-10-24 11:43 UTC (permalink / raw) To: Yonghong Song, Alexei Starovoitov Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau 在 2023/10/24 14:23, Chuyi Zhou 写道: > Hello, > > 在 2023/10/24 14:08, Yonghong Song 写道: >> >> On 10/23/23 10:52 PM, Chuyi Zhou wrote: >>> Hello, >>> >>> 在 2023/10/24 12:57, Alexei Starovoitov 写道: >>>> On Mon, Oct 23, 2023 at 7:42 PM Chuyi Zhou <zhouchuyi@bytedance.com> >>>> wrote: >>>>> >>>>> The newly added open-coded css_task iter would try to hold the global >>>>> css_set_lock in bpf_iter_css_task_new, so the bpf side has to be >>>>> careful in >>>>> where it allows to use this iter. The mainly concern is dead >>>>> locking on >>>>> css_set_lock. check_css_task_iter_allowlist() in verifier enforced >>>>> css_task >>>>> can only be used in bpf_lsm hooks and sleepable bpf_iter. >>>>> >>>>> This patch relax the allowlist for css_task iter. Any lsm and any iter >>>>> (even non-sleepable) and any sleepable are safe since they would >>>>> not hold >>>>> the css_set_lock before entering BPF progs context. >>>>> >>>>> This patch also fixes the misused BPF_TRACE_ITER in >>>>> check_css_task_iter_allowlist which compared bpf_prog_type with >>>>> bpf_attach_type. >>>>> >>>>> Fixes: 9c66dc94b62ae ("bpf: Introduce css_task open-coded iterator >>>>> kfuncs") >>>>> Signed-off-by: Chuyi Zhou <zhouchuyi@bytedance.com> >>>>> --- >>>>> kernel/bpf/verifier.c | 21 >>>>> ++++++++++++------- >>>>> .../selftests/bpf/progs/iters_task_failure.c | 4 ++-- >>>>> 2 files changed, 15 insertions(+), 10 deletions(-) >>>>> >>>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >>>>> index e9bc5d4a25a1..9f209adc4ccb 100644 >>>>> --- a/kernel/bpf/verifier.c >>>>> +++ b/kernel/bpf/verifier.c >>>>> @@ -11088,18 +11088,23 @@ static int >>>>> process_kf_arg_ptr_to_rbtree_node(struct bpf_verifier_env *env, >>>>> &meta->arg_rbtree_root.field); >>>>> } >>>>> >>>>> +/* >>>>> + * css_task iter allowlist is needed to avoid dead locking on >>>>> css_set_lock. >>>>> + * LSM hooks and iters (both sleepable and non-sleepable) are safe. >>>>> + * Any sleepable progs are also safe since >>>>> bpf_check_attach_target() enforce >>>>> + * them can only be attached to some specific hook points. >>>>> + */ >>>>> static bool check_css_task_iter_allowlist(struct bpf_verifier_env >>>>> *env) >>>>> { >>>>> enum bpf_prog_type prog_type = resolve_prog_type(env->prog); >>>>> >>>>> - switch (prog_type) { >>>>> - case BPF_PROG_TYPE_LSM: >>>>> + if (prog_type == BPF_PROG_TYPE_LSM) >>>>> return true; >>>>> - case BPF_TRACE_ITER: >>>>> - return env->prog->aux->sleepable; >>>>> - default: >>>>> - return false; >>>>> - } >>>>> + >>>>> + if (env->prog->expected_attach_type == BPF_TRACE_ITER) >>>>> + return true; >>>> >>>> I think the switch by prog_type has to stay. >>>> Checking attach_type == BPF_TRACE_ITER without considering prog_type >>>> is fragile. It likely works, but we don't do it anywhere else. >>>> Let's stick to what is known to work. >>>> >>> >>> IIUC, do you mean: >>> >>> static bool check_css_task_iter_allowlist(struct bpf_verifier_env *env) >>> { >>> enum bpf_prog_type prog_type = resolve_prog_type(env->prog); >>> >>> switch (prog_type) { >>> case BPF_PROG_TYPE_LSM: >>> return true; >>> case BPF_PROG_TYPE_TRACING: >>> if (env->prog->expected_attach_type == BPF_TRACE_ITER) >>> return true; >>> return env->prog->aux->sleepable; >> >> >> The above can be a fullthrough instead. >> > > Sorry, what do you mean 'a fullthrough' ? > Do you mean we can check env->prog->aux->sleepable first and then fall > back to check prog/attach type ? > I see... Sorry for the above noise. I noticed verifier.c uses 'fallthrough' to avoid the build warning, so we can: static bool check_css_task_iter_allowlist(struct bpf_verifier_env *env) { enum bpf_prog_type prog_type = resolve_prog_type(env->prog); switch (prog_type) { case BPF_PROG_TYPE_LSM: return true; case BPF_PROG_TYPE_TRACING: if (env->prog->expected_attach_type == BPF_TRACE_ITER) return true; fallthrough; default: return env->prog->aux->sleepable; } } ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next v2 1/2] bpf: Relax allowlist for css_task iter 2023-10-24 11:43 ` Chuyi Zhou @ 2023-10-24 15:28 ` Yonghong Song 0 siblings, 0 replies; 9+ messages in thread From: Yonghong Song @ 2023-10-24 15:28 UTC (permalink / raw) To: Chuyi Zhou, Alexei Starovoitov Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau On 10/24/23 4:43 AM, Chuyi Zhou wrote: > > > 在 2023/10/24 14:23, Chuyi Zhou 写道: >> Hello, >> >> 在 2023/10/24 14:08, Yonghong Song 写道: >>> >>> On 10/23/23 10:52 PM, Chuyi Zhou wrote: >>>> Hello, >>>> >>>> 在 2023/10/24 12:57, Alexei Starovoitov 写道: >>>>> On Mon, Oct 23, 2023 at 7:42 PM Chuyi Zhou >>>>> <zhouchuyi@bytedance.com> wrote: >>>>>> >>>>>> The newly added open-coded css_task iter would try to hold the >>>>>> global >>>>>> css_set_lock in bpf_iter_css_task_new, so the bpf side has to be >>>>>> careful in >>>>>> where it allows to use this iter. The mainly concern is dead >>>>>> locking on >>>>>> css_set_lock. check_css_task_iter_allowlist() in verifier >>>>>> enforced css_task >>>>>> can only be used in bpf_lsm hooks and sleepable bpf_iter. >>>>>> >>>>>> This patch relax the allowlist for css_task iter. Any lsm and any >>>>>> iter >>>>>> (even non-sleepable) and any sleepable are safe since they would >>>>>> not hold >>>>>> the css_set_lock before entering BPF progs context. >>>>>> >>>>>> This patch also fixes the misused BPF_TRACE_ITER in >>>>>> check_css_task_iter_allowlist which compared bpf_prog_type with >>>>>> bpf_attach_type. >>>>>> >>>>>> Fixes: 9c66dc94b62ae ("bpf: Introduce css_task open-coded >>>>>> iterator kfuncs") >>>>>> Signed-off-by: Chuyi Zhou <zhouchuyi@bytedance.com> >>>>>> --- >>>>>> kernel/bpf/verifier.c | 21 >>>>>> ++++++++++++------- >>>>>> .../selftests/bpf/progs/iters_task_failure.c | 4 ++-- >>>>>> 2 files changed, 15 insertions(+), 10 deletions(-) >>>>>> >>>>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >>>>>> index e9bc5d4a25a1..9f209adc4ccb 100644 >>>>>> --- a/kernel/bpf/verifier.c >>>>>> +++ b/kernel/bpf/verifier.c >>>>>> @@ -11088,18 +11088,23 @@ static int >>>>>> process_kf_arg_ptr_to_rbtree_node(struct bpf_verifier_env *env, >>>>>> &meta->arg_rbtree_root.field); >>>>>> } >>>>>> >>>>>> +/* >>>>>> + * css_task iter allowlist is needed to avoid dead locking on >>>>>> css_set_lock. >>>>>> + * LSM hooks and iters (both sleepable and non-sleepable) are safe. >>>>>> + * Any sleepable progs are also safe since >>>>>> bpf_check_attach_target() enforce >>>>>> + * them can only be attached to some specific hook points. >>>>>> + */ >>>>>> static bool check_css_task_iter_allowlist(struct >>>>>> bpf_verifier_env *env) >>>>>> { >>>>>> enum bpf_prog_type prog_type = >>>>>> resolve_prog_type(env->prog); >>>>>> >>>>>> - switch (prog_type) { >>>>>> - case BPF_PROG_TYPE_LSM: >>>>>> + if (prog_type == BPF_PROG_TYPE_LSM) >>>>>> return true; >>>>>> - case BPF_TRACE_ITER: >>>>>> - return env->prog->aux->sleepable; >>>>>> - default: >>>>>> - return false; >>>>>> - } >>>>>> + >>>>>> + if (env->prog->expected_attach_type == BPF_TRACE_ITER) >>>>>> + return true; >>>>> >>>>> I think the switch by prog_type has to stay. >>>>> Checking attach_type == BPF_TRACE_ITER without considering prog_type >>>>> is fragile. It likely works, but we don't do it anywhere else. >>>>> Let's stick to what is known to work. >>>>> >>>> >>>> IIUC, do you mean: >>>> >>>> static bool check_css_task_iter_allowlist(struct bpf_verifier_env >>>> *env) >>>> { >>>> enum bpf_prog_type prog_type = resolve_prog_type(env->prog); >>>> >>>> switch (prog_type) { >>>> case BPF_PROG_TYPE_LSM: >>>> return true; >>>> case BPF_PROG_TYPE_TRACING: >>>> if (env->prog->expected_attach_type == BPF_TRACE_ITER) >>>> return true; >>>> return env->prog->aux->sleepable; >>> >>> >>> The above can be a fullthrough instead. >>> >> >> Sorry, what do you mean 'a fullthrough' ? >> Do you mean we can check env->prog->aux->sleepable first and then >> fall back to check prog/attach type ? >> > > I see... > > Sorry for the above noise. I noticed verifier.c uses 'fallthrough' to > avoid the build warning, so we can: > > static bool check_css_task_iter_allowlist(struct bpf_verifier_env *env) > { > enum bpf_prog_type prog_type = resolve_prog_type(env->prog); > > switch (prog_type) { > case BPF_PROG_TYPE_LSM: > return true; > case BPF_PROG_TYPE_TRACING: > if (env->prog->expected_attach_type == BPF_TRACE_ITER) > return true; > fallthrough; > default: > return env->prog->aux->sleepable; > } > } The above LGTM. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH bpf-next v2 2/2] selftests/bpf: Add test for css_task iter combining with cgroup iter 2023-10-24 2:42 [PATCH bpf-next v2 0/2] Relax allowlist for open-coded css_task iter Chuyi Zhou 2023-10-24 2:42 ` [PATCH bpf-next v2 1/2] bpf: Relax allowlist for " Chuyi Zhou @ 2023-10-24 2:42 ` Chuyi Zhou 1 sibling, 0 replies; 9+ messages in thread From: Chuyi Zhou @ 2023-10-24 2:42 UTC (permalink / raw) To: bpf; +Cc: ast, daniel, andrii, martin.lau, Chuyi Zhou This patch adds a test which demonstrates how css_task iter can be combined with cgroup iter and it won't cause deadlock, though cgroup iter is not sleepable. Signed-off-by: Chuyi Zhou <zhouchuyi@bytedance.com> --- .../selftests/bpf/prog_tests/cgroup_iter.c | 33 +++++++++++++++ .../selftests/bpf/progs/iters_css_task.c | 41 +++++++++++++++++++ 2 files changed, 74 insertions(+) diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_iter.c b/tools/testing/selftests/bpf/prog_tests/cgroup_iter.c index e02feb5fae97..3679687a6927 100644 --- a/tools/testing/selftests/bpf/prog_tests/cgroup_iter.c +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_iter.c @@ -4,6 +4,7 @@ #include <test_progs.h> #include <bpf/libbpf.h> #include <bpf/btf.h> +#include "iters_css_task.skel.h" #include "cgroup_iter.skel.h" #include "cgroup_helpers.h" @@ -263,6 +264,35 @@ static void test_walk_dead_self_only(struct cgroup_iter *skel) close(cgrp_fd); } +static void test_walk_self_only_css_task(void) +{ + struct iters_css_task *skel = NULL; + int err; + + skel = iters_css_task__open(); + if (!ASSERT_OK_PTR(skel, "skel_open")) + return; + + bpf_program__set_autoload(skel->progs.cgroup_id_printer, true); + + err = iters_css_task__load(skel); + if (!ASSERT_OK(err, "skel_load")) + goto cleanup; + + err = join_cgroup(cg_path[CHILD2]); + if (!ASSERT_OK(err, "join_cgroup")) + goto cleanup; + + skel->bss->target_pid = getpid(); + snprintf(expected_output, sizeof(expected_output), + PROLOGUE "%8llu\n" EPILOGUE, cg_id[CHILD2]); + read_from_cgroup_iter(skel->progs.cgroup_id_printer, cg_fd[CHILD2], + BPF_CGROUP_ITER_SELF_ONLY, "test_walk_self_only_css_task"); + ASSERT_EQ(skel->bss->css_task_cnt, 1, "css_task_cnt"); +cleanup: + iters_css_task__destroy(skel); +} + void test_cgroup_iter(void) { struct cgroup_iter *skel = NULL; @@ -293,6 +323,9 @@ void test_cgroup_iter(void) test_walk_self_only(skel); if (test__start_subtest("cgroup_iter__dead_self_only")) test_walk_dead_self_only(skel); + if (test__start_subtest("cgroup_iter__self_only_css_task")) + test_walk_self_only_css_task(); + out: cgroup_iter__destroy(skel); cleanup_cgroups(); diff --git a/tools/testing/selftests/bpf/progs/iters_css_task.c b/tools/testing/selftests/bpf/progs/iters_css_task.c index 5089ce384a1c..0974e6f44328 100644 --- a/tools/testing/selftests/bpf/progs/iters_css_task.c +++ b/tools/testing/selftests/bpf/progs/iters_css_task.c @@ -10,6 +10,7 @@ char _license[] SEC("license") = "GPL"; +struct cgroup *bpf_cgroup_acquire(struct cgroup *p) __ksym; struct cgroup *bpf_cgroup_from_id(u64 cgid) __ksym; void bpf_cgroup_release(struct cgroup *p) __ksym; @@ -45,3 +46,43 @@ int BPF_PROG(iter_css_task_for_each, struct vm_area_struct *vma, return -EPERM; } + +static inline u64 cgroup_id(struct cgroup *cgrp) +{ + return cgrp->kn->id; +} + +SEC("?iter/cgroup") +int cgroup_id_printer(struct bpf_iter__cgroup *ctx) +{ + struct seq_file *seq = ctx->meta->seq; + struct cgroup *cgrp, *acquired; + struct cgroup_subsys_state *css; + struct task_struct *task; + + cgrp = ctx->cgroup; + + /* epilogue */ + if (cgrp == NULL) { + BPF_SEQ_PRINTF(seq, "epilogue\n"); + return 0; + } + + /* prologue */ + if (ctx->meta->seq_num == 0) + BPF_SEQ_PRINTF(seq, "prologue\n"); + + BPF_SEQ_PRINTF(seq, "%8llu\n", cgroup_id(cgrp)); + + acquired = bpf_cgroup_from_id(cgroup_id(cgrp)); + if (!acquired) + return 0; + css = &acquired->self; + css_task_cnt = 0; + bpf_for_each(css_task, task, css, CSS_TASK_ITER_PROCS) { + if (task->pid == target_pid) + css_task_cnt++; + } + bpf_cgroup_release(acquired); + return 0; +} -- 2.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-10-24 15:28 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-10-24 2:42 [PATCH bpf-next v2 0/2] Relax allowlist for open-coded css_task iter Chuyi Zhou 2023-10-24 2:42 ` [PATCH bpf-next v2 1/2] bpf: Relax allowlist for " Chuyi Zhou 2023-10-24 4:57 ` Alexei Starovoitov 2023-10-24 5:52 ` Chuyi Zhou 2023-10-24 6:08 ` Yonghong Song 2023-10-24 6:23 ` Chuyi Zhou 2023-10-24 11:43 ` Chuyi Zhou 2023-10-24 15:28 ` Yonghong Song 2023-10-24 2:42 ` [PATCH bpf-next v2 2/2] selftests/bpf: Add test for css_task iter combining with cgroup iter Chuyi Zhou
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).