| Message ID | 20240115120300.27606-1-lhenriques@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v2] keys: update key quotas in key_put() | expand |
On Mon Jan 15, 2024 at 12:03 PM UTC, 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 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ This commit fixes the 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. The commit message fails to describe the implementation changes. You have a motivation paragraph but you also need to add implementation paragraph, which describes how the code changes reflect the motivation. BR, Jarkko
"Jarkko Sakkinen" <jarkko@kernel.org> writes: > On Mon Jan 15, 2024 at 12:03 PM UTC, 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 > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > This commit fixes the 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. > > The commit message fails to describe the implementation changes. > > You have a motivation paragraph but you also need to add implementation > paragraph, which describes how the code changes reflect the motivation. Thank you for your feedback, Jarkko. I'll address your comments in v3. But before sending another rev, I'll wait a bit more, maybe David also has some feedback on the implementation. Cheers,
On Mon Jan 22, 2024 at 1:50 PM EET, Luis Henriques wrote: > "Jarkko Sakkinen" <jarkko@kernel.org> writes: > > > On Mon Jan 15, 2024 at 12:03 PM UTC, 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 > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > This commit fixes the 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. > > > > The commit message fails to describe the implementation changes. > > > > You have a motivation paragraph but you also need to add implementation > > paragraph, which describes how the code changes reflect the motivation. > > Thank you for your feedback, Jarkko. I'll address your comments in v3. > > But before sending another rev, I'll wait a bit more, maybe David also has > some feedback on the implementation. > > Cheers, Take you time :-) BR, Jarkko
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. - Eric
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; 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. Cheers,
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; } > 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. - Eric
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,
diff --git a/security/keys/gc.c b/security/keys/gc.c index eaddaceda14e..7d687b0962b1 100644 --- a/security/keys/gc.c +++ b/security/keys/gc.c @@ -155,14 +155,6 @@ static noinline void key_gc_unused_keys(struct list_head *keys) security_key_free(key); - /* deal with the user's key tracking and quota */ - if (test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) { - spin_lock(&key->user->lock); - key->user->qnkeys--; - key->user->qnbytes -= key->quotalen; - spin_unlock(&key->user->lock); - } - atomic_dec(&key->user->nkeys); if (state != KEY_IS_UNINSTANTIATED) atomic_dec(&key->user->nikeys); diff --git a/security/keys/key.c b/security/keys/key.c index 5b10641debd5..ec155cfaae38 100644 --- a/security/keys/key.c +++ b/security/keys/key.c @@ -231,6 +231,7 @@ struct key *key_alloc(struct key_type *type, const char *desc, struct key *key; size_t desclen, quotalen; int ret; + unsigned long irqflags; key = ERR_PTR(-EINVAL); if (!desc || !*desc) @@ -260,7 +261,7 @@ struct key *key_alloc(struct key_type *type, const char *desc, unsigned maxbytes = uid_eq(uid, GLOBAL_ROOT_UID) ? key_quota_root_maxbytes : key_quota_maxbytes; - spin_lock(&user->lock); + spin_lock_irqsave(&user->lock, irqflags); if (!(flags & KEY_ALLOC_QUOTA_OVERRUN)) { if (user->qnkeys + 1 > maxkeys || user->qnbytes + quotalen > maxbytes || @@ -270,7 +271,7 @@ struct key *key_alloc(struct key_type *type, const char *desc, user->qnkeys++; user->qnbytes += quotalen; - spin_unlock(&user->lock); + spin_unlock_irqrestore(&user->lock, irqflags); } /* allocate and initialise the key and its description */ @@ -328,10 +329,10 @@ struct key *key_alloc(struct key_type *type, const char *desc, kfree(key->description); kmem_cache_free(key_jar, key); if (!(flags & KEY_ALLOC_NOT_IN_QUOTA)) { - spin_lock(&user->lock); + spin_lock_irqsave(&user->lock, irqflags); user->qnkeys--; user->qnbytes -= quotalen; - spin_unlock(&user->lock); + spin_unlock_irqrestore(&user->lock, irqflags); } key_user_put(user); key = ERR_PTR(ret); @@ -341,10 +342,10 @@ struct key *key_alloc(struct key_type *type, const char *desc, kmem_cache_free(key_jar, key); no_memory_2: if (!(flags & KEY_ALLOC_NOT_IN_QUOTA)) { - spin_lock(&user->lock); + spin_lock_irqsave(&user->lock, irqflags); user->qnkeys--; user->qnbytes -= quotalen; - spin_unlock(&user->lock); + spin_unlock_irqrestore(&user->lock, irqflags); } key_user_put(user); no_memory_1: @@ -352,7 +353,7 @@ struct key *key_alloc(struct key_type *type, const char *desc, goto error; no_quota: - spin_unlock(&user->lock); + spin_unlock_irqrestore(&user->lock, irqflags); key_user_put(user); key = ERR_PTR(-EDQUOT); goto error; @@ -381,8 +382,9 @@ int key_payload_reserve(struct key *key, size_t datalen) if (delta != 0 && test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) { unsigned maxbytes = uid_eq(key->user->uid, GLOBAL_ROOT_UID) ? key_quota_root_maxbytes : key_quota_maxbytes; + unsigned long flags; - spin_lock(&key->user->lock); + spin_lock_irqsave(&key->user->lock, flags); if (delta > 0 && (key->user->qnbytes + delta > maxbytes || @@ -393,7 +395,7 @@ int key_payload_reserve(struct key *key, size_t datalen) key->user->qnbytes += delta; key->quotalen += delta; } - spin_unlock(&key->user->lock); + spin_unlock_irqrestore(&key->user->lock, flags); } /* change the recorded data length if that didn't generate an error */ @@ -646,8 +648,18 @@ void key_put(struct key *key) if (key) { key_check(key); - if (refcount_dec_and_test(&key->usage)) + if (refcount_dec_and_test(&key->usage)) { + unsigned long flags; + + /* deal with the user's key tracking and quota */ + if (test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) { + spin_lock_irqsave(&key->user->lock, flags); + key->user->qnkeys--; + key->user->qnbytes -= key->quotalen; + spin_unlock_irqrestore(&key->user->lock, flags); + } schedule_work(&key_gc_work); + } } } EXPORT_SYMBOL(key_put); diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c index 10ba439968f7..4bc3e9398ee3 100644 --- a/security/keys/keyctl.c +++ b/security/keys/keyctl.c @@ -954,6 +954,7 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group) long ret; kuid_t uid; kgid_t gid; + unsigned long flags; uid = make_kuid(current_user_ns(), user); gid = make_kgid(current_user_ns(), group); @@ -1010,7 +1011,7 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group) unsigned maxbytes = uid_eq(uid, GLOBAL_ROOT_UID) ? key_quota_root_maxbytes : key_quota_maxbytes; - spin_lock(&newowner->lock); + spin_lock_irqsave(&newowner->lock, flags); if (newowner->qnkeys + 1 > maxkeys || newowner->qnbytes + key->quotalen > maxbytes || newowner->qnbytes + key->quotalen < @@ -1019,12 +1020,12 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group) newowner->qnkeys++; newowner->qnbytes += key->quotalen; - spin_unlock(&newowner->lock); + spin_unlock_irqrestore(&newowner->lock, flags); - spin_lock(&key->user->lock); + spin_lock_irqsave(&key->user->lock, flags); key->user->qnkeys--; key->user->qnbytes -= key->quotalen; - spin_unlock(&key->user->lock); + spin_unlock_irqrestore(&key->user->lock, flags); } atomic_dec(&key->user->nkeys); @@ -1056,7 +1057,7 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group) return ret; quota_overrun: - spin_unlock(&newowner->lock); + spin_unlock_irqrestore(&newowner->lock, flags); zapowner = newowner; ret = -EDQUOT; goto error_put;
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(-)