Message-ID: <Z-55p44u6rJRrY5A@kernel.org>
Date: Thu, 3 Apr 2025 15:05:59 +0300
From: Jarkko Sakkinen <jarkko@...nel.org>
To: David Howells <dhowells@...hat.com>
Cc: keyrings@...r.kernel.org, Jarkko Sakkinen <jarkko.sakkinen@...nsys.com>,
	Paul Moore <paul@...l-moore.com>, James Morris <jmorris@...ei.org>,
	"Serge E. Hallyn" <serge@...lyn.com>, linux-kernel@...r.kernel.org,
	linux-security-module@...r.kernel.org
Subject: Re: [RFC PATCH v3] KEYS: Add a list for unreferenced keys

On Wed, Apr 02, 2025 at 02:54:43PM +0100, David Howells wrote:
> Jarkko Sakkinen <jarkko@...nel.org> wrote:
> 
> > Add an isolated list for unreferenced keys, thereby splitting key
> > deletion as separate phase, after the reaping cycle. This makes the
> > whole process more safe, as these two distinct tasks don't intervene
> > each other. As an effect, KEY_FLAG_FINAL_PUT is no longer needed.
> >
> > Since `security/keys/proc.c` reserves key_serial_lock for variable
> > amount of time, `rb_erase()` cannot be done within `key_put()` by using
> > IRQ saving/restoring versions of the spin lock functions. For the same
> > reason, the key needs to co-exist both in the tree and in the list for
> > period of time.
> >
> > Therefore, split the union in `struct key` into separate `serial_node`
> > and `graveyard_link` fields, and introduce a separate lock for the
> > graveyard, which is locked and unlocked using IRQ saving/restoring
> > versions of the locking routines.

For this response I'll just discuss around the bullets brought up.
I need more time with seqlock solution.

What I did pick from your patch is that I'll revert the name of the
helper function back to key_gc_unused_keys() because I think it is
not senseful rename (only adds unneeded delta).

> Splitting the union seems reasonable.
> 
> However, you need to be very careful extracting keys from the serial tree
> outside of the gc loop:
> 
>  (1) The serial tree walking loop assumes that it can keep a pointer to the
>      key it is currently examining without holding any locks.  I think this is
>      probably not a problem for your patch as you're doing the removal in the
>      same work item.
> 
>  (2) We have to deal with put keys belonging to a key type that's undergoing
>      removal.  That might not be a problem as we can just get rid of them
>      directly - but we have to deal with the RCU implications and may need to
>      call synchronize_rcu() again after calling key_gc_graveyard().
> 
>  (3) We still have to deal with a new key_put() happening immediately after
>      you've done the rb_erase() calls.  I think it's probably worth still
>      skipping the evaluation of the key state if the refcount is 0 (if we're
>      getting rid of the FINAL_PUT flag).  We might be holding up key
>      insertion, after all.

In this v3, true, the processing is broken, as pages with usage at zero
go also go through "gc state machine" (or whatever you want to call the
key type teardown process).

How my solution could be improved to address these concerns for the most
part, would be to do this in the beginning of each iteration:

	if (!refcount_inc_not_zero(&key->usage))
		goto skip_dead_key;

And after processing: call key_put() (essentially open-coded
kref_get_unless_zero).

This would guarantee that the traversal keeps it hands off from pages
with usage reduced to zero, wouldn't it?

If this is added to the patch, I don't really see how this patch would
break the causality i.e., order of how things are executed in the
garbage collection flow.

BR, Jarkko
 

Powered by blists - more mailing lists