On Tue, Dec 5, 2023 at 3:22 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Mon, 2023-12-04 at 15:39 -0800, Andrii Nakryiko wrote:
> [...]
>
> > @@ -6845,7 +6845,47 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
> > * Only PTR_TO_CTX and SCALAR are supported atm.
> > */
> > for (i = 0; i < nargs; i++) {
> > + bool is_nonnull = false;
> > + const char *tag;
> > +
> > t = btf_type_by_id(btf, args[i].type);
> > +
> > + 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.
>
> > + if (IS_ERR(tag) && PTR_ERR(tag) == -ENOENT) {
> > + tag = NULL;
> > + } else if (IS_ERR(tag)) {
> > + bpf_log(log, "arg#%d type's tag fetching failure: %ld\n", i, PTR_ERR(tag));
> > + return PTR_ERR(tag);
> > + }
> > + /* '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.
>
> [...]
>
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 5787b7fd16ba..61e778dbde10 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -9268,9 +9268,30 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
> > ret = check_func_arg_reg_off(env, reg, regno, ARG_DONTCARE);
> > if (ret < 0)
> > return ret;
> > -
> > if (check_mem_reg(env, reg, regno, arg->mem_size))
> > return -EINVAL;
> > + if (!(arg->arg_type & PTR_MAYBE_NULL) && (reg->type & PTR_MAYBE_NULL)) {
> > + bpf_log(log, "arg#%d is expected to be non-NULL\n", i);
> > + return -EINVAL;
> > + }
> > + } 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? 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...
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.
>
> > + bpf_log(log, "arg#%d expected pkt, but got %s\n",
> > + i, reg_type_str(env, reg->type));
> > + return -EINVAL;
> > + }
> > + } else if (arg->arg_type == ARG_PTR_TO_PACKET_END) {
> > + if (reg->type != PTR_TO_PACKET_END) {
> > + bpf_log(log, "arg#%d expected pkt_end, but got %s\n",
> > + i, reg_type_str(env, reg->type));
> > + return -EINVAL;
> > + }
> > } else {
> > bpf_log(log, "verifier bug: unrecognized arg#%d type %d\n",
> > i, arg->arg_type);
>
> [...]
>
>
