[v3] keys: update key quotas in key_put()
Commit Message
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(-)
Comments
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,
@@ -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);
@@ -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);
@@ -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;