linux-kernel - Re: [PATCH 1/2] bpf: Add BPF_MAP_TYPE_CRED

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPhsuW4phthSOfSGCrf5iFHqZH8DpTiGW+zgmTJQzNu0LByshw@mail.gmail.com>
Date: Fri, 12 Sep 2025 17:11:54 -0700
From: Song Liu <song@...nel.org>
To: David Windsor <dwindsor@...il.com>
Cc: bpf@...r.kernel.org, linux-kernel@...r.kernel.org, martin.lau@...ux.dev, 
	ast@...nel.org, daniel@...earbox.net, andrii@...nel.org, eddyz87@...il.com, 
	yonghong.song@...ux.dev, john.fastabend@...il.com, kpsingh@...nel.org, 
	sdf@...ichev.me, haoluo@...gle.com, jolsa@...nel.org
Subject: Re: [PATCH 1/2] bpf: Add BPF_MAP_TYPE_CRED_STORAGE map type and kfuncs

On Fri, Sep 12, 2025 at 3:25 PM David Windsor <dwindsor@...il.com> wrote:
>
> All other bpf local storage is obtained using helpers which benefit from
> RET_PTR_TO_MAP_VALUE_OR_NULL, so can return void * pointers directly to
> map values. kfuncs don't have that, so return struct
> bpf_local_storage_data * and access map values through sdata->data.
>
> Signed-off-by: David Windsor <dwindsor@...il.com>

Maybe I missed something, but I think you haven't addressed Alexei's
question in v1: why this is needed and why hash map is not sufficient.

Other local storage types (task, inode, sk storage) may get a large
number of entries in a system, and thus would benefit from object
local storage. I don't think we expect too many creds in a system.
hash map of a smallish size should be good in most cases, and be
faster than cred local storage.

Did I get this right?

Thanks,
Song

The following are some quick feedbacks of the patch, but let's
first address the question above.

This is hitting KASAN BUG in CI:

https://github.com/kernel-patches/bpf/actions/runs/17687566710/job/50275683479

(You may need to log in GitHub to see details).

[...]

> +
> +__bpf_kfunc int bpf_cred_storage_delete(struct bpf_map *map, struct cred *cred)
> +{
> +       if (!cred)
> +               return -EINVAL;
> +
> +       return cred_storage_delete(cred, map);
> +}
> +
> +BTF_KFUNCS_START(bpf_cred_storage_kfunc_ids)
> +BTF_ID_FLAGS(func, bpf_cred_storage_delete, 0)
> +BTF_ID_FLAGS(func, bpf_cred_storage_get, KF_RET_NULL)
> +BTF_KFUNCS_END(bpf_cred_storage_kfunc_ids)
> +
> +static const struct btf_kfunc_id_set bpf_cred_storage_kfunc_set = {
> +       .owner = THIS_MODULE,
> +       .set   = &bpf_cred_storage_kfunc_ids,
> +};
> +
> +static int __init bpf_cred_storage_init(void)
> +{
> +       int err;

We need an empty line after the declaration.
scripts/checkpatch.pl should warn this. Please fix other warnings
from checkpatch.pl.

> +       err = register_btf_kfunc_id_set(BPF_PROG_TYPE_LSM, &bpf_cred_storage_kfunc_set);
> +       if (err) {

[...]

> diff --git a/kernel/cred.c b/kernel/cred.c
> index 9676965c0981..a1be27fe5f4c 100644
> --- a/kernel/cred.c
> +++ b/kernel/cred.c
> @@ -38,6 +38,10 @@ static struct kmem_cache *cred_jar;
>  /* init to 2 - one for init_task, one to ensure it is never freed */
>  static struct group_info init_groups = { .usage = REFCOUNT_INIT(2) };
>
> +#ifdef CONFIG_BPF_LSM
> +#include <linux/bpf_lsm.h>
> +#endif

We defined a dummy version of bpf_cred_storage_free
in bpf_lsm.h, so the ifdef here is not needed.

> +
>  /*
>   * The initial credentials for the initial task
>   */
> @@ -76,6 +80,9 @@ static void put_cred_rcu(struct rcu_head *rcu)
>                       cred, atomic_long_read(&cred->usage));
>
>         security_cred_free(cred);
> +#ifdef CONFIG_BPF_LSM
> +       bpf_cred_storage_free(cred);
> +#endif

Ditto.

>         key_put(cred->session_keyring);
>         key_put(cred->process_keyring);
>         key_put(cred->thread_keyring);
[...]