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

[Jarkko Sakkinen]

On Tue, Sep 01, 2026 at 06:43:23PM +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.
> 
> Have key_create_persistent() report whether it allocated a new keyring.
> Another caller may create one 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@xxxxxxxxx>
> ---
> 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.
> 
>  security/keys/persistent.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/security/keys/persistent.c b/security/keys/persistent.c
> index 97af230aa4b22..f356df8f3c973 100644
> --- a/security/keys/persistent.c
> +++ b/security/keys/persistent.c
> @@ -38,7 +38,8 @@ static int key_create_persistent_register(struct user_namespace *ns)
>   * Called with the namespace's sem locked for writing.
>   */
>  static key_ref_t key_create_persistent(struct user_namespace *ns, kuid_t uid,
> -				       struct keyring_index_key *index_key)
> +				       struct keyring_index_key *index_key,
> +				       bool *created)

I'd instead convert the interface to have a result parameter:

static int key_create_persistent(struct user_namespace *ns, kuid_t uid,
				 struct keyring_index_key *index_key,
				 key_ref_t *persistent_ref)

And then return -ENOENT here:

	} 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 -ENOENT;
	}

Then deal with that errno at the call sites.

I think this make intuitively much more sense.

>  {
>  	struct key *persistent;
>  	key_ref_t reg_ref, persistent_ref;
> @@ -63,6 +64,8 @@ static key_ref_t key_create_persistent(struct user_namespace *ns, kuid_t uid,
>  	if (IS_ERR(persistent))
>  		return ERR_CAST(persistent);
>  
> +	*created = true;
> +
>  	return make_key_ref(persistent, true);
>  }
>  
> @@ -78,6 +81,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,7 +104,7 @@ 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);
> +	persistent_ref = key_create_persistent(ns, uid, &index_key, &created);
>  	up_write(&ns->keyring_sem);
>  	if (!IS_ERR(persistent_ref))
>  		goto found;
> @@ -108,15 +112,16 @@ static long key_get_persistent(struct user_namespace *ns, kuid_t uid,
>  	return PTR_ERR(persistent_ref);
>  
>  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