Re: [PATCH bpf-next v4 07/12] bpf: Unify referenced object tracking in verifier

[Eduard Zingerman <eddyz87@gmail.com>]

On Tue, 2026-05-12 at 23:27 +0200, Amery Hung wrote:
> On Tue, May 12, 2026 at 2:09 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > On Tue, 2026-05-12 at 22:37 +0200, Amery Hung wrote:
> > > On Tue, May 12, 2026 at 12:04 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > > > 
> > > > On Wed, 2026-05-06 at 07:27 -0700, Amery Hung wrote:
> > > > 
> > > > LGTM, a nit below.
> > > > 
> > > > [...]
> > > > 
> > > > > @@ -8028,14 +8024,13 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> > > > >       }
> > > > > 
> > > > >       if (reg->ref_obj_id && base_type(arg_type) != ARG_KPTR_XCHG_DEST) {
> > > > > -             if (meta->ref_obj_id) {
> > > > > -                     verbose(env, "more than one arg with ref_obj_id R%d %u %u",
> > > > > -                             regno, reg->ref_obj_id,
> > > > > -                             meta->ref_obj_id);
> > > > > +             if (meta->release_regno && meta->ref_obj.cnt) {
> > > > > +                     verbose(env, "more than one arg with ref_obj_id %s %u %u",
> > > > > +                             reg_arg_name(env, argno), reg->ref_obj_id,
> > > > > +                             meta->ref_obj.ref_obj_id);
> > > > 
> > > > I think this should be reported from update_ref_obj() itself,
> > > > it is more consistent logically and also avoids reporting code
> > > > duplication in check_kfunc_args() and check_kfunc_call().
> > > > Also, technically the reg_arg_name() is an independent fix.
> > > 
> > > We have kfuncs that have more than one ref_obj. This is not a problem
> > > unless ref_obj is referenced in the verification and is ambiguous
> > > (e.g., used it to tie the lifetime of the allocated memory with
> > > ref_obj). Therefore, it is kept at the location of use instead of
> > > update_ref_obj() to prevent breaking existing kfuncs.
> > 
> > Ack, thank you for explaining.
> > 
> > > > 
> > > > > Drop the selftest introduced in 7ec899ac90a2 (“selftests/bpf: Negative
> > > > > test case for ref_obj_id in args”) since the verifier no longer
> > > > > complains about ambiguous ref_obj if it is not used.
> > > > 
> > > > On the other hand, if you think that this property is worth having,
> > > > then maybe wrap the check with in some utility function?
> > > 
> > > A utility function like below makes sense if we fold ref_obj_id into
> > > id. It would be too cumbersome for v3 as we may need two of this for
> > > ref_obj->ref_obj_id and ref_obj->id.
> > > 
> > > static int get_ref_obj_id()
> > > {
> > >         if (ref_obj->cnt > 1) {
> > >                 verifier_bug(env, "function expects only one
> > > referenced object but got %d\n", ref_obj->cnt);
> > >                 return -EFAULT;
> > >         }
> > >         return ref_obj->id;
> > > }
> > 
> > Maybe just like this:
> > 
> >   static void validate_ref_obj(struct ref_obj_desc *d) { ... }
> > 
> 
> Do you mean:
> static int validate_ref_obj(struct ref_obj_desc *d) { ... }
> 
> Then to use it:
> err = validate_ref_obj(meta.ref_obj);
> if (err)
>         return err;
> 
> regs[BPF_REG_0].ref_obj_id = meta.ref_obj.ref_obj_id;
> 
> > ?

Yes, like that, sorry.