| Message ID | 20231024024240.42790-2-zhouchuyi@bytedance.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Relax allowlist for open-coded css_task iter | expand |
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.
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.
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. >
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 ?
在 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; } }
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.
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();
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(-)