Re: [PATCH bpf-next v3 05/15] bpf: add bpf_skc_to_tcp6_sock() helper

[Yonghong Song <yhs@fb.com>]

On 6/22/20 11:39 PM, Andrii Nakryiko wrote:
> On Mon, Jun 22, 2020 at 5:38 PM Yonghong Song <yhs@fb.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@fb.com>
>> Signed-off-by: Yonghong Song <yhs@fb.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@fb.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.

> 
>> +               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 */

> 
>> +                       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;
>> +}
>> +
> 
> [...]
>