Re: [PATCH bpf-next v2 1/2] bpf: Relax allowlist for css_task iter

[Yonghong Song <yonghong.song@linux.dev>]

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.