[PATCH] KEYS: Invert FINAL_PUT bit

[PATCH] KEYS: Invert FINAL_PUT bit

[David Howells]

Hi Linus,

Could you apply this, please?  There shouldn't be any functional change,
rather it's a switch to using combined bit-barrier ops and lesser barriers.
A better way to do this might be to provide set_bit_release(), but the end
result would be much the same.

Thanks,
David
---
From: Herbert Xu <herbert@gondor.apana.org.au>

KEYS: Invert FINAL_PUT bit

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>
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
cc: keyrings@vger.kernel.org
cc: linux-security-module@vger.kernel.org
cc: linux-crypto@vger.kernel.org
cc: linux-integrity@vger.kernel.org
---
 include/linux/key.h |    2 +-
 security/keys/gc.c  |    4 ++--
 security/keys/key.c |    5 +++--
 3 files changed, 6 insertions(+), 5 deletions(-)

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

Re: [PATCH] KEYS: Invert FINAL_PUT bit

[Paul Moore]

On Wed, May 28, 2025 at 8:19 AM David Howells <dhowells@redhat.com> wrote:
>
> Hi Linus,
>
> Could you apply this, please?  There shouldn't be any functional change,
> rather it's a switch to using combined bit-barrier ops and lesser barriers.
> A better way to do this might be to provide set_bit_release(), but the end
> result would be much the same.
>
> Thanks,
> David
> ---
> From: Herbert Xu <herbert@gondor.apana.org.au>
>
> KEYS: Invert FINAL_PUT bit
>
> 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>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> cc: keyrings@vger.kernel.org
> cc: linux-security-module@vger.kernel.org
> cc: linux-crypto@vger.kernel.org
> cc: linux-integrity@vger.kernel.org
> ---
>  include/linux/key.h |    2 +-
>  security/keys/gc.c  |    4 ++--
>  security/keys/key.c |    5 +++--
>  3 files changed, 6 insertions(+), 5 deletions(-)

It doesn't look like this has made its way to Linus.  David or Jarkko,
do one of you want to pick this up into a tree and send this to Linus
properly?

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

-- 
paul-moore.com

Re: [PATCH] KEYS: Invert FINAL_PUT bit

[Herbert Xu]

On Tue, Jun 10, 2025 at 08:22:59PM -0400, Paul Moore wrote:
>
> It doesn't look like this has made its way to Linus.  David or Jarkko,
> do one of you want to pick this up into a tree and send this to Linus
> properly?

I can pick it up for the next merge window.

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: Invert FINAL_PUT bit

[Paul Moore]

On Wed, Jun 11, 2025 at 5:12 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Tue, Jun 10, 2025 at 08:22:59PM -0400, Paul Moore wrote:
> >
> > It doesn't look like this has made its way to Linus.  David or Jarkko,
> > do one of you want to pick this up into a tree and send this to Linus
> > properly?
>
> I can pick it up for the next merge window.

Great, thanks Herbert.

-- 
paul-moore.com

Re: [PATCH] KEYS: Invert FINAL_PUT bit

[Jarkko Sakkinen]

On Tue, Jun 10, 2025 at 08:22:59PM -0400, Paul Moore wrote:
> On Wed, May 28, 2025 at 8:19 AM David Howells <dhowells@redhat.com> wrote:
> >
> > Hi Linus,
> >
> > Could you apply this, please?  There shouldn't be any functional change,
> > rather it's a switch to using combined bit-barrier ops and lesser barriers.
> > A better way to do this might be to provide set_bit_release(), but the end
> > result would be much the same.
> >
> > Thanks,
> > David
> > ---
> > From: Herbert Xu <herbert@gondor.apana.org.au>
> >
> > KEYS: Invert FINAL_PUT bit
> >
> > 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>
> > Signed-off-by: David Howells <dhowells@redhat.com>
> > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> > cc: keyrings@vger.kernel.org
> > cc: linux-security-module@vger.kernel.org
> > cc: linux-crypto@vger.kernel.org
> > cc: linux-integrity@vger.kernel.org
> > ---
> >  include/linux/key.h |    2 +-
> >  security/keys/gc.c  |    4 ++--
> >  security/keys/key.c |    5 +++--
> >  3 files changed, 6 insertions(+), 5 deletions(-)
> 
> It doesn't look like this has made its way to Linus.  David or Jarkko,
> do one of you want to pick this up into a tree and send this to Linus
> properly?

I'm open for anything but need comment from David at first. It is up to
him as he carries the torch ATM for this one :-)

BR, Jarkko

Re: [PATCH] KEYS: Invert FINAL_PUT bit

[Linus Torvalds]

On Tue, 10 Jun 2025 at 17:23, Paul Moore <paul@paul-moore.com> wrote:
>
> It doesn't look like this has made its way to Linus.

Bah. It "made it" in the sense that sure, it's in my inbox.

But particularly during the the early merge window I end up heavily
limiting my emails to pull requests. And then it ended up composted at
the bottom of my endless pile of emails.

I guess I can still take it if people just say "do it".

            Linus

Re: [PATCH] KEYS: Invert FINAL_PUT bit

[David Howells]

Linus Torvalds <torvalds@linux-foundation.org> wrote:

> I guess I can still take it if people just say "do it".

Do you want a signed tag and git pull for it?

David

Re: [PATCH] KEYS: Invert FINAL_PUT bit

[Linus Torvalds]

On Wed, 11 Jun 2025 at 11:45, David Howells <dhowells@redhat.com> wrote:
>
> Do you want a signed tag and git pull for it?

Particularly during the merge window that makes sense just to make it
trigger my usual "git pull" pattern, but now that I'm more aware of it
I can just take the patch directly.

Anyway - done just to get this behind us. But for next time, just do
it as a signed tag pull request, _particularly_ during the merge
window when most other emails get much lower priority.

             Linus

Re: [PATCH] KEYS: Invert FINAL_PUT bit

[Al Viro]

On Wed, Jun 11, 2025 at 11:59:19AM -0700, Linus Torvalds wrote:
> On Wed, 11 Jun 2025 at 11:45, David Howells <dhowells@redhat.com> wrote:
> >
> > Do you want a signed tag and git pull for it?
> 
> Particularly during the merge window that makes sense just to make it
> trigger my usual "git pull" pattern, but now that I'm more aware of it
> I can just take the patch directly.
> 
> Anyway - done just to get this behind us. But for next time, just do
> it as a signed tag pull request, _particularly_ during the merge
> window when most other emails get much lower priority.

Speaking of the stuff fallen through the cracks - could you take another
look at https://lore.kernel.org/all/20250602041118.GA2675383@ZenIV/?

Re: [PATCH] KEYS: Invert FINAL_PUT bit

[Linus Torvalds]

On Wed, 11 Jun 2025 at 13:38, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Speaking of the stuff fallen through the cracks - could you take another
> look at https://lore.kernel.org/all/20250602041118.GA2675383@ZenIV/?

Also done.

Well, the script part is, it's still doing the test-build and I'll
have to make a commit message etc.

              Linus

Re: [PATCH] KEYS: Invert FINAL_PUT bit

[Jarkko Sakkinen]

On Wed, Jun 11, 2025 at 10:50:46AM -0700, Linus Torvalds wrote:
> On Tue, 10 Jun 2025 at 17:23, Paul Moore <paul@paul-moore.com> wrote:
> >
> > It doesn't look like this has made its way to Linus.
> 
> Bah. It "made it" in the sense that sure, it's in my inbox.
> 
> But particularly during the the early merge window I end up heavily
> limiting my emails to pull requests. And then it ended up composted at
> the bottom of my endless pile of emails.
> 
> I guess I can still take it if people just say "do it".
> 
>             Linus

+1 for picking it.

BR, Jarkko

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

[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()

[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