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.
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?
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
