Re: [PATCH net-next v4 1/3] net: flow_dissector: extend bpf flow dissector support with vnet hdr

[Martin KaFai Lau <kafai@fb.com>]

On Wed, Jun 09, 2021 at 10:12:36AM -0700, Tanner Love wrote:
[ ... ]

> > > @@ -4218,6 +4243,23 @@ static int check_mem_access(struct
> > bpf_verifier_env *env, int insn_idx, u32 regn
> > >               }
> > >
> > >               err = check_flow_keys_access(env, off, size);
> > > +             if (!err && t == BPF_READ && value_regno >= 0) {
> > > +                     if (off == offsetof(struct bpf_flow_keys, vhdr)) {
> > > +                             regs[value_regno].type =
> > PTR_TO_VNET_HDR_OR_NULL;
> > check_flow_keys_access() needs some modifications
> >
> > 1. What if t == BPF_WRITE?  I think "keys->vhdr = 0xdead" has to be
> > rejected.
> >
> > 2. It needs to check loading keys->vhdr is in sizeof(__u64) like other
> >    pointer loading does.  Take a look at the flow_keys case in
> >    flow_dissector_is_valid_access().
> >
> > It also needs to convert the pointer loading like how
> > flow_dissector_convert_ctx_access() does on flow_keys.
> >
> 
> I understand 1 and 2, and I agree. I will make the changes in the
> next version. Thanks. But I do not understand your comment "It
> also needs to convert the pointer loading like how
> flow_dissector_convert_ctx_access() does on flow_keys."
> Convert it to what? The pointer to struct virtio_net_hdr is in struct
> bpf_flow_keys, not struct bpf_flow_dissector (the kernel context).
> Could you please elaborate? Thank you!
Ah, right. There is no kernel counter part for bpf_flow_keys.
Please ignore the "convert the pointer loading" comment.

> 
> >
> > A high level design question.  "struct virtio_net_hdr" is in uapi and
> > there is no need to do convert_ctx.  I think using PTR_TO_BTF_ID_OR_NULL
> > will be easier here and the new PTR_TO_VNET_HDR* related changes will go
> > away.
> >
> > The "else if (reg->type == PTR_TO_CTX)" case earlier could be a good
> > example.
> >
> > To get the btf_id for "struct virtio_net_hdr", take a look at
> > the BTF_ID_LIST_SINGLE() usage in filter.c
> >
> 
> Thanks for the suggestion. Still ruminating on this, but figured I'd send my
> above question in the meantime.
btf_id points to a BTF debuginfo that describes how a kernel struct looks like.

PTR_TO_BTF_ID(_NOT_NULL) means a reg is a pointer to a kernel struct described
by a BTF (so the btf_id).  With the BTF, the access is checked commonly
in btf_struct_access() for any kernel struct.

It needs the BPF_PROBE_MEM support in the JIT which is currently in
x86/arm64/s390.