Re: [PATCH v3] keys: finalize persistent keyring timeout after link attempt

[Jarkko Sakkinen <jarkko@kernel.org>]

On Tue, Sep 01, 2026 at 09:17:43PM +0200, Karl Mehltretter wrote:
> When no keyring exists for the requested UID, KEYCTL_GET_PERSISTENT
> creates and registers one before linking it to the requested destination.
> The configured timeout is set only after the destination link succeeds.
> 
> A destination restricted with KEYCTL_RESTRICT_KEYRING makes that link fail
> with -EPERM. With persistent_keyring_expiry set to 60 seconds, /proc/keys
> still reports the registered keyring's expiry as "perm".
> The failed call therefore leaves a quota-exempt keyring in the namespace's
> hidden register, where it may remain until namespace teardown.
> 
> Rename the write-locked helper to key_get_or_create_persistent() and return
> the key reference through a result parameter. Return 0 when it creates a
> keyring and 1 when the retry finds an existing one. Return a negative error
> on failure. Another caller may create the keyring between the initial
> read-locked lookup and the retry under the write lock.
> 
> Set the timeout after permission checking and linking. Do this on success,
> or on failure if the call allocated the keyring. This leaves a new keyring
> collectible after failure without starting its timeout while linking is
> still in progress.
> 
> Fixes: f36f8c75ae2e ("KEYS: Add per-user_namespace registers for persistent per-UID kerberos caches")
> Assisted-by: LLM
> Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
> ---
> Changes in v3:
> - Return the persistent key reference through a result parameter (Jarkko).
> - Keep lookup-or-create atomic inside the helper and use 0 for creation, 1
>   for an existing keyring, and negative values only for errors.
> 
> v2: https://lore.kernel.org/r/20260901164323.35785-1-kmehltretter@gmail.com/
> 
> Changes in v2:
> - Rewrite the commit message around the restricted-destination reproducer
>   and trim the explanation (Jarkko).
> - Track whether the persistent keyring was newly allocated and finalize its
>   timeout after the permission and link checks. This preserves an existing
>   keyring's timeout on failure and avoids expiry during linking.
> 
> Tested with QEMU 10.2.1 TCG on i386 and x86_64. With
> persistent_keyring_expiry set to 60 seconds, KEYCTL_GET_PERSISTENT returned
> -EPERM and /proc/keys reported "perm" before the fix and "1m" after it on
> both architectures.
> 
> Also tested on x86_64 with persistent_keyring_expiry set to 1 second and
> gc_delay set to 0. Successful calls reused the same serial, a newly created
> keyring from a failed call expired, and a failed call using an existing
> keyring left its timeout unchanged. A test-only two-second delay before
> timeout finalization did not allow the keyring to be collected before link.
> 
> Two concurrent callers returned the same serial. A test-only 100 ms delay
> after the read-locked miss forced both callers into the write-locked retry
> and confirmed that the second caller took the 1 (existing) return path.
> 
>  security/keys/persistent.c | 55 ++++++++++++++++++++++----------------
>  1 file changed, 32 insertions(+), 23 deletions(-)
> 
> diff --git a/security/keys/persistent.c b/security/keys/persistent.c
> index 97af230aa4b22..e99929b81a1a2 100644
> --- a/security/keys/persistent.c
> +++ b/security/keys/persistent.c
> @@ -33,25 +33,31 @@ static int key_create_persistent_register(struct user_namespace *ns)
>  }
>  
>  /*
> - * Create the persistent keyring for the specified user.
> + * Get or create the persistent keyring for the specified user.
>   *
>   * Called with the namespace's sem locked for writing.
> + *
> + * Return 0 if a keyring was created, 1 if an existing keyring was found, or a
> + * negative error.  On a nonnegative return, persistent_ref holds a reference
> + * to the keyring.
>   */
> -static key_ref_t key_create_persistent(struct user_namespace *ns, kuid_t uid,
> -				       struct keyring_index_key *index_key)
> +static int key_get_or_create_persistent(struct user_namespace *ns, kuid_t uid,
> +					struct keyring_index_key *index_key,
> +					key_ref_t *persistent_ref)
>  {
>  	struct key *persistent;
> -	key_ref_t reg_ref, persistent_ref;
> +	key_ref_t reg_ref;
>  
>  	if (!ns->persistent_keyring_register) {
> -		long err = key_create_persistent_register(ns);
> +		int err = key_create_persistent_register(ns);
> +
>  		if (err < 0)
> -			return ERR_PTR(err);
> +			return err;
>  	} else {
>  		reg_ref = make_key_ref(ns->persistent_keyring_register, true);
> -		persistent_ref = find_key_to_update(reg_ref, index_key);
> -		if (persistent_ref)
> -			return persistent_ref;
> +		*persistent_ref = find_key_to_update(reg_ref, index_key);
> +		if (*persistent_ref)
> +			return 1;

I'd return -ENOENT here instead and not make return value tristate.

>  	}
>  
>  	persistent = keyring_alloc(index_key->description,
> @@ -61,9 +67,10 @@ static key_ref_t key_create_persistent(struct user_namespace *ns, kuid_t uid,
>  				   KEY_ALLOC_NOT_IN_QUOTA, NULL,
>  				   ns->persistent_keyring_register);
>  	if (IS_ERR(persistent))
> -		return ERR_CAST(persistent);
> +		return PTR_ERR(persistent);
>  
> -	return make_key_ref(persistent, true);
> +	*persistent_ref = make_key_ref(persistent, true);
> +	return 0;
>  }
>  
>  /*
> @@ -78,6 +85,7 @@ static long key_get_persistent(struct user_namespace *ns, kuid_t uid,
>  	key_ref_t reg_ref, persistent_ref;
>  	char buf[32];
>  	long ret;
> +	bool created = false;
>  
>  	/* Look in the register if it exists */
>  	memset(&index_key, 0, sizeof(index_key));
> @@ -100,23 +108,24 @@ static long key_get_persistent(struct user_namespace *ns, kuid_t uid,
>  	 * also need to create the register.
>  	 */
>  	down_write(&ns->keyring_sem);
> -	persistent_ref = key_create_persistent(ns, uid, &index_key);
> +	ret = key_get_or_create_persistent(ns, uid, &index_key,
> +					   &persistent_ref);
>  	up_write(&ns->keyring_sem);
> -	if (!IS_ERR(persistent_ref))
> -		goto found;
> -
> -	return PTR_ERR(persistent_ref);
> +	if (ret < 0)
> +		return ret;
> +	created = ret == 0;
>  
>  found:
> +	persistent = key_ref_to_ptr(persistent_ref);
>  	ret = key_task_permission(persistent_ref, current_cred(), KEY_NEED_LINK);
> -	if (ret == 0) {
> -		persistent = key_ref_to_ptr(persistent_ref);
> +	if (ret == 0)
>  		ret = key_link(key_ref_to_ptr(dest_ref), persistent);
> -		if (ret == 0) {
> -			key_set_timeout(persistent, persistent_keyring_expiry);
> -			ret = persistent->serial;
> -		}
> -	}
> +
> +	if (ret == 0 || created)
> +		key_set_timeout(persistent, persistent_keyring_expiry);
> +
> +	if (ret == 0)
> +		ret = persistent->serial;
>  
>  	key_ref_put(persistent_ref);
>  	return ret;
> 
> base-commit: 08dbfad3f5040f5bdb6c529da20d6d4e81fefd72
> -- 
> 2.53.0

BR, Jarkko