Re: [PATCH bpf-next 10/13] bpf: support 'arg:xxx' btf_decl_tag-based hints for global subprog args

[Eduard Zingerman <eddyz87@gmail.com>]

On Wed, 2023-12-06 at 10:15 -0800, Andrii Nakryiko wrote:
[...]
> > > +             tag = btf_find_decl_tag_value(btf, fn_t, i, "arg:");
> > 
> > Nit: this does a linear scan over all BTF type ids for each
> >      function parameter, which is kind of ugly.
> 
> I know, so it's a good thing I added caching, right? :) I'm just
> reusing existing code, though. It also errors out on having two
> matching tags with the same prefix, which for now is good enough, but
> we'll probably have to lift this restriction.
> 
> As for linear search. This might be fine, BPF program's BTF is
> generally much smaller than vmlinux's BTF, and it's not clear if
> building hashmap-based lookup for tags is worthwhile. For now it works
> well enough, so there is little motivation to get this improved.

Yeah, probably not that big of a deal.
Still ugly though :)

> > > +             /* 'arg:<tag>' decl_tag takes precedence over derivation of
> > > +              * register type from BTF type itself
> > > +              */
> > > +             if (tag) {
> > > +                     /* disallow arg tags in static subprogs */
> > > +                     if (!is_global) {
> > > +                             bpf_log(log, "arg#%d type tag is not supported in static functions\n", i);
> > > +                             return -EOPNOTSUPP;
> > > +                     }
> > 
> > Nit: this would be annoying if someone would add/remove 'static' a few
> >      times while developing BPF program. Are there safety reasons to
> >      forbid this?
> 
> I'm just trying to not introduce unintended interactions between arg
> tags and static functions, which basically can freely ignore BTF at
> verification time, as they don't need BTF info for correctness. If in
> the future we add tags support for static functions, I'd like to have
> a clean slate instead of worrying for backwards compat.

Ok, might be changed if people would complain.

[...]

> > > +             } else if (arg->arg_type == ARG_PTR_TO_PACKET_META) {
> > > +                     if (reg->type != PTR_TO_PACKET_META) {
> > > +                             bpf_log(log, "arg#%d expected pkt_meta, but got %s\n",
> > > +                                     i, reg_type_str(env, reg->type));
> > > +                             return -EINVAL;
> > > +                     }
> > > +             } else if (arg->arg_type == ARG_PTR_TO_PACKET_DATA) {
> > > +                     if (reg->type != PTR_TO_PACKET) {
> > 
> > I think it is necessary to check that 'reg->umax_value == 0'.
> > check_packet_access() uses reg->umax_value to bump
> > env->prog->aux->max_pkt_offset. When body of a global function is
> > verified it starts with 'umax_value == 0'.
> > Might be annoying from usability POV, however.
> 
> I'm not even sure what we are using this max_pkt_offset for?

Idk, but last time I asked if it could be removed Alexei was very
unhappy, referring to hardware offload. Probably to nfp_bpf_offload_check_mtu()
in drivers/net/ethernet/netronome/nfp/bpf/offload.c .

> I see that verifier is maintaining it, but I don't see it being
> checked... Seems like when we have tail calls we even set it to
> MAX_PACKET_OFF unconditionally...

That won't guarantee that offset is always in bounds [0, MAX_PACKET_OFF].

This property is enforced by find_good_pkt_pointers(), so that packet
pointers where umax_value might exceed MAX_PACKET_OFF won't gain
'range' (and if there is no range it is forbidden to read/write
using this pointer).

> This PKT_xxx business is a very unfamiliar territory for me, so I hope
> Martin and/or Alexei can chime in and suggest how to make global funcs
> safe to work with packet pointers without hurting usability.

The way I understand it there are only few aspects:
- max_pkt_offset is maintained;
- access through packet pointer is allowed only if it has 'range';
- 'range' is gained by comparing pkt + X with pkt_end;
- 'range' is not gained if access might exceed MAX_PACKET_OFF;
- whenever some pointer gains range all pointers with same id gain it
  (see find_good_pkt_pointers() for this and two points above);
- when a non constant is added to a packet pointer it gets new id
  (see adjust_ptr_min_max_vals()).

I think that all.