Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Organization: Red Hat UK Ltd. Registered Address: Red Hat UK Ltd, Amberley
        Place, 107-111 Peascod Street, Windsor, Berkshire, SI4 1TE, United
        Kingdom.
        Registered in England and Wales under Company Registration No. 3798903
From:   David Howells <dhowells@redhat.com>
In-Reply-To: <20231207024323.GA1994@sol.localdomain>
References: <20231207024323.GA1994@sol.localdomain> <20231206145744.17277-1-lhenriques@suse.de> <498294.1701878642@warthog.procyon.org.uk> <87bkb3z047.fsf@suse.de>
To:     Eric Biggers <ebiggers@kernel.org>
Cc:     dhowells@redhat.com, Luis Henriques <lhenriques@suse.de>,
        Jarkko Sakkinen <jarkko@kernel.org>, keyrings@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH] keys: flush work when accessing /proc/key-users
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2744562.1702303367.1@warthog.procyon.org.uk>
Date:   Mon, 11 Dec 2023 14:02:47 +0000
Message-ID: <2744563.1702303367@warthog.procyon.org.uk>
Precedence: bulk

Eric Biggers <ebiggers@kernel.org> wrote:

> If there was a function that fully and synchronously releases a key's quota,
> fs/crypto/ could call it before unlinking the key.  key_payload_reserve(key,
> 0) almost does the trick, but it would release the key's bytes, not the key
> itself.

Umm...  The point of the quota is that the key is occupying unswappable kernel
memory (partly true in the case of big_key) and we need to limit that.
Further, the key is not released until it is unlinked.

> However, that would only fix the flakiness of the key quota for fs/crypto/,
> not for other users of the keyrings service.  Maybe this suggests that
> key_put() should release the key's quota right away if the key's refcount
> drops to 0?

That I would be okay with as the key should be removed in short order.

Note that you'd have to change the spinlocks on key->user->lock to irq-locking
types.  Or maybe we can do without them, at least for key gc, and use atomic
counters.  key_invalidate() should probably drop the quota also.

I'm also working up a patch so that key types can be marked for immediate gc
if they expire, rather than there being a period (key_gc_delay) in which they
cause EKEYEXPIRED rather than ENOKEY to be returned for better indication to
userspace as to what's happened when a filesystem op fails to to key problems.

> Either way, note that where fs/crypto/ does key_put() on a whole keyring at
> once, it would first need to call keyring_clear() to clear it synchronously.

What if there's another link on the keyring?  Should it still be cleared?

Do we need faster disposal of keys?  Perhaps keeping a list of keys that need
destroying rather than scanning the entire key set for them.  We still need to
scan non-destroyed keyrings, though, to find the pointers to defunct keys
unless I have some sort of backpointer list.

David