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.
