Re: [RFC PATCH] keys: flush work when accessing /proc/key-users

[Jarkko Sakkinen <jarkko@kernel.org>]

David, this really needs your feedback.

BR, Jarkko

On Wed, 2023-12-06 at 14:57 +0000, Luis Henriques wrote:
> Make sure the garbage collector has been run before cycling through
> all
> the user keys.
> 
> Signed-off-by: Luis Henriques <lhenriques@suse.de>
> ---
> Hi!
> 
> This patch is mostly for getting some feedback on how to fix an
> fstest
> failing for ext4/fscrypt (generic/581).  Basically, the test relies
> on the
> data read from /proc/key-users to be up-to-date regarding the number
> of
> keys a given user currently has.  However, this file can't be trusted
> because it races against the keys GC.
> 
> Using flush_work() seems to work (I can't reproduce the failure), but
> it
> may be overkill.  Or simply not acceptable.  Maybe, as Eric suggested
> elsewhere [1], there could be a synchronous
> key_put/revoke/invalidate/...,
> which would wait for the key GC to do its work, although that
> probably
> would require some more code re-work.
> 
> [1]
> https://lore.kernel.org/all/20231128173734.GD1148@sol.localdomain/
> 
>  security/keys/gc.c       | 6 ++++++
>  security/keys/internal.h | 1 +
>  security/keys/proc.c     | 1 +
>  3 files changed, 8 insertions(+)
> 
> diff --git a/security/keys/gc.c b/security/keys/gc.c
> index 3c90807476eb..57b5a54490a0 100644
> --- a/security/keys/gc.c
> +++ b/security/keys/gc.c
> @@ -44,6 +44,12 @@ struct key_type key_type_dead = {
>  	.name = ".dead",
>  };
>  
> +void key_flush_gc(void)
> +{
> +	kenter("");
> +	flush_work(&key_gc_work);
> +}
> +
>  /*
>   * Schedule a garbage collection run.
>   * - time precision isn't particularly important
> diff --git a/security/keys/internal.h b/security/keys/internal.h
> index 471cf36dedc0..fee1d0025d96 100644
> --- a/security/keys/internal.h
> +++ b/security/keys/internal.h
> @@ -170,6 +170,7 @@ extern void keyring_restriction_gc(struct key
> *keyring,
>  extern void key_schedule_gc(time64_t gc_at);
>  extern void key_schedule_gc_links(void);
>  extern void key_gc_keytype(struct key_type *ktype);
> +extern void key_flush_gc(void);
>  
>  extern int key_task_permission(const key_ref_t key_ref,
>  			       const struct cred *cred,
> diff --git a/security/keys/proc.c b/security/keys/proc.c
> index d0cde6685627..2837e00a240a 100644
> --- a/security/keys/proc.c
> +++ b/security/keys/proc.c
> @@ -277,6 +277,7 @@ static void *proc_key_users_start(struct seq_file
> *p, loff_t *_pos)
>  	struct rb_node *_p;
>  	loff_t pos = *_pos;
>  
> +	key_flush_gc();
>  	spin_lock(&key_user_lock);
>  
>  	_p = key_user_first(seq_user_ns(p), &key_user_tree);