[PATCH] KEYS: Reduce smp_mb() calls in key_put()

[PATCH] KEYS: Reduce smp_mb() calls in key_put()

[Jarkko Sakkinen]

Rely only on the memory ordering of spin_unlock() when setting
KEY_FLAG_FINAL_PUT under key->user->lock in key_put().

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 security/keys/key.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/security/keys/key.c b/security/keys/key.c
index 7198cd2ac3a3..aecbd624612d 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -656,10 +656,12 @@ void key_put(struct key *key)
 				spin_lock_irqsave(&key->user->lock, flags);
 				key->user->qnkeys--;
 				key->user->qnbytes -= key->quotalen;
+				set_bit(KEY_FLAG_FINAL_PUT, &key->flags);
 				spin_unlock_irqrestore(&key->user->lock, flags);
+			} else {
+				set_bit(KEY_FLAG_FINAL_PUT, &key->flags);
+				smp_mb(); /* key->user before FINAL_PUT set. */
 			}
-			smp_mb(); /* key->user before FINAL_PUT set. */
-			set_bit(KEY_FLAG_FINAL_PUT, &key->flags);
 			schedule_work(&key_gc_work);
 		}
 	}
-- 
2.47.2

Re: [PATCH] KEYS: Reduce smp_mb() calls in key_put()

[Jarkko Sakkinen]

On Wed, Apr 30, 2025 at 06:25:53PM +0300, Jarkko Sakkinen wrote:
> Rely only on the memory ordering of spin_unlock() when setting
> KEY_FLAG_FINAL_PUT under key->user->lock in key_put().
> 
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
>  security/keys/key.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/security/keys/key.c b/security/keys/key.c
> index 7198cd2ac3a3..aecbd624612d 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -656,10 +656,12 @@ void key_put(struct key *key)
>  				spin_lock_irqsave(&key->user->lock, flags);
>  				key->user->qnkeys--;
>  				key->user->qnbytes -= key->quotalen;
> +				set_bit(KEY_FLAG_FINAL_PUT, &key->flags);
>  				spin_unlock_irqrestore(&key->user->lock, flags);
> +			} else {
> +				set_bit(KEY_FLAG_FINAL_PUT, &key->flags);
> +				smp_mb(); /* key->user before FINAL_PUT set. */
>  			}
> -			smp_mb(); /* key->user before FINAL_PUT set. */
> -			set_bit(KEY_FLAG_FINAL_PUT, &key->flags);

Oops, my bad (order swap), sorry. Should have been:
	
 				spin_unlock_irqrestore(&key->user->lock, flags);
			} else {
				smp_mb(); /* key->user before FINAL_PUT set. */
 			}
			set_bit(KEY_FLAG_FINAL_PUT, &key->flags);

Should spin_lock()/unlock() be good enough or what good does smp_mb() do
in that branch? Just checking if I'm missing something before sending
fixed version.

BR, Jarkko

Re: [PATCH] KEYS: Reduce smp_mb() calls in key_put()

[Herbert Xu]

On Sat, May 03, 2025 at 05:39:16PM +0300, Jarkko Sakkinen wrote:
> On Wed, Apr 30, 2025 at 06:25:53PM +0300, Jarkko Sakkinen wrote:
> > Rely only on the memory ordering of spin_unlock() when setting
> > KEY_FLAG_FINAL_PUT under key->user->lock in key_put().
> > 
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> >  security/keys/key.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/security/keys/key.c b/security/keys/key.c
> > index 7198cd2ac3a3..aecbd624612d 100644
> > --- a/security/keys/key.c
> > +++ b/security/keys/key.c
> > @@ -656,10 +656,12 @@ void key_put(struct key *key)
> >  				spin_lock_irqsave(&key->user->lock, flags);
> >  				key->user->qnkeys--;
> >  				key->user->qnbytes -= key->quotalen;
> > +				set_bit(KEY_FLAG_FINAL_PUT, &key->flags);
> >  				spin_unlock_irqrestore(&key->user->lock, flags);
> > +			} else {
> > +				set_bit(KEY_FLAG_FINAL_PUT, &key->flags);
> > +				smp_mb(); /* key->user before FINAL_PUT set. */
> >  			}
> > -			smp_mb(); /* key->user before FINAL_PUT set. */
> > -			set_bit(KEY_FLAG_FINAL_PUT, &key->flags);
> 
> Oops, my bad (order swap), sorry. Should have been:
> 	
>  				spin_unlock_irqrestore(&key->user->lock, flags);
> 			} else {
> 				smp_mb(); /* key->user before FINAL_PUT set. */

You can use smp_mb__before_atomic here as it is equivalent to
smp_mb in this situation.

>  			}
> 			set_bit(KEY_FLAG_FINAL_PUT, &key->flags);
> 
> Should spin_lock()/unlock() be good enough or what good does smp_mb() do
> in that branch? Just checking if I'm missing something before sending
> fixed version.

I don't think spin_unlock alone is enough to replace an smp_mb.
A spin_lock + spin_unlock would be enough though.

However, looking at the bigger picture this smp_mb looks bogus.
What exactly is it protecting against?

The race condition that this is supposed to fix should have been
dealt with by the set_bit/test_bit of FINAL_PUT alone.  I don't
see any point in having this smb_mb at all.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] KEYS: Reduce smp_mb() calls in key_put()

[Jarkko Sakkinen]

On Sat, May 03, 2025 at 11:02:57PM +0800, Herbert Xu wrote:
> On Sat, May 03, 2025 at 05:39:16PM +0300, Jarkko Sakkinen wrote:
> > On Wed, Apr 30, 2025 at 06:25:53PM +0300, Jarkko Sakkinen wrote:
> > > Rely only on the memory ordering of spin_unlock() when setting
> > > KEY_FLAG_FINAL_PUT under key->user->lock in key_put().
> > > 
> > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > ---
> > >  security/keys/key.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/security/keys/key.c b/security/keys/key.c
> > > index 7198cd2ac3a3..aecbd624612d 100644
> > > --- a/security/keys/key.c
> > > +++ b/security/keys/key.c
> > > @@ -656,10 +656,12 @@ void key_put(struct key *key)
> > >  				spin_lock_irqsave(&key->user->lock, flags);
> > >  				key->user->qnkeys--;
> > >  				key->user->qnbytes -= key->quotalen;
> > > +				set_bit(KEY_FLAG_FINAL_PUT, &key->flags);
> > >  				spin_unlock_irqrestore(&key->user->lock, flags);
> > > +			} else {
> > > +				set_bit(KEY_FLAG_FINAL_PUT, &key->flags);
> > > +				smp_mb(); /* key->user before FINAL_PUT set. */
> > >  			}
> > > -			smp_mb(); /* key->user before FINAL_PUT set. */
> > > -			set_bit(KEY_FLAG_FINAL_PUT, &key->flags);
> > 
> > Oops, my bad (order swap), sorry. Should have been:
> > 	
> >  				spin_unlock_irqrestore(&key->user->lock, flags);
> > 			} else {
> > 				smp_mb(); /* key->user before FINAL_PUT set. */
> 
> You can use smp_mb__before_atomic here as it is equivalent to
> smp_mb in this situation.
> 
> >  			}
> > 			set_bit(KEY_FLAG_FINAL_PUT, &key->flags);
> > 
> > Should spin_lock()/unlock() be good enough or what good does smp_mb() do
> > in that branch? Just checking if I'm missing something before sending
> > fixed version.
> 
> I don't think spin_unlock alone is enough to replace an smp_mb.
> A spin_lock + spin_unlock would be enough though.
> 
> However, looking at the bigger picture this smp_mb looks bogus.
> What exactly is it protecting against?
> 
> The race condition that this is supposed to fix should have been
> dealt with by the set_bit/test_bit of FINAL_PUT alone.  I don't
> see any point in having this smb_mb at all.

smp_mb() there makes sure that key->user change don't spill between
key_put() and gc.

GC pairs smp_mb() in key_put() after FINAL_PUT to make sure that also
in its side key->user changes have been walled before moving the key
as part of unrefenced keys.

See also [1]. It cleared this up for me. Here user->lock easily misleads
to overlook the actual synchronization scheme.

> 
> Cheers,
> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[1] https://lore.kernel.org/keyrings/1121543.1746310761@warthog.procyon.org.uk/

BR, Jarkko

Re: [PATCH] KEYS: Reduce smp_mb() calls in key_put()

[David Howells]

Jarkko Sakkinen <jarkko@kernel.org> wrote:

> Oops, my bad (order swap), sorry. Should have been:
> 	
>  				spin_unlock_irqrestore(&key->user->lock, flags);
> 			} else {
> 				smp_mb(); /* key->user before FINAL_PUT set. */
>  			}
> 			set_bit(KEY_FLAG_FINAL_PUT, &key->flags);
> 
> Should spin_lock()/unlock() be good enough or what good does smp_mb() do
> in that branch? Just checking if I'm missing something before sending
> fixed version.

spin_unlock() is semi-permeable, so stuff after it can leak into the inside of
it up as far as the spin_lock().  With your change, the garbage collector can
no longer guarantee that key_put() will have done with accessing key->user
when it sees KEY_FLAG_FINAL_PUT is set.

So, NAK on this patch, I think.  If you want a second opinion, I'd suggest
waving it in front of Paul McKenney.

Possibly we only need smp_mb() in the IN_QUOTA branch in key_put().

David

Re: [PATCH] KEYS: Reduce smp_mb() calls in key_put()

[Herbert Xu]

On Sat, May 03, 2025 at 11:19:21PM +0100, David Howells wrote:
>
> Possibly we only need smp_mb() in the IN_QUOTA branch in key_put().

Just change the smp_mb to smp_mb__before_atomic, at least on x86
it just disappears because set_bit is already a serialising operation.

Or even better, reverse the FINAL_PUT bit and call it ALIVE, so
that you can use test_bit_acquire and clear_bit_unlock.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH] KEYS: Invert FINAL_PUT bit

[Herbert Xu]

On Sun, May 04, 2025 at 08:35:57AM +0800, Herbert Xu wrote:
> 
> Or even better, reverse the FINAL_PUT bit and call it ALIVE, so
> that you can use test_bit_acquire and clear_bit_unlock.

Something like:

---8<---
Invert the FINAL_PUT bit so that test_bit_acquire and clear_bit_unlock
can be used instead of smp_mb.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/include/linux/key.h b/include/linux/key.h
index ba05de8579ec..aaab26d84d25 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -236,7 +236,7 @@ struct key {
 #define KEY_FLAG_ROOT_CAN_INVAL	7	/* set if key can be invalidated by root without permission */
 #define KEY_FLAG_KEEP		8	/* set if key should not be removed */
 #define KEY_FLAG_UID_KEYRING	9	/* set if key is a user or user session keyring */
-#define KEY_FLAG_FINAL_PUT	10	/* set if final put has happened on key */
+#define KEY_FLAG_DONT_GC_YET	10	/* set if final put has not happened on key yet */
 
 	/* the key type and key description string
 	 * - the desc is used to match a key against search criteria
diff --git a/security/keys/gc.c b/security/keys/gc.c
index f27223ea4578..d00002054ada 100644
--- a/security/keys/gc.c
+++ b/security/keys/gc.c
@@ -218,8 +218,8 @@ static void key_garbage_collector(struct work_struct *work)
 		key = rb_entry(cursor, struct key, serial_node);
 		cursor = rb_next(cursor);
 
-		if (test_bit(KEY_FLAG_FINAL_PUT, &key->flags)) {
-			smp_mb(); /* Clobber key->user after FINAL_PUT seen. */
+		if (test_bit_acquire(KEY_FLAG_DONT_GC_YET, &key->flags)) {
+			/* Clobber key->user after final put seen. */
 			goto found_unreferenced_key;
 		}
 
diff --git a/security/keys/key.c b/security/keys/key.c
index 7198cd2ac3a3..7be12d132c4e 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -298,6 +298,7 @@ struct key *key_alloc(struct key_type *type, const char *desc,
 	key->restrict_link = restrict_link;
 	key->last_used_at = ktime_get_real_seconds();
 
+	key->flags |= KEY_FLAG_DONT_GC_YET;
 	if (!(flags & KEY_ALLOC_NOT_IN_QUOTA))
 		key->flags |= 1 << KEY_FLAG_IN_QUOTA;
 	if (flags & KEY_ALLOC_BUILT_IN)
@@ -658,8 +659,8 @@ void key_put(struct key *key)
 				key->user->qnbytes -= key->quotalen;
 				spin_unlock_irqrestore(&key->user->lock, flags);
 			}
-			smp_mb(); /* key->user before FINAL_PUT set. */
-			set_bit(KEY_FLAG_FINAL_PUT, &key->flags);
+			/* Mark key as safe for GC after key->user done. */
+			clear_bit_unlock(KEY_FLAG_DONT_GC_YET, &key->flags);
 			schedule_work(&key_gc_work);
 		}
 	}
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] KEYS: Invert FINAL_PUT bit

[David Howells]

Herbert Xu <herbert@gondor.apana.org.au> wrote:

> +	key->flags |= KEY_FLAG_DONT_GC_YET;

You need __set_bit() or 1<<N.

Also, don't really like the name, but that's just bikeshedding.  I think I'd
lean more to your initial suggestion of KEY_FLAG_ALIVE.

David

[v2 PATCH] KEYS: Invert FINAL_PUT bit

[Herbert Xu]

On Sun, May 04, 2025 at 08:44:13AM +0100, David Howells wrote:
> 
> You need __set_bit() or 1<<N.

Sorry, I'll fix that.

> Also, don't really like the name, but that's just bikeshedding.  I think I'd
> lean more to your initial suggestion of KEY_FLAG_ALIVE.

I was going to do that but there is already a flag called
KEY_FLAG_DEAD and it would be very confusing since they mean
completely diferent things.

How about USER_ALIVE?

---8<---
Invert the FINAL_PUT bit so that test_bit_acquire and clear_bit_unlock
can be used instead of smp_mb.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/include/linux/key.h b/include/linux/key.h
index ba05de8579ec..aaab26d84d25 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -236,7 +236,7 @@ struct key {
 #define KEY_FLAG_ROOT_CAN_INVAL	7	/* set if key can be invalidated by root without permission */
 #define KEY_FLAG_KEEP		8	/* set if key should not be removed */
 #define KEY_FLAG_UID_KEYRING	9	/* set if key is a user or user session keyring */
-#define KEY_FLAG_FINAL_PUT	10	/* set if final put has happened on key */
+#define KEY_FLAG_USER_ALIVE	10	/* set if final put has not happened on key yet */
 
 	/* the key type and key description string
 	 * - the desc is used to match a key against search criteria
diff --git a/security/keys/gc.c b/security/keys/gc.c
index f27223ea4578..d00002054ada 100644
--- a/security/keys/gc.c
+++ b/security/keys/gc.c
@@ -218,8 +218,8 @@ static void key_garbage_collector(struct work_struct *work)
 		key = rb_entry(cursor, struct key, serial_node);
 		cursor = rb_next(cursor);
 
-		if (test_bit(KEY_FLAG_FINAL_PUT, &key->flags)) {
-			smp_mb(); /* Clobber key->user after FINAL_PUT seen. */
+		if (test_bit_acquire(KEY_FLAG_USER_ALIVE, &key->flags)) {
+			/* Clobber key->user after final put seen. */
 			goto found_unreferenced_key;
 		}
 
diff --git a/security/keys/key.c b/security/keys/key.c
index 7198cd2ac3a3..fb78c3a0be76 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -298,6 +298,7 @@ struct key *key_alloc(struct key_type *type, const char *desc,
 	key->restrict_link = restrict_link;
 	key->last_used_at = ktime_get_real_seconds();
 
+	key->flags |= 1 << KEY_FLAG_USER_ALIVE;
 	if (!(flags & KEY_ALLOC_NOT_IN_QUOTA))
 		key->flags |= 1 << KEY_FLAG_IN_QUOTA;
 	if (flags & KEY_ALLOC_BUILT_IN)
@@ -658,8 +659,8 @@ void key_put(struct key *key)
 				key->user->qnbytes -= key->quotalen;
 				spin_unlock_irqrestore(&key->user->lock, flags);
 			}
-			smp_mb(); /* key->user before FINAL_PUT set. */
-			set_bit(KEY_FLAG_FINAL_PUT, &key->flags);
+			/* Mark key as safe for GC after key->user done. */
+			clear_bit_unlock(KEY_FLAG_USER_ALIVE, &key->flags);
 			schedule_work(&key_gc_work);
 		}
 	}

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [v2 PATCH] KEYS: Invert FINAL_PUT bit

[kernel test robot]

Hello,


our bot applied this patch directly upon v6.15-rc5. could you let us know if
this is a correct appliment?

* a78cdfa4388ab9 (linux-review/Herbert-Xu/KEYS-Invert-FINAL_PUT-bit/20250505-122533) KEYS: Invert FINAL_PUT bit
* 92a09c47464d04 (tag: v6.15-rc5, 


below reports is based on this appliement.


kernel test robot noticed "refcount_t:underflow;use-after-free" on:

commit: a78cdfa4388ab9b210c804b92453f14bbe199cbf ("[v2 PATCH] KEYS: Invert FINAL_PUT bit")
url: https://github.com/intel-lab-lkp/linux/commits/Herbert-Xu/KEYS-Invert-FINAL_PUT-bit/20250505-122533
base: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git 92a09c47464d040866cf2b4cd052bc60555185fb
patch link: https://lore.kernel.org/all/aBccz2nJs5Asg6cN@gondor.apana.org.au/
patch subject: [v2 PATCH] KEYS: Invert FINAL_PUT bit

in testcase: trinity
version: trinity-i386-abe9de86-1_20230429
with following parameters:

	runtime: 300s
	group: group-04
	nr_groups: 5



config: i386-randconfig-014-20250509
compiler: gcc-12
test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G

(please refer to attached dmesg/kmsg for entire log/backtrace)


there are other (random) issues as below.

+-------------------------------------------------------------------------+-----------+------------+
|                                                                         | v6.15-rc5 | a78cdfa438 |
+-------------------------------------------------------------------------+-----------+------------+
| boot_successes                                                          | 80        | 0          |
| boot_failures                                                           | 0         | 48         |
| refcount_t:underflow;use-after-free                                     | 0         | 48         |
| WARNING:at_lib/refcount.c:#refcount_warn_saturate                       | 0         | 47         |
| EIP:refcount_warn_saturate                                              | 0         | 48         |
| addition_on#;use-after-free                                             | 0         | 46         |
| saturated;leaking_memory                                                | 0         | 44         |
| BUG:kernel_NULL_pointer_dereference,address                             | 0         | 31         |
| Oops                                                                    | 0         | 41         |
| EIP:keyctl_read_key                                                     | 0         | 27         |
| Kernel_panic-not_syncing:Fatal_exception                                | 0         | 36         |
| BUG:unable_to_handle_page_fault_for_address                             | 0         | 10         |
| EIP:key_put                                                             | 0         | 1          |
| Kernel_panic-not_syncing:Fatal_exception_in_interrupt                   | 0         | 5          |
| EIP:kmem_cache_alloc_noprof                                             | 0         | 2          |
| BUG:Bad_rss-counter_state_mm:#type:MM_SWAPENTS_val                      | 0         | 1          |
| EIP:keyctl_describe_key                                                 | 0         | 1          |
| EIP:keyring_gc_check_iterator                                           | 0         | 1          |
| EIP:dst_destroy                                                         | 0         | 3          |
| EIP:_raw_spin_unlock_irqrestore                                         | 0         | 1          |
| EIP:put_pid                                                             | 0         | 4          |
| EIP:rb_erase                                                            | 0         | 1          |
| EIP:kernel_init_pages                                                   | 0         | 1          |
| EIP:lookup_user_key                                                     | 0         | 1          |
| EIP:strlen                                                              | 0         | 1          |
| INFO:task_blocked_for_more_than#seconds                                 | 0         | 1          |
| BUG:kernel_hang_in_test_stage                                           | 0         | 1          |
+-------------------------------------------------------------------------+-----------+------------+


If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202505091721.245cbe78-lkp@intel.com


[    8.510562][   T60] ------------[ cut here ]------------
[    8.511283][   T60] refcount_t: underflow; use-after-free.
[ 8.511950][ T60] WARNING: CPU: 0 PID: 60 at lib/refcount.c:28 refcount_warn_saturate (kbuild/obj/consumer/i386-randconfig-014-20250509/lib/refcount.c:28 (discriminator 3)) 
[    8.512948][   T60] Modules linked in:
[    8.513488][   T60] CPU: 0 UID: 0 PID: 60 Comm: kworker/0:2 Not tainted 6.15.0-rc5-00001-ga78cdfa4388a #1 PREEMPT  231a29fdcec5e4259d3c91818150ae4baf2b3615
[    8.514973][   T60] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[    8.516145][   T60] Workqueue: events key_garbage_collector
[ 8.516849][ T60] EIP: refcount_warn_saturate (kbuild/obj/consumer/i386-randconfig-014-20250509/lib/refcount.c:28 (discriminator 3)) 
[ 8.517490][ T60] Code: fa c2 82 01 68 28 15 60 82 e8 e3 88 72 ff 0f 0b 58 c9 c3 8d b6 00 00 00 00 c6 05 2e fa c2 82 01 68 d0 14 60 82 e8 c7 88 72 ff <0f> 0b 59 c9 c3 66 90 89 c2 8b 00 3d 00 00 00 c0 74 12 83 f8 01 74
All code
========
   0:	fa                   	cli
   1:	c2 82 01             	ret    $0x182
   4:	68 28 15 60 82       	push   $0xffffffff82601528
   9:	e8 e3 88 72 ff       	call   0xffffffffff7288f1
   e:	0f 0b                	ud2
  10:	58                   	pop    %rax
  11:	c9                   	leave
  12:	c3                   	ret
  13:	8d b6 00 00 00 00    	lea    0x0(%rsi),%esi
  19:	c6 05 2e fa c2 82 01 	movb   $0x1,-0x7d3d05d2(%rip)        # 0xffffffff82c2fa4e
  20:	68 d0 14 60 82       	push   $0xffffffff826014d0
  25:	e8 c7 88 72 ff       	call   0xffffffffff7288f1
  2a:*	0f 0b                	ud2		<-- trapping instruction
  2c:	59                   	pop    %rcx
  2d:	c9                   	leave
  2e:	c3                   	ret
  2f:	66 90                	xchg   %ax,%ax
  31:	89 c2                	mov    %eax,%edx
  33:	8b 00                	mov    (%rax),%eax
  35:	3d 00 00 00 c0       	cmp    $0xc0000000,%eax
  3a:	74 12                	je     0x4e
  3c:	83 f8 01             	cmp    $0x1,%eax
  3f:	74                   	.byte 0x74

Code starting with the faulting instruction
===========================================
   0:	0f 0b                	ud2
   2:	59                   	pop    %rcx
   3:	c9                   	leave
   4:	c3                   	ret
   5:	66 90                	xchg   %ax,%ax
   7:	89 c2                	mov    %eax,%edx
   9:	8b 00                	mov    (%rax),%eax
   b:	3d 00 00 00 c0       	cmp    $0xc0000000,%eax
  10:	74 12                	je     0x24
  12:	83 f8 01             	cmp    $0x1,%eax
  15:	74                   	.byte 0x74
[    8.519470][   T60] EAX: 00000026 EBX: 85c8c9c0 ECX: 0000025c EDX: 00000000
[    8.520241][   T60] ESI: 85d4ede0 EDI: 821a0f00 EBP: 8405fe6c ESP: 8405fe68
[    8.521168][   T60] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 EFLAGS: 00010286
[    8.522055][   T60] CR0: 80050033 CR2: 77ecb6a1 CR3: 040b8000 CR4: 000406f0
[    8.522824][   T60] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[    8.523614][   T60] DR6: fffe0ff0 DR7: 00000400
[    8.524161][   T60] Call Trace:
[ 8.524619][ T60] key_put (kbuild/obj/consumer/i386-randconfig-014-20250509/include/linux/refcount.h:400 kbuild/obj/consumer/i386-randconfig-014-20250509/include/linux/refcount.h:432 kbuild/obj/consumer/i386-randconfig-014-20250509/include/linux/refcount.h:450 kbuild/obj/consumer/i386-randconfig-014-20250509/security/keys/key.c:652) 
[ 8.525119][ T60] keyring_free_object (kbuild/obj/consumer/i386-randconfig-014-20250509/security/keys/keyring.c:390) 
[ 8.525736][ T60] assoc_array_destroy_subtree+0x7b/0x17c 
[ 8.526446][ T60] assoc_array_destroy (kbuild/obj/consumer/i386-randconfig-014-20250509/lib/assoc_array.c:445) 
[ 8.527048][ T60] keyring_destroy (kbuild/obj/consumer/i386-randconfig-014-20250509/security/keys/keyring.c:432) 
[ 8.527617][ T60] key_gc_unused_keys+0xfb/0x134 
[ 8.528301][ T60] key_garbage_collector (kbuild/obj/consumer/i386-randconfig-014-20250509/security/keys/gc.c:305) 
[ 8.528967][ T60] process_one_work (kbuild/obj/consumer/i386-randconfig-014-20250509/kernel/workqueue.c:3243) 
[ 8.529586][ T60] worker_thread (kbuild/obj/consumer/i386-randconfig-014-20250509/kernel/workqueue.c:3313 kbuild/obj/consumer/i386-randconfig-014-20250509/kernel/workqueue.c:3400) 
[ 8.530157][ T60] kthread (kbuild/obj/consumer/i386-randconfig-014-20250509/kernel/kthread.c:464) 
[ 8.530681][ T60] ? rescuer_thread (kbuild/obj/consumer/i386-randconfig-014-20250509/kernel/workqueue.c:3346) 
[ 8.531244][ T60] ? kthread_fetch_affinity+0x34/0x34 
[ 8.531930][ T60] ret_from_fork (kbuild/obj/consumer/i386-randconfig-014-20250509/arch/x86/kernel/process.c:159) 
[ 8.532498][ T60] ? kthread_fetch_affinity+0x34/0x34 
[ 8.533164][ T60] ret_from_fork_asm (kbuild/obj/consumer/i386-randconfig-014-20250509/arch/x86/entry/entry_32.S:737) 
[ 8.533766][ T60] entry_INT80_32 (kbuild/obj/consumer/i386-randconfig-014-20250509/arch/x86/entry/entry_32.S:945) 
[    8.534333][   T60] irq event stamp: 3905
[ 8.534868][ T60] hardirqs last enabled at (3917): __up_console_sem (kbuild/obj/consumer/i386-randconfig-014-20250509/arch/x86/include/asm/irqflags.h:42 (discriminator 1) kbuild/obj/consumer/i386-randconfig-014-20250509/arch/x86/include/asm/irqflags.h:119 (discriminator 1) kbuild/obj/consumer/i386-randconfig-014-20250509/arch/x86/include/asm/irqflags.h:159 (discriminator 1) kbuild/obj/consumer/i386-randconfig-014-20250509/kernel/printk/printk.c:344 (discriminator 1)) 
[ 8.535880][ T60] hardirqs last disabled at (3928): __up_console_sem (kbuild/obj/consumer/i386-randconfig-014-20250509/kernel/printk/printk.c:342 (discriminator 1)) 
[ 8.535891][ T60] softirqs last enabled at (3856): handle_softirqs (kbuild/obj/consumer/i386-randconfig-014-20250509/kernel/softirq.c:426 kbuild/obj/consumer/i386-randconfig-014-20250509/kernel/softirq.c:607) 
[ 8.535896][ T60] softirqs last disabled at (3851): __do_softirq (kbuild/obj/consumer/i386-randconfig-014-20250509/kernel/softirq.c:614) 
[    8.535904][   T60] ---[ end trace 0000000000000000 ]---


The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20250509/202505091721.245cbe78-lkp@intel.com



-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[v3 PATCH] KEYS: Invert FINAL_PUT bit

[Herbert Xu]

On Fri, May 09, 2025 at 05:34:09PM +0800, kernel test robot wrote:
> 
> our bot applied this patch directly upon v6.15-rc5. could you let us know if
> this is a correct appliment?

Yes it is correct.

The patch was buggy, the test_bit_acquire should've been inverted too:

---8<---
Invert the FINAL_PUT bit so that test_bit_acquire and clear_bit_unlock
can be used instead of smp_mb.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/include/linux/key.h b/include/linux/key.h
index ba05de8579ec..81b8f05c6898 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -236,7 +236,7 @@ struct key {
 #define KEY_FLAG_ROOT_CAN_INVAL	7	/* set if key can be invalidated by root without permission */
 #define KEY_FLAG_KEEP		8	/* set if key should not be removed */
 #define KEY_FLAG_UID_KEYRING	9	/* set if key is a user or user session keyring */
-#define KEY_FLAG_FINAL_PUT	10	/* set if final put has happened on key */
+#define KEY_FLAG_USER_ALIVE	10	/* set if final put has not happened on key yet */
 
 	/* the key type and key description string
 	 * - the desc is used to match a key against search criteria
diff --git a/security/keys/gc.c b/security/keys/gc.c
index f27223ea4578..748e83818a76 100644
--- a/security/keys/gc.c
+++ b/security/keys/gc.c
@@ -218,8 +218,8 @@ static void key_garbage_collector(struct work_struct *work)
 		key = rb_entry(cursor, struct key, serial_node);
 		cursor = rb_next(cursor);
 
-		if (test_bit(KEY_FLAG_FINAL_PUT, &key->flags)) {
-			smp_mb(); /* Clobber key->user after FINAL_PUT seen. */
+		if (!test_bit_acquire(KEY_FLAG_USER_ALIVE, &key->flags)) {
+			/* Clobber key->user after final put seen. */
 			goto found_unreferenced_key;
 		}
 
diff --git a/security/keys/key.c b/security/keys/key.c
index 7198cd2ac3a3..3bbdde778631 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -298,6 +298,7 @@ struct key *key_alloc(struct key_type *type, const char *desc,
 	key->restrict_link = restrict_link;
 	key->last_used_at = ktime_get_real_seconds();
 
+	key->flags |= 1 << KEY_FLAG_USER_ALIVE;
 	if (!(flags & KEY_ALLOC_NOT_IN_QUOTA))
 		key->flags |= 1 << KEY_FLAG_IN_QUOTA;
 	if (flags & KEY_ALLOC_BUILT_IN)
@@ -658,8 +659,8 @@ void key_put(struct key *key)
 				key->user->qnbytes -= key->quotalen;
 				spin_unlock_irqrestore(&key->user->lock, flags);
 			}
-			smp_mb(); /* key->user before FINAL_PUT set. */
-			set_bit(KEY_FLAG_FINAL_PUT, &key->flags);
+			/* Mark key as safe for GC after key->user done. */
+			clear_bit_unlock(KEY_FLAG_USER_ALIVE, &key->flags);
 			schedule_work(&key_gc_work);
 		}
 	}
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [v3 PATCH] KEYS: Invert FINAL_PUT bit

[David Howells]

Herbert Xu <herbert@gondor.apana.org.au> wrote:

> Invert the FINAL_PUT bit so that test_bit_acquire and clear_bit_unlock
> can be used instead of smp_mb.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Acked-by: David Howells <dhowells@redhat.com>

Re: [v3 PATCH] KEYS: Invert FINAL_PUT bit

[Jarkko Sakkinen]

On Mon, May 12, 2025 at 10:19:14AM +0100, David Howells wrote:
> Herbert Xu <herbert@gondor.apana.org.au> wrote:
> 
> > Invert the FINAL_PUT bit so that test_bit_acquire and clear_bit_unlock
> > can be used instead of smp_mb.
> > 
> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> Acked-by: David Howells <dhowells@redhat.com>

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

David, are you going to pick this?

BR, Jarkko

Re: [v3 PATCH] KEYS: Invert FINAL_PUT bit

[David Howells]

Jarkko Sakkinen <jarkko@kernel.org> wrote:

> > Acked-by: David Howells <dhowells@redhat.com>
> 
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> 
> David, are you going to pick this?

I can do unless Herbert wants to pass it through his tree?

David

Re: [v3 PATCH] KEYS: Invert FINAL_PUT bit

[Herbert Xu]

On Mon, May 12, 2025 at 01:01:50PM +0100, David Howells wrote:
>
> I can do unless Herbert wants to pass it through his tree?

I have no desire to do that :)

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] KEYS: Reduce smp_mb() calls in key_put()

[Jarkko Sakkinen]

On Sat, May 03, 2025 at 11:19:21PM +0100, David Howells wrote:
> Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> > Oops, my bad (order swap), sorry. Should have been:
> > 	
> >  				spin_unlock_irqrestore(&key->user->lock, flags);
> > 			} else {
> > 				smp_mb(); /* key->user before FINAL_PUT set. */
> >  			}
> > 			set_bit(KEY_FLAG_FINAL_PUT, &key->flags);
> > 
> > Should spin_lock()/unlock() be good enough or what good does smp_mb() do
> > in that branch? Just checking if I'm missing something before sending
> > fixed version.
> 
> spin_unlock() is semi-permeable, so stuff after it can leak into the inside of
> it up as far as the spin_lock().  With your change, the garbage collector can
> no longer guarantee that key_put() will have done with accessing key->user
> when it sees KEY_FLAG_FINAL_PUT is set.
> 
> So, NAK on this patch, I think.  If you want a second opinion, I'd suggest
> waving it in front of Paul McKenney.

Fair enough.

If I revisit this in a way or another,  I'll cc to him for comments but
for this I'll buy what you said.

> 
> Possibly we only need smp_mb() in the IN_QUOTA branch in key_put().
> 
> David

Thank you for the comments.

BR, Jarkko