Re: [PATCH v2 2/3] bpf: remove bpf_key reference

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Thu, Jul 31, 2025 at 10:27 AM James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> On Thu, 2025-07-31 at 10:03 -0700, Alexei Starovoitov wrote:
> > On Wed, Jul 30, 2025 at 10:32 AM James Bottomley
> > <James.Bottomley@hansenpartnership.com> wrote:
> > >
> > > bpf_key.has_ref is used to distinguish between real key pointers
> > > and
> > > the fake key pointers that are used for system keyrings (to ensure
> > > the
> > > actual pointers to system keyrings are never visible outside
> > > certs/system_keyring.c).  The keyrings subsystem has an exported
> > > function to do this, so use that in the bpf keyring code
> > > eliminating
> > > the need to store has_ref.
> > >
> > > Signed-off-by: James Bottomley
> > > <James.Bottomley@HansenPartnership.com>
> > >
> > > ---
> > > v2: use unsigned long for pointer to int conversion
> > > ---
> > >  kernel/trace/bpf_trace.c | 7 ++-----
> > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > index e7bf00d1cd05..c0ccd55a4d91 100644
> > > --- a/kernel/trace/bpf_trace.c
> > > +++ b/kernel/trace/bpf_trace.c
> > > @@ -1244,7 +1244,6 @@ static const struct bpf_func_proto
> > > bpf_get_func_arg_cnt_proto = {
> > >  #ifdef CONFIG_KEYS
> > >  struct bpf_key {
> > >         struct key *key;
> > > -       bool has_ref;
> > >  };
> > >
> > >  __bpf_kfunc_start_defs();
> > > @@ -1297,7 +1296,6 @@ __bpf_kfunc struct bpf_key
> > > *bpf_lookup_user_key(s32 serial, u64 flags)
> > >         }
> > >
> > >         bkey->key = key_ref_to_ptr(key_ref);
> > > -       bkey->has_ref = true;
> > >
> > >         return bkey;
> > >  }
> > > @@ -1335,7 +1333,6 @@ __bpf_kfunc struct bpf_key
> > > *bpf_lookup_system_key(u64 id)
> > >                 return NULL;
> > >
> > >         bkey->key = (struct key *)(unsigned long)id;
> > > -       bkey->has_ref = false;
> > >
> > >         return bkey;
> > >  }
> > > @@ -1349,7 +1346,7 @@ __bpf_kfunc struct bpf_key
> > > *bpf_lookup_system_key(u64 id)
> > >   */
> > >  __bpf_kfunc void bpf_key_put(struct bpf_key *bkey)
> > >  {
> > > -       if (bkey->has_ref)
> > > +       q
> > >                 key_put(bkey->key);
> >
> > Should be (u64) to avoid truncation ?
>
> It can't be: gcc only allows pointer to unsigned long conversion, so
> the statement
>
>   if (system_keyring_id_check((u64)bkey->key) < 0)
>
> produces a pointer to int conversion error.  Since the function
> prototype is u64 the conversion from unsigned long to u64 happens
> automatically.
>
>
> > But is it really the case that id==1 and id==2 are exposed to UAPI
> > already?
> >
> > As far as I can see lookup_user_key() does:
> >         default:
> >                 key_ref = ERR_PTR(-EINVAL);
> >                 if (id < 1)
> >                         goto error;
> >
> >                 key = key_lookup(id);
> >
> > so only id==0 is invalid, but id=1 can be a valid user key, no?
>
> Well, remember the id as pointer trick is only used for the system_key
> lookup.  What you get back from user_key lookup is a real pointer to
> the key (regardless of what serial id you pass in) so there's no chance
> of getting 1 or 2 back for a user key.
>
> However, if you were thinking of overloading key look up, it is
> currently the case, in spite of the check in lookup above, that user
> key serial numbers begin at three thanks to this code in
> key.c:key_alloc_serial()
>
>         do {
>                 get_random_bytes(&key->serial, sizeof(key->serial));
>
>                 key->serial >>= 1; /* negative numbers are not permitted */
>         } while (key->serial < 3);

I see. That's what I was missing.

> David said he would prefer, if we to allow system keyring lookup here,
> to use negative ids (like keyrings) for them.

Makes sense to me as well.
Do you want to do a follow up or respin this set ?

Would be great if he can ack this set too.