Re: [PATCH] fscrypt: Replace mk_users keyring with simple list

[Luis Henriques <luis@igalia.com>]

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: syzbot+f55b043dacf43776b50c@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=f55b043dacf43776b50c
> Reported-by: Mohammed EL Kadiri <med08elkadiri@gmail.com>
> Closes: https://lore.kernel.org/keyrings/20260614150041.21172-1-med08elkadiri@gmail.com/
> Fixes: 23c688b54016 ("fscrypt: allow unprivileged users to add/remove keys for v2 policies")
> Cc: stable@vger.kernel.org
> Signed-off-by: Eric Biggers <ebiggers@kernel.org>
> ---
>
> 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
>