Hi Eric!
On Thu, Jun 18 2026, Eric Biggers wrote:
> Change mk_users (the set of user claims to an fscrypt master key) from a
> 'struct key' keyring to a simple linked list.
>
> It's still a collection of 'struct key' for quota tracking. It was
> originally thought to be natural that a collection of 'struct key'
> should be held in a 'struct key' keyring. In reality, it's just been
> causing problems, similar to how using 'struct key' for the filesystem
> keyring caused problems and was removed in commit d7e7b9af104c
> ("fscrypt: stop using keyrings subsystem for fscrypt_master_key").
>
> Commit d3a7bd420076 ("fscrypt: clear keyring before calling key_put()")
> fixed mk_users cleanup to be synchronous. But that apparently wasn't
> enough: the keyring subsystem's redundant locking is still generating
> lockdep false positives due to the interaction with filesystem reclaim.
>
> With the simple list, the redundant locking and lockdep issue goes away.
>
> Of course, searching a linked list is linear-time whereas the
> 'struct key' keyring used a fancy constant-time associative array. But
> that's fine here, since in practice there's just one entry in the list.
> In fact the new code is much faster in practice, since it's much smaller
> and doesn't have to convert the kuid_t into a string to search for it.
>
> Reported-by: [email protected]
> Closes: https://syzkaller.appspot.com/bug?extid=f55b043dacf43776b50c
> Reported-by: Mohammed EL Kadiri <[email protected]>
> Closes:
> https://lore.kernel.org/keyrings/[email protected]/
> Fixes: 23c688b54016 ("fscrypt: allow unprivileged users to add/remove keys
> for v2 policies")
> Cc: [email protected]
> Signed-off-by: Eric Biggers <[email protected]>
> ---
>
> I'm planning to take this via the fscrypt tree for 7.2
I was hoping to have some more time to test this patch, but I don't think
that will happen any time soon.
I've done a review of the patch and couldn't find any obvious problem.
Though, again, a more in-depth review would require more time as it has
been a while since I took a look into this code.
I just wonder if this is really stable material. It's a bit intrusive
(doesn't even apply cleanly in mainline), but above all it's fixing a
lockdep false positive. Other than syzbot, has this been seen in the
wild?
Cheers,
--
Luís
>
> fs/crypto/fscrypt_private.h | 32 ++++--
> fs/crypto/keyring.c | 216 +++++++++++++++---------------------
> 2 files changed, 113 insertions(+), 135 deletions(-)
>
> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index 4263cac24b32..0053b5c45412 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -494,10 +494,23 @@ fscrypt_is_key_prepared(const struct
> fscrypt_prepared_key *prep_key,
> }
> #endif /* !CONFIG_FS_ENCRYPTION_INLINE_CRYPT */
>
> /* keyring.c */
>
> +/*
> + * fscrypt_master_key_user - a user's claim to a master key
> + */
> +struct fscrypt_master_key_user {
> + struct list_head link;
> + kuid_t uid;
> + /*
> + * This 'struct key' contains no secret. It exists solely to charge the
> + * appropriate user's key quota.
> + */
> + struct key *quota_key;
> +};
> +
> /*
> * fscrypt_master_key_secret - secret key material of an in-use master key
> */
> struct fscrypt_master_key_secret {
>
> @@ -609,23 +622,22 @@ struct fscrypt_master_key {
> * For v2 policy keys: a cryptographic hash of this key (->identifier).
> */
> struct fscrypt_key_specifier mk_spec;
>
> /*
> - * Keyring which contains a key of type 'key_type_fscrypt_user' for each
> - * user who has added this key. Normally each key will be added by just
> - * one user, but it's possible that multiple users share a key, and in
> - * that case we need to keep track of those users so that one user can't
> - * remove the key before the others want it removed too.
> + * List of user claims to this key (struct fscrypt_master_key_user).
> + * Normally each key will be added by just one user, but it's possible
> + * that multiple users share a key, and in that case we need to keep
> + * track of those users so that one user can't remove the key before the
> + * others want it removed too.
> *
> - * This is NULL for v1 policy keys; those can only be added by root.
> + * Used only for v2 policy keys. v1 policy keys can be added only by
> + * root, so user tracking doesn't apply to them.
> *
> - * Locking: protected by ->mk_sem. (We don't just rely on the keyrings
> - * subsystem semaphore ->mk_users->sem, as we need support for atomic
> - * search+insert along with proper synchronization with other fields.)
> + * Locking: protected by ->mk_sem.
> */
> - struct key *mk_users;
> + struct list_head mk_users;
>
> /*
> * List of inodes that were unlocked using this key. This allows the
> * inodes to be evicted efficiently if the key is removed.
> */
> diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c
> index 6ce6b436c34f..16bc348213ca 100644
> --- a/fs/crypto/keyring.c
> +++ b/fs/crypto/keyring.c
> @@ -63,26 +63,23 @@ static void fscrypt_free_master_key(struct rcu_head *head)
> * Nevertheless, use kfree_sensitive() in case anything was missed.
> */
> kfree_sensitive(mk);
> }
>
> +static void clear_mk_users(struct fscrypt_master_key *mk);
> +
> void fscrypt_put_master_key(struct fscrypt_master_key *mk)
> {
> if (!refcount_dec_and_test(&mk->mk_struct_refs))
> return;
> /*
> - * No structural references left, so free ->mk_users, and also free the
> + * No structural references left, so clear ->mk_users, and also free the
> * fscrypt_master_key struct itself after an RCU grace period ensures
> * that concurrent keyring lookups can no longer find it.
> */
> WARN_ON_ONCE(refcount_read(&mk->mk_active_refs) != 0);
> - if (mk->mk_users) {
> - /* Clear the keyring so the quota gets released right away. */
> - keyring_clear(mk->mk_users);
> - key_put(mk->mk_users);
> - mk->mk_users = NULL;
> - }
> + clear_mk_users(mk);
> call_rcu(&mk->mk_rcu_head, fscrypt_free_master_key);
> }
>
> void fscrypt_put_master_key_activeref(struct super_block *sb,
> struct fscrypt_master_key *mk)
> @@ -163,12 +160,12 @@ static void fscrypt_user_key_describe(const struct key
> *key, struct seq_file *m)
> {
> seq_puts(m, key->description);
> }
>
> /*
> - * Type of key in ->mk_users. Each key of this type represents a particular
> - * user who has added a particular master key.
> + * Type of fscrypt_master_key_user::quota_key. This contains no secret; it
> + * exists solely to charge a user's key quota.
> *
> * Note that the name of this key type really should be something like
> * ".fscrypt-user" instead of simply ".fscrypt". But the shorter name is
> chosen
> * mainly for simplicity of presentation in /proc/keys when read by a
> non-root
> * user. And it is expected to be rare that a key is actually added by
> multiple
> @@ -178,34 +175,13 @@ static struct key_type key_type_fscrypt_user = {
> .name = ".fscrypt",
> .instantiate = fscrypt_user_key_instantiate,
> .describe = fscrypt_user_key_describe,
> };
>
> -#define FSCRYPT_MK_USERS_DESCRIPTION_SIZE \
> - (CONST_STRLEN("fscrypt-") + 2 * FSCRYPT_KEY_IDENTIFIER_SIZE + \
> - CONST_STRLEN("-users") + 1)
> -
> #define FSCRYPT_MK_USER_DESCRIPTION_SIZE \
> (2 * FSCRYPT_KEY_IDENTIFIER_SIZE + CONST_STRLEN(".uid.") + 10 + 1)
>
> -static void format_mk_users_keyring_description(
> - char description[FSCRYPT_MK_USERS_DESCRIPTION_SIZE],
> - const u8 mk_identifier[FSCRYPT_KEY_IDENTIFIER_SIZE])
> -{
> - sprintf(description, "fscrypt-%*phN-users",
> - FSCRYPT_KEY_IDENTIFIER_SIZE, mk_identifier);
> -}
> -
> -static void format_mk_user_description(
> - char description[FSCRYPT_MK_USER_DESCRIPTION_SIZE],
> - const u8 mk_identifier[FSCRYPT_KEY_IDENTIFIER_SIZE])
> -{
> -
> - sprintf(description, "%*phN.uid.%u", FSCRYPT_KEY_IDENTIFIER_SIZE,
> - mk_identifier, __kuid_val(current_fsuid()));
> -}
> -
> /* Create ->s_master_keys if needed. Synchronized by fscrypt_add_key_mutex.
> */
> static int allocate_filesystem_keyring(struct super_block *sb)
> {
> struct fscrypt_keyring *keyring;
>
> @@ -336,95 +312,98 @@ fscrypt_find_master_key(struct super_block *sb,
> out:
> rcu_read_unlock();
> return mk;
> }
>
> -static int allocate_master_key_users_keyring(struct fscrypt_master_key *mk)
> -{
> - char description[FSCRYPT_MK_USERS_DESCRIPTION_SIZE];
> - struct key *keyring;
> -
> - format_mk_users_keyring_description(description,
> - mk->mk_spec.u.identifier);
> - keyring = keyring_alloc(description, GLOBAL_ROOT_UID, GLOBAL_ROOT_GID,
> - current_cred(), KEY_POS_SEARCH |
> - KEY_USR_SEARCH | KEY_USR_READ | KEY_USR_VIEW,
> - KEY_ALLOC_NOT_IN_QUOTA, NULL, NULL);
> - if (IS_ERR(keyring))
> - return PTR_ERR(keyring);
> -
> - mk->mk_users = keyring;
> - return 0;
> -}
> -
> -/*
> - * Find the current user's "key" in the master key's ->mk_users.
> - * Returns ERR_PTR(-ENOKEY) if not found.
> - */
> -static struct key *find_master_key_user(struct fscrypt_master_key *mk)
> +/* Find the current user's claim in ->mk_users. ->mk_sem must be held. */
> +static struct fscrypt_master_key_user *
> +find_master_key_user(struct fscrypt_master_key *mk)
> {
> - char description[FSCRYPT_MK_USER_DESCRIPTION_SIZE];
> - key_ref_t keyref;
> + struct fscrypt_master_key_user *mk_user;
> + kuid_t uid = current_fsuid();
>
> - format_mk_user_description(description, mk->mk_spec.u.identifier);
> -
> - /*
> - * We need to mark the keyring reference as "possessed" so that we
> - * acquire permission to search it, via the KEY_POS_SEARCH permission.
> - */
> - keyref = keyring_search(make_key_ref(mk->mk_users, true /*possessed*/),
> - &key_type_fscrypt_user, description, false);
> - if (IS_ERR(keyref)) {
> - if (PTR_ERR(keyref) == -EAGAIN || /* not found */
> - PTR_ERR(keyref) == -EKEYREVOKED) /* recently invalidated */
> - keyref = ERR_PTR(-ENOKEY);
> - return ERR_CAST(keyref);
> + list_for_each_entry(mk_user, &mk->mk_users, link) {
> + if (uid_eq(mk_user->uid, uid))
> + return mk_user;
> }
> - return key_ref_to_ptr(keyref);
> + return NULL;
> }
>
> /*
> - * Give the current user a "key" in ->mk_users. This charges the user's
> quota
> + * Give the current user a claim in ->mk_users. This charges the user's
> quota
> * and marks the master key as added by the current user, so that it cannot
> be
> * removed by another user with the key. Either ->mk_sem must be held for
> * write, or the master key must be still undergoing initialization.
> */
> static int add_master_key_user(struct fscrypt_master_key *mk)
> {
> + kuid_t uid = current_fsuid();
> char description[FSCRYPT_MK_USER_DESCRIPTION_SIZE];
> - struct key *mk_user;
> + struct key *quota_key;
> + struct fscrypt_master_key_user *mk_user;
> int err;
>
> - format_mk_user_description(description, mk->mk_spec.u.identifier);
> - mk_user = key_alloc(&key_type_fscrypt_user, description,
> - current_fsuid(), current_gid(), current_cred(),
> - KEY_POS_SEARCH | KEY_USR_VIEW, 0, NULL);
> - if (IS_ERR(mk_user))
> - return PTR_ERR(mk_user);
> + snprintf(description, sizeof(description), "%*phN.uid.%u",
> + FSCRYPT_KEY_IDENTIFIER_SIZE, mk->mk_spec.u.identifier,
> + __kuid_val(uid));
> + quota_key = key_alloc(&key_type_fscrypt_user, description, uid,
> + current_gid(), current_cred(),
> + KEY_POS_SEARCH | KEY_USR_VIEW, 0, NULL);
> + if (IS_ERR(quota_key))
> + return PTR_ERR(quota_key);
> +
> + err = key_instantiate_and_link(quota_key, NULL, 0, NULL, NULL);
> + if (err) {
> + key_put(quota_key);
> + return err;
> + }
>
> - err = key_instantiate_and_link(mk_user, NULL, 0, mk->mk_users, NULL);
> - key_put(mk_user);
> - return err;
> + mk_user = kzalloc_obj(*mk_user);
> + if (!mk_user) {
> + key_put(quota_key);
> + return -ENOMEM;
> + }
> + mk_user->uid = uid;
> + mk_user->quota_key = quota_key;
> + list_add(&mk_user->link, &mk->mk_users);
> + return 0;
> +}
> +
> +static void unlink_and_free_mk_user(struct fscrypt_master_key_user *mk_user)
> +{
> + list_del(&mk_user->link);
> + key_put(mk_user->quota_key);
> + kfree(mk_user);
> }
>
> /*
> - * Remove the current user's "key" from ->mk_users.
> + * Remove the current user's claim from ->mk_users.
> * ->mk_sem must be held for write.
> *
> - * Returns 0 if removed, -ENOKEY if not found, or another -errno code.
> + * Returns 0 if removed or -ENOKEY if not found.
> */
> static int remove_master_key_user(struct fscrypt_master_key *mk)
> {
> - struct key *mk_user;
> - int err;
> + struct fscrypt_master_key_user *mk_user;
>
> mk_user = find_master_key_user(mk);
> - if (IS_ERR(mk_user))
> - return PTR_ERR(mk_user);
> - err = key_unlink(mk->mk_users, mk_user);
> - key_put(mk_user);
> - return err;
> + if (!mk_user)
> + return -ENOKEY;
> + unlink_and_free_mk_user(mk_user);
> + return 0;
> +}
> +
> +/*
> + * Clear ->mk_users. Either ->mk_sem must be held for write, or 'mk' must
> have
> + * no structural references left.
> + */
> +static void clear_mk_users(struct fscrypt_master_key *mk)
> +{
> + struct fscrypt_master_key_user *mk_user, *tmp;
> +
> + list_for_each_entry_safe(mk_user, tmp, &mk->mk_users, link)
> + unlink_and_free_mk_user(mk_user);
> }
>
> /*
> * Allocate a new fscrypt_master_key, transfer the given secret over to it,
> and
> * insert it into sb->s_master_keys.
> @@ -443,19 +422,18 @@ static int add_new_master_key(struct super_block *sb,
>
> init_rwsem(&mk->mk_sem);
> refcount_set(&mk->mk_struct_refs, 1);
> mk->mk_spec = *mk_spec;
>
> + INIT_LIST_HEAD(&mk->mk_users);
> +
> INIT_LIST_HEAD(&mk->mk_decrypted_inodes);
> spin_lock_init(&mk->mk_decrypted_inodes_lock);
>
> INIT_LIST_HEAD(&mk->mk_mode_keys);
>
> if (mk_spec->type == FSCRYPT_KEY_SPEC_TYPE_IDENTIFIER) {
> - err = allocate_master_key_users_keyring(mk);
> - if (err)
> - goto out_put;
> err = add_master_key_user(mk);
> if (err)
> goto out_put;
> }
>
> @@ -480,23 +458,17 @@ static int add_existing_master_key(struct
> fscrypt_master_key *mk,
> struct fscrypt_master_key_secret *secret)
> {
> int err;
>
> /*
> - * If the current user is already in ->mk_users, then there's nothing to
> - * do. Otherwise, we need to add the user to ->mk_users. (Neither is
> - * applicable for v1 policy keys, which have NULL ->mk_users.)
> + * For v2 policy keys (FSCRYPT_KEY_SPEC_TYPE_IDENTIFIER): If the current
> + * user is already in ->mk_users, then there's nothing to do.
> + * Otherwise, add the user to ->mk_users.
> */
> - if (mk->mk_users) {
> - struct key *mk_user = find_master_key_user(mk);
> -
> - if (mk_user != ERR_PTR(-ENOKEY)) {
> - if (IS_ERR(mk_user))
> - return PTR_ERR(mk_user);
> - key_put(mk_user);
> + if (mk->mk_spec.type == FSCRYPT_KEY_SPEC_TYPE_IDENTIFIER) {
> + if (find_master_key_user(mk) != NULL)
> return 0;
> - }
> err = add_master_key_user(mk);
> if (err)
> return err;
> }
>
> @@ -890,11 +862,10 @@ int fscrypt_add_test_dummy_key(struct super_block *sb,
> int fscrypt_verify_key_added(struct super_block *sb,
> const u8 identifier[FSCRYPT_KEY_IDENTIFIER_SIZE])
> {
> struct fscrypt_key_specifier mk_spec;
> struct fscrypt_master_key *mk;
> - struct key *mk_user;
> int err;
>
> mk_spec.type = FSCRYPT_KEY_SPEC_TYPE_IDENTIFIER;
> memcpy(mk_spec.u.identifier, identifier, FSCRYPT_KEY_IDENTIFIER_SIZE);
>
> @@ -902,17 +873,14 @@ int fscrypt_verify_key_added(struct super_block *sb,
> if (!mk) {
> err = -ENOKEY;
> goto out;
> }
> down_read(&mk->mk_sem);
> - mk_user = find_master_key_user(mk);
> - if (IS_ERR(mk_user)) {
> - err = PTR_ERR(mk_user);
> - } else {
> - key_put(mk_user);
> + if (find_master_key_user(mk) != NULL)
> err = 0;
> - }
> + else
> + err = -ENOKEY;
> up_read(&mk->mk_sem);
> fscrypt_put_master_key(mk);
> out:
> if (err == -ENOKEY && capable(CAP_FOWNER))
> err = 0;
> @@ -1100,20 +1068,22 @@ static int do_remove_key(struct file *filp, void
> __user *_uarg, bool all_users)
> if (!mk)
> return -ENOKEY;
> down_write(&mk->mk_sem);
>
> /* If relevant, remove current user's (or all users) claim to the key */
> - if (mk->mk_users && mk->mk_users->keys.nr_leaves_on_tree != 0) {
> - if (all_users)
> - err = keyring_clear(mk->mk_users);
> - else
> + if (!list_empty(&mk->mk_users)) {
> + if (all_users) {
> + clear_mk_users(mk);
> + err = 0;
> + } else {
> err = remove_master_key_user(mk);
> + }
> if (err) {
> up_write(&mk->mk_sem);
> goto out_put_key;
> }
> - if (mk->mk_users->keys.nr_leaves_on_tree != 0) {
> + if (!list_empty(&mk->mk_users)) {
> /*
> * Other users have still added the key too. We removed
> * the current user's claim to the key, but we still
> * can't remove the key itself.
> */
> @@ -1195,10 +1165,12 @@ EXPORT_SYMBOL_GPL(fscrypt_ioctl_remove_key_all_users);
> int fscrypt_ioctl_get_key_status(struct file *filp, void __user *uarg)
> {
> struct super_block *sb = file_inode(filp)->i_sb;
> struct fscrypt_get_key_status_arg arg;
> struct fscrypt_master_key *mk;
> + kuid_t uid;
> + const struct fscrypt_master_key_user *mk_user;
> int err;
>
> if (copy_from_user(&arg, uarg, sizeof(arg)))
> return -EFAULT;
>
> @@ -1227,23 +1199,17 @@ int fscrypt_ioctl_get_key_status(struct file *filp,
> void __user *uarg)
> err = 0;
> goto out_release_key;
> }
>
> arg.status = FSCRYPT_KEY_STATUS_PRESENT;
> - if (mk->mk_users) {
> - struct key *mk_user;
>
> - arg.user_count = mk->mk_users->keys.nr_leaves_on_tree;
> - mk_user = find_master_key_user(mk);
> - if (!IS_ERR(mk_user)) {
> + uid = current_fsuid();
> + list_for_each_entry(mk_user, &mk->mk_users, link) {
> + arg.user_count++;
> + if (uid_eq(mk_user->uid, uid))
> arg.status_flags |=
> FSCRYPT_KEY_STATUS_FLAG_ADDED_BY_SELF;
> - key_put(mk_user);
> - } else if (mk_user != ERR_PTR(-ENOKEY)) {
> - err = PTR_ERR(mk_user);
> - goto out_release_key;
> - }
> }
> err = 0;
> out_release_key:
> up_read(&mk->mk_sem);
> fscrypt_put_master_key(mk);
>
> base-commit: 83f1454877cc292b88baf13c829c16ce6937d120
> prerequisite-patch-id: 319d2891e88c7df1ebb5ebf434d18b68f770399f
> prerequisite-patch-id: 5330c9e4b65644baae81bd177a46be6223d2b494
> --
> 2.54.0
>
