| Message ID | 20240130101344.28936-1-lhenriques@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] keys: update key quotas in key_put() | expand |
On Tue Jan 30, 2024 at 12:13 PM EET, Luis Henriques wrote: > Delaying key quotas update when key's refcount reaches 0 in key_put() has > been causing some issues in fscrypt testing, specifically in fstest > generic/581. This commit fixes this test flakiness by dealing with the > quotas immediately, and leaving all the other clean-ups to the key garbage > collector. > > This is done by moving the updates to the qnkeys and qnbytes fields in > struct key_user from key_gc_unused_keys() into key_put(). Unfortunately, > this also means that we need to switch to the irq-version of the spinlock > that protects these fields and use spin_lock_{irqsave,irqrestore} in all the > code that touches these fields. > > Signed-off-by: Luis Henriques <lhenriques@suse.de> OK this is great. I mean in this commit it is pretty essentiual to document that there is an ownership change. Such changes have by far the biggest impact to kernel semantics, and thus very useful to mark such commits for e.g. bisection. Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> BR, Jarkko
Luis Henriques <lhenriques@suse.de> wrote: > Delaying key quotas update when key's refcount reaches 0 in key_put() has > been causing some issues in fscrypt testing, specifically in fstest > generic/581. This commit fixes this test flakiness by dealing with the > quotas immediately, and leaving all the other clean-ups to the key garbage > collector. Okay, I'll accept this. > This is done by moving the updates to the qnkeys and qnbytes fields in > struct key_user from key_gc_unused_keys() into key_put(). Unfortunately, > this also means that we need to switch to the irq-version of the spinlock > that protects these fields and use spin_lock_{irqsave,irqrestore} in all the > code that touches these fields. ... Which shouldn't be that often. It only happens when a key is created or finally let go of. > Signed-off-by: Luis Henriques <lhenriques@suse.de> Acked-by: David Howells <dhowells@redhat.com> Jarkko - could you pick this up?
David Howells <dhowells@redhat.com> writes: > Luis Henriques <lhenriques@suse.de> wrote: > >> Delaying key quotas update when key's refcount reaches 0 in key_put() has >> been causing some issues in fscrypt testing, specifically in fstest >> generic/581. This commit fixes this test flakiness by dealing with the >> quotas immediately, and leaving all the other clean-ups to the key garbage >> collector. > > Okay, I'll accept this. > That's awesome, thanks a lot David. And, as Eric requested, I'll send out shortly a follow-up fscrypt-specific patch, which will make generic/581 fstest finally pass. Cheers,
Luis Henriques <lhenriques@suse.de> writes: > David Howells <dhowells@redhat.com> writes: > >> Luis Henriques <lhenriques@suse.de> wrote: >> >>> Delaying key quotas update when key's refcount reaches 0 in key_put() has >>> been causing some issues in fscrypt testing, specifically in fstest >>> generic/581. This commit fixes this test flakiness by dealing with the >>> quotas immediately, and leaving all the other clean-ups to the key garbage >>> collector. >> >> Okay, I'll accept this. >> > > That's awesome, thanks a lot David. And, as Eric requested, I'll send out > shortly a follow-up fscrypt-specific patch, which will make generic/581 > fstest finally pass. Ping. Looks like this fell through the cracks...? I took a quick look at some git trees ('jarkko' and 'dhowells') but couldn't see this patch anywhere. Cheers,
On Wed Mar 13, 2024 at 2:37 PM EET, Luis Henriques wrote: > Luis Henriques <lhenriques@suse.de> writes: > > > David Howells <dhowells@redhat.com> writes: > > > >> Luis Henriques <lhenriques@suse.de> wrote: > >> > >>> Delaying key quotas update when key's refcount reaches 0 in key_put() has > >>> been causing some issues in fscrypt testing, specifically in fstest > >>> generic/581. This commit fixes this test flakiness by dealing with the > >>> quotas immediately, and leaving all the other clean-ups to the key garbage > >>> collector. > >> > >> Okay, I'll accept this. > >> > > > > That's awesome, thanks a lot David. And, as Eric requested, I'll send out > > shortly a follow-up fscrypt-specific patch, which will make generic/581 > > fstest finally pass. > > Ping. Looks like this fell through the cracks...? > > I took a quick look at some git trees ('jarkko' and 'dhowells') but > couldn't see this patch anywhere. > > Cheers, My bad! I'll pick this up now. BR, Jarkko
On Mon Mar 18, 2024 at 11:14 PM EET, Jarkko Sakkinen wrote: > On Wed Mar 13, 2024 at 2:37 PM EET, Luis Henriques wrote: > > Luis Henriques <lhenriques@suse.de> writes: > > > > > David Howells <dhowells@redhat.com> writes: > > > > > >> Luis Henriques <lhenriques@suse.de> wrote: > > >> > > >>> Delaying key quotas update when key's refcount reaches 0 in key_put() has > > >>> been causing some issues in fscrypt testing, specifically in fstest > > >>> generic/581. This commit fixes this test flakiness by dealing with the > > >>> quotas immediately, and leaving all the other clean-ups to the key garbage > > >>> collector. > > >> > > >> Okay, I'll accept this. > > >> > > > > > > That's awesome, thanks a lot David. And, as Eric requested, I'll send out > > > shortly a follow-up fscrypt-specific patch, which will make generic/581 > > > fstest finally pass. > > > > Ping. Looks like this fell through the cracks...? > > > > I took a quick look at some git trees ('jarkko' and 'dhowells') but > > couldn't see this patch anywhere. > > > > Cheers, > > My bad! I'll pick this up now. I applied it. Only for completeness sake I mention that I tuned the 2nd paragraph so that lines do no exceed 75 characters :-) Otherwise, I did not modify the commit. I'll schedule this to a PR after rc1 is out (just to see if there is something else that needs to be picked before that). BR, Jarkko
"Jarkko Sakkinen" <jarkko@kernel.org> writes: > On Wed Mar 13, 2024 at 2:37 PM EET, Luis Henriques wrote: >> Luis Henriques <lhenriques@suse.de> writes: >> >> > David Howells <dhowells@redhat.com> writes: >> > >> >> Luis Henriques <lhenriques@suse.de> wrote: >> >> >> >>> Delaying key quotas update when key's refcount reaches 0 in key_put() has >> >>> been causing some issues in fscrypt testing, specifically in fstest >> >>> generic/581. This commit fixes this test flakiness by dealing with the >> >>> quotas immediately, and leaving all the other clean-ups to the key garbage >> >>> collector. >> >> >> >> Okay, I'll accept this. >> >> >> > >> > That's awesome, thanks a lot David. And, as Eric requested, I'll send out >> > shortly a follow-up fscrypt-specific patch, which will make generic/581 >> > fstest finally pass. >> >> Ping. Looks like this fell through the cracks...? >> >> I took a quick look at some git trees ('jarkko' and 'dhowells') but >> couldn't see this patch anywhere. >> >> Cheers, > > My bad! I'll pick this up now. Awesome, thanks a lot Jarkko. 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, specifically in fstest generic/581. This commit fixes this test flakiness by dealing with the quotas immediately, and leaving all the other clean-ups to the key garbage collector. This is done by moving the updates to the qnkeys and qnbytes fields in struct key_user from key_gc_unused_keys() into key_put(). Unfortunately, this also means that we need to switch to the irq-version of the spinlock that protects these fields and use spin_lock_{irqsave,irqrestore} in all the code that touches these fields. Signed-off-by: Luis Henriques <lhenriques@suse.de> --- Changes since v2: - Updated commit message as suggested by Jarkko, adding details on the implementation security/keys/gc.c | 8 -------- security/keys/key.c | 32 ++++++++++++++++++++++---------- security/keys/keyctl.c | 11 ++++++----- 3 files changed, 28 insertions(+), 23 deletions(-)