Re: [PATCH v2] keys: fix lost wakeup when reaping a dead key type

On Fri, Aug 21, 2026 at 04:53:27AM +0200, Karl Mehltretter wrote:
> clear_bit() is atomic with respect to the word it modifies, but it is
> an unordered operation: it implies no memory barrier on either side
> (Documentation/atomic_bitops.txt).
> 
> key_garbage_collector() clears KEY_GC_REAPING_KEYTYPE with clear_bit()
> and calls wake_up_bit() after reaping a dead key type. wake_up_bit()
> uses a lockless waitqueue check and requires a full barrier after the
> clear.
> 
> The existing smp_mb() is before clear_bit(), so nothing orders the clear
> against that check. The GC can see an empty waitqueue while
> unregister_key_type() still sees the bit set. The final wakeup is then
> lost, leaving module unload stuck in wait_on_bit().
> 
> Use clear_and_wake_up_bit(). Its clear_bit_unlock() has RELEASE
> semantics, so the completed GC work stays ordered before the clear, and
> its smp_mb__after_atomic() orders the clear before the waitqueue check.
> 
> Fixes: 0c061b5707ab ("KEYS: Correctly destroy key payloads when their keytype is removed")
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Karl Mehltretter <[email protected]>
> ---
> v2: open with the ordering semantics of clear_bit(), as suggested by
>     Jarkko. No code change.
> v1: https://lore.kernel.org/r/[email protected]/
> 
> LKMM (herdtools7 7.58). LKMM has no clear_bit*() primitives, so these
> tests abstract the bit clear as a store while preserving the ordering
> relevant to this race. The fixed test models clear_bit_unlock() with
> smp_store_release() and smp_mb__after_atomic() with smp_mb().
> 
>   C keys-gc-buggy
>   { flag=1; }
>   P0(int *flag, int *wq)
>   {
>   	int r0;
>   	smp_mb();
>   	WRITE_ONCE(*flag, 0);
>   	r0 = READ_ONCE(*wq);
>   }
>   P1(int *flag, int *wq)
>   {
>   	int r1;
>   	WRITE_ONCE(*wq, 1);
>   	smp_mb();
>   	r1 = READ_ONCE(*flag);
>   }
>   exists (0:r0=0 /\ 1:r1=1)
> 
>   C keys-gc-fixed
>   { flag=1; }
>   P0(int *flag, int *wq)
>   {
>   	int r0;
>   	smp_store_release(flag, 0);
>   	smp_mb();
>   	r0 = READ_ONCE(*wq);
>   }
>   P1(int *flag, int *wq)
>   {
>   	int r1;
>   	WRITE_ONCE(*wq, 1);
>   	smp_mb();
>   	r1 = READ_ONCE(*flag);
>   }
>   exists (0:r0=0 /\ 1:r1=1)
> 
>   herd7 -conf linux-kernel.cfg keys-gc-buggy.litmus
>   herd7 -conf linux-kernel.cfg keys-gc-fixed.litmus
> 
>   pre-fix: Sometimes
>   fixed:   Never
> 
>  security/keys/gc.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/security/keys/gc.c b/security/keys/gc.c
> index 748e83818a760..eda445f815d47 100644
> --- a/security/keys/gc.c
> +++ b/security/keys/gc.c
> @@ -318,9 +318,7 @@ static void key_garbage_collector(struct work_struct *work)
> 
>  	if (unlikely(gc_state & KEY_GC_REAPING_DEAD_3)) {
>  		kdebug("dead wake");
> -		smp_mb();
> -		clear_bit(KEY_GC_REAPING_KEYTYPE, &key_gc_flags);
> -		wake_up_bit(&key_gc_flags, KEY_GC_REAPING_KEYTYPE);
> +		clear_and_wake_up_bit(KEY_GC_REAPING_KEYTYPE, &key_gc_flags);
>  	}
> 
>  	if (gc_state & KEY_GC_REAP_AGAIN)
> --
> 2.53.0

Reviewed-by: Jarkko Sakkinen <[email protected]>

I send this to -rc2 as I already sent keys PR and I have quite heavy
TPM PR to prepare.

BR, Jarkko