On Fri, Aug 07, 2020 at 01:04:26PM -0700, Andrii Nakryiko wrote:
SNIP
> > + }
> > }
> > } else if (!fn->check_btf_id(reg->btf_id, arg)) {
>
> Put this on a wishlist for now. I don't think we should expect
> fb->check_btf_id() to do btf_struct_ids_match() internally, so to
> support this, we'd have to call fb->check_btf_id() inside the loop
> while doing WALK_STRUCT struct. But let's not change all this in this
> patch set, it's involved enough already.
>
> > verbose(env, "Helper does not support %s in R%d\n",
> > @@ -3977,7 +3982,8 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> >
> > return -EACCES;
> > }
> > - if (!tnum_is_const(reg->var_off) || reg->var_off.value || reg->off) {
> > + if (!ids_match &&
> > + (!tnum_is_const(reg->var_off) || reg->var_off.value || reg->off)) {
>
> Isn't this still wrong? if ids_match, but reg->var_off is non-zero,
> that's still bad, right?
> ids_match just "mitigates" reg->off check, so should be something like this:
>
> if ((reg->off && !ids_match) || !tnum_is_const(reg->var_off) ||
> reg->var_off.value)
> ... then bad ...
damn you're right, those are separated things,
I mixed it up, I'll send new version
thanks,
jirka
