On Fri, Jan 5, 2024 at 12:26 PM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxx> wrote:
>
> I'm still looking through the patches, but in the early parts I do
> note this oddity:
>
> On Wed, 3 Jan 2024 at 14:21, Andrii Nakryiko <andrii@xxxxxxxxxx> wrote:
> >
> > +struct bpf_token {
> > + struct work_struct work;
> > + atomic64_t refcnt;
> > + struct user_namespace *userns;
> > + u64 allowed_cmds;
> > +};
>
> Ok, not huge, and makes sense, although I wonder if that
>
> atomic64_t refcnt;
>
> should just be 'atomic_long_t' since presumably on 32-bit
> architectures you can't create enough references for a 64-bit atomic
> to make much sense.
>
> Or are there references to tokens that might not use any memory?
>
> Not a big deal, but 'atomic64_t' is very expensive on 32-bit
> architectures, and doesn't seem to make much sense unless you really
> specifically need 64 bits for some reason.
I used atomic64_t for consistency with other BPF objects (program,
etc) and not to have to worry even about hypothetical overflows.
32-bit atomic performance doesn't seem to be a big concern as a token
is passed into a pretty heavy-weight operations that create new BPF
object (map, program, BTF object), no matter how slow refcounting is
it will be lost in the noise for those operations.
>
> But regardless, this is odd:
>
> > diff --git a/kernel/bpf/token.c b/kernel/bpf/token.c
> > +
> > +static void bpf_token_free(struct bpf_token *token)
> > +{
> > + put_user_ns(token->userns);
> > + kvfree(token);
> > +}
>
> > +int bpf_token_create(union bpf_attr *attr)
> > +{
> > ....
> > + token = kvzalloc(sizeof(*token), GFP_USER);
>
> Ok, so the kvzalloc() and kvfree() certainly line up, but why use them at all?
No particular reason, kzalloc/kfree should totally work, it's a small
struct anyways. I will switch.
>
> kvmalloc() and friends are for "use kmalloc, and fall back on vmalloc
> for big allocations when that fails".
>
> For just a structure, a plain 'kzalloc()/kfree()' pair would seem to
> make much more sense.
>
> Neither of these issues are at all important, but I mention them
> because they made me go "What?" when reading through the patches.
>
> Linus
