On Tue, Jun 23, 2020 at 12:47 PM Yonghong Song <yhs@xxxxxx> wrote:
>
>
>
> On 6/23/20 11:23 AM, Andrii Nakryiko wrote:
> > On Tue, Jun 23, 2020 at 7:52 AM Yonghong Song <yhs@xxxxxx> wrote:
> >>
> >>
> >>
> >> On 6/22/20 11:39 PM, Andrii Nakryiko wrote:
> >>> On Mon, Jun 22, 2020 at 5:38 PM Yonghong Song <yhs@xxxxxx> 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@xxxxxx>
> >>>> Signed-off-by: Yonghong Song <yhs@xxxxxx>
> >>>> ---
> >>>
> >>> 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@xxxxxx>
> >>>
> >>>
> >>>> 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.
Right, it's not a universal rule. But grep for "misconfigured" in
kernel/bpf/verifier.c to see a bunch of places where the verifier
could crash on NULL dereference, but instead emits an error message
and returns failure.
This was a suggestion, I'll stop asking for this :)
>
> 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.
I understand that. Say it points to value 25. BTF type with ID=25 is
going to be BTF_KIND_PTR -> BTF_KIND_STRUCT. I was wondering if we
want/need to check that it's always BTF_KIND_PTR -> (modifier)* ->
BTF_KIND_STRUCT/BTF_KIND_UNION. That's it.
>
> >
> >>
> >>>
> >>>> + 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;
> >>>> +}
> >>>> +
> >>>
> >>> [...]
> >>>
