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

[Chuyi Zhou <zhouchuyi@bytedance.com>]

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 ?