Message-ID: <CAEf4BzYvra0bijcbzpBbwwtFQg4_8Uy3tGLwYYj=9CpkMPW=-w@mail.gmail.com>
Date:   Tue, 23 Jun 2020 11:23:18 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Yonghong Song <yhs@...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 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.

>
> >
> >> +               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?

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