Message-ID: <bfd134a9-d808-d66d-3870-361f8f5aab64@fb.com>
Date:   Tue, 23 Jun 2020 12:45:56 -0700
From:   Yonghong Song <yhs@...com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
CC:     bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>,
        Martin KaFai Lau <kafai@...com>
Subject: Re: [PATCH bpf-next v3 05/15] bpf: add bpf_skc_to_tcp6_sock() helper



On 6/23/20 11:23 AM, Andrii Nakryiko wrote:
> On Tue, Jun 23, 2020 at 7:52 AM Yonghong Song <yhs@...com> wrote:
>>
>>
>>
>> On 6/22/20 11:39 PM, Andrii Nakryiko wrote:
>>> On Mon, Jun 22, 2020 at 5:38 PM Yonghong Song <yhs@...com> wrote:
>>>>
>>>> The helper is used in tracing programs to cast a socket
>>>> pointer to a tcp6_sock pointer.
>>>> The return value could be NULL if the casting is illegal.
>>>>
>>>> A new helper return type RET_PTR_TO_BTF_ID_OR_NULL is added
>>>> so the verifier is able to deduce proper return types for the helper.
>>>>
>>>> Different from the previous BTF_ID based helpers,
>>>> the bpf_skc_to_tcp6_sock() argument can be several possible
>>>> btf_ids. More specifically, all possible socket data structures
>>>> with sock_common appearing in the first in the memory layout.
>>>> This patch only added socket types related to tcp and udp.
>>>>
>>>> All possible argument btf_id and return value btf_id
>>>> for helper bpf_skc_to_tcp6_sock() are pre-calculcated and
>>>> cached. In the future, it is even possible to precompute
>>>> these btf_id's at kernel build time.
>>>>
>>>> Acked-by: Martin KaFai Lau <kafai@...com>
>>>> Signed-off-by: Yonghong Song <yhs@...com>
>>>> ---
>>>
>>> Looks good to me as is, but see a few suggestions, they will probably
>>> save me time at some point as well :)
>>>
>>> Acked-by: Andrii Nakryiko <andriin@...com>
>>>
>>>
>>>>    include/linux/bpf.h            | 12 +++++
>>>>    include/uapi/linux/bpf.h       |  9 +++-
>>>>    kernel/bpf/btf.c               |  1 +
>>>>    kernel/bpf/verifier.c          | 43 +++++++++++++-----
>>>>    kernel/trace/bpf_trace.c       |  2 +
>>>>    net/core/filter.c              | 80 ++++++++++++++++++++++++++++++++++
>>>>    scripts/bpf_helpers_doc.py     |  2 +
>>>>    tools/include/uapi/linux/bpf.h |  9 +++-
>>>>    8 files changed, 146 insertions(+), 12 deletions(-)
>>>>
>>>
>>> [...]
>>>
>>>> @@ -4815,6 +4826,18 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
>>>>                   regs[BPF_REG_0].type = PTR_TO_MEM_OR_NULL;
>>>>                   regs[BPF_REG_0].id = ++env->id_gen;
>>>>                   regs[BPF_REG_0].mem_size = meta.mem_size;
>>>> +       } else if (fn->ret_type == RET_PTR_TO_BTF_ID_OR_NULL) {
>>>> +               int ret_btf_id;
>>>> +
>>>> +               mark_reg_known_zero(env, regs, BPF_REG_0);
>>>> +               regs[BPF_REG_0].type = PTR_TO_BTF_ID_OR_NULL;
>>>> +               ret_btf_id = *fn->ret_btf_id;
>>>
>>> might be a good idea to check fb->ret_btf_id for NULL and print a
>>> warning + return -EFAULT. It's not supposed to happen on properly
>>> configured kernel, but during development this will save a bunch of
>>> time and frustration for next person trying to add something with
>>> RET_PTR_TO_BTF_ID_OR_NULL.
>>
>> I would like prefer to delay this with current code. Otherwise,
>> it gives an impression fn->ret_btf_id might be NULL and it is
>> actually never NULL. We can add NULL check if the future change
>> requires it. I am not sure what the future change could be,
>> but you need some way to get the return btf_id, the above is
>> one of them.
> 
> It's not **supposed** to be NULL, same as a bunch of other invariants
> about BPF helper proto definitions, but verifier does check sanity for
> such cases, instead of crashing. But up to you. I'm pretty sure
> someone will trip up on this.

I think there are certain expectation for argument reg_state vs. certain
fields in the structure.

int btf_resolve_helper_id(struct bpf_verifier_log *log,
                           const struct bpf_func_proto *fn, int arg)
{
         int *btf_id = &fn->btf_id[arg];
         int ret;

         if (fn->arg_type[arg] != ARG_PTR_TO_BTF_ID)
                 return -EINVAL;

         ret = READ_ONCE(*btf_id);
	...
}

If ARG_PTR_TO_BTF_ID, the verifier did not really check
whether btf_id pointer is valid or not. It just use it.

The same applies to the new return type. If in func_proto,
somebody sets RET_PTR_TO_BTF_ID_OR_NULL, it is expected
that func_proto->ret_btf_id is valid.

Code review or feature selftest should catch errors
if they are out-of-sync.

> 
>>
>>>
>>>> +               if (ret_btf_id == 0) {
>>>
>>> This also has to be struct/union (after typedef/mods stripping, of
>>> course). Or are there other cases?
>>
>> This is an "int". The func_proto difinition is below:
>> int *ret_btf_id; /* return value btf_id */
> 
> I meant the BTF type itself that this btf_id points to. Is there any
> use case where this won't be a pointer to struct/union and instead
> something like a pointer to an int?

Maybe you misunderstood. The mechanism is similar to the argument btf_id 
encoding in func_proto's:

static int bpf_seq_printf_btf_ids[5];
...
         .btf_id         = bpf_seq_printf_btf_ids,

func_proto->ret_btf_id will be a pointer to int which encodes the 
btf_id, not the btf_type.

> 
>>
>>>
>>>> +                       verbose(env, "invalid return type %d of func %s#%d\n",
>>>> +                               fn->ret_type, func_id_name(func_id), func_id);
>>>> +                       return -EINVAL;
>>>> +               }
>>>> +               regs[BPF_REG_0].btf_id = ret_btf_id;
>>>>           } else {
>>>>                   verbose(env, "unknown return type %d of func %s#%d\n",
>>>>                           fn->ret_type, func_id_name(func_id), func_id);
>>>
>>> [...]
>>>
>>>> +void init_btf_sock_ids(struct btf *btf)
>>>> +{
>>>> +       int i, btf_id;
>>>> +
>>>> +       for (i = 0; i < MAX_BTF_SOCK_TYPE; i++) {
>>>> +               btf_id = btf_find_by_name_kind(btf, bpf_sock_types[i],
>>>> +                                              BTF_KIND_STRUCT);
>>>> +               if (btf_id > 0)
>>>> +                       btf_sock_ids[i] = btf_id;
>>>> +       }
>>>> +}
>>>
>>> This will hopefully go away with Jiri's work on static BTF IDs, right?
>>> So looking forward to that :)
>>
>> Yes. That's the plan.
>>
>>>
>>>> +
>>>> +static bool check_arg_btf_id(u32 btf_id, u32 arg)
>>>> +{
>>>> +       int i;
>>>> +
>>>> +       /* only one argument, no need to check arg */
>>>> +       for (i = 0; i < MAX_BTF_SOCK_TYPE; i++)
>>>> +               if (btf_sock_ids[i] == btf_id)
>>>> +                       return true;
>>>> +       return false;
>>>> +}
>>>> +
>>>
>>> [...]
>>>

Powered by blists - more mailing lists