From: Luis Henriques <lhenriques@suse.de>
To: Eric Biggers <ebiggers@kernel.org>
Cc: David Howells <dhowells@redhat.com>,
Jarkko Sakkinen <jarkko@kernel.org>,
keyrings@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v2] keys: update key quotas in key_put()
Date: Mon, 29 Jan 2024 11:23:59 +0000 [thread overview]
Message-ID: <87bk94qt68.fsf@suse.de> (raw)
In-Reply-To: <20240127064220.GB11935@sol.localdomain> (Eric Biggers's message of "Fri, 26 Jan 2024 22:42:20 -0800")
Eric Biggers <ebiggers@kernel.org> writes:
> On Fri, Jan 26, 2024 at 04:12:59PM +0000, Luis Henriques wrote:
>> Eric Biggers <ebiggers@kernel.org> writes:
>>
>> > On Mon, Jan 15, 2024 at 12:03:00PM +0000, Luis Henriques wrote:
>> >> Delaying key quotas update when key's refcount reaches 0 in key_put() has
>> >> been causing some issues in fscrypt testing. This patches fixes this test
>> >> flakiness by dealing with the quotas immediately, but leaving all the other
>> >> clean-ups to the key garbage collector. Unfortunately, this means that we
>> >> also need to switch to the irq-version of the spinlock that protects quota.
>> >>
>> >> Signed-off-by: Luis Henriques <lhenriques@suse.de>
>> >> ---
>> >> Hi David!
>> >>
>> >> I have these changes in my local disk for a while; I wanted to send them
>> >> before EOY break but... yeah, it didn't happen. Anyway, I'm still sending
>> >> it as an RFC as I'm probably missing something.
>> >>
>> >> security/keys/gc.c | 8 --------
>> >> security/keys/key.c | 32 ++++++++++++++++++++++----------
>> >> security/keys/keyctl.c | 11 ++++++-----
>> >> 3 files changed, 28 insertions(+), 23 deletions(-)
>> >
>> > This patch seems reasonable to me, though I'm still thinking about changing
>> > fs/crypto/ to manage its key quotas itself which would avoid the issue entirely.
>> >
>> > Note that as I said before, fs/crypto/ does key_put() on a whole keyring at
>> > once, in order to release the quota of the keys in the keyring. Do you plan to
>> > also change fs/crypto/ to keyring_clear() the keyring before putting it?
>> > Without that, I don't think the problem is solved, as the quota release will
>> > still happen asynchronously due to the keyring being cleared asynchronously.
>>
>> Ah, good point. In the meantime I had forgotten everything about this
>> code and missed that. So, I can send another patch to fs/crypto to add
>> that extra call once (or if) this patch is accepted.
>>
>> If I'm reading the code correctly, the only place where this extra call is
>> required is on fscrypt_put_master_key():
>>
>> diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c
>> index 0edf0b58daa7..4afd32f1aed9 100644
>> --- a/fs/crypto/keyring.c
>> +++ b/fs/crypto/keyring.c
>> @@ -74,6 +74,7 @@ void fscrypt_put_master_key(struct fscrypt_master_key *mk)
>> * that concurrent keyring lookups can no longer find it.
>> */
>> WARN_ON_ONCE(refcount_read(&mk->mk_active_refs) != 0);
>> + keyring_clear(mk->mk_users);
>> key_put(mk->mk_users);
>> mk->mk_users = NULL;
>
> This will need a NULL check, since keyring_clear() doesn't accept NULL:
>
> if (mk->mk_users) {
> keyring_clear(mk->mk_users);
> key_put(mk->mk_users);
> mk->mk_users = NULL;
> }
>
Ah, good point. Makes sense.
>> call_rcu(&mk->mk_rcu_head, fscrypt_free_master_key);
>>
>> On the other hand, if you're really working towards dropping this code
>> entirely, maybe there's not point doing that.
>
> Well, it depends whether this patch (the one for security/keys/) is accepted or
> not. If it's accepted, I think it makes sense to add the keyring_clear() and
> otherwise keep the fs/crypto/ code as-is. If it's not accepted, I think I'll
> have to make fs/crypto/ handle the quotas itself.
Awesome, thank.
David, do you have any feedback on this patch or would you rather have me
send v3, addressing Jarkko comments regarding the commit message?
Cheers,
--
Luís
next prev parent reply other threads:[~2024-01-29 11:24 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-06 14:57 [RFC PATCH] keys: flush work when accessing /proc/key-users Luis Henriques
2023-12-06 16:04 ` David Howells
2023-12-06 17:55 ` Luis Henriques
2023-12-07 2:43 ` Eric Biggers
2023-12-11 14:02 ` David Howells
2023-12-12 3:03 ` Eric Biggers
2023-12-14 14:44 ` Luís Henriques
2024-01-15 12:03 ` [RFC PATCH v2] keys: update key quotas in key_put() Luis Henriques
2024-01-19 21:10 ` Jarkko Sakkinen
2024-01-22 11:50 ` Luis Henriques
2024-01-22 19:45 ` Jarkko Sakkinen
2024-01-24 22:12 ` Eric Biggers
2024-01-26 16:12 ` Luis Henriques
2024-01-27 6:42 ` Eric Biggers
2024-01-29 11:23 ` Luis Henriques [this message]
2023-12-07 4:33 ` [RFC PATCH] keys: flush work when accessing /proc/key-users Jarkko Sakkinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87bk94qt68.fsf@suse.de \
--to=lhenriques@suse.de \
--cc=dhowells@redhat.com \
--cc=ebiggers@kernel.org \
--cc=jarkko@kernel.org \
--cc=keyrings@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).