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 <[email protected]>
---
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(-)
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;
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 <[email protected]>
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 <[email protected]>
BR, Jarkko
Luis Henriques <[email protected]> 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 <[email protected]>
Acked-by: David Howells <[email protected]>
Jarkko - could you pick this up?
David Howells <[email protected]> writes:
> Luis Henriques <[email protected]> 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,
--
Luís
>> 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 <[email protected]>
>
> Acked-by: David Howells <[email protected]>
>
> Jarkko - could you pick this up?
>
Luis Henriques <[email protected]> writes:
> David Howells <[email protected]> writes:
>
>> Luis Henriques <[email protected]> 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,
--
Luís
On Wed Mar 13, 2024 at 2:37 PM EET, Luis Henriques wrote:
> Luis Henriques <[email protected]> writes:
>
> > David Howells <[email protected]> writes:
> >
> >> Luis Henriques <[email protected]> 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 <[email protected]> writes:
> >
> > > David Howells <[email protected]> writes:
> > >
> > >> Luis Henriques <[email protected]> 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" <[email protected]> writes:
> On Wed Mar 13, 2024 at 2:37 PM EET, Luis Henriques wrote:
>> Luis Henriques <[email protected]> writes:
>>
>> > David Howells <[email protected]> writes:
>> >
>> >> Luis Henriques <[email protected]> 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,
--
Luis