* [PATCH 0/7] keys: Add tracepoints
@ 2024-08-21 12:36 David Howells
2024-08-21 12:36 ` [PATCH 1/7] keys: Out of line key_is_dead() so it can have tracepoints added in David Howells
` (6 more replies)
0 siblings, 7 replies; 16+ messages in thread
From: David Howells @ 2024-08-21 12:36 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: David Howells, keyrings, linux-security-module, linux-kernel
Hi Jarkko,
Here's a patch to add some tracepoints to keyrings to make it easier to
debug what's going on, plus a bunch of preliminary patches to move things
about a bit to make that easier.
The patches can also be found here:
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-next
Thanks,
David
David Howells (7):
keys: Out of line key_is_dead() so it can have tracepoints added in
keys: Extract struct key_user to its own header for tracing purposes
keys: Move key_get() out of line so a tracepoint can be added
keys: Add a key_ref_get() wrapper
keys: Use key_get() instead of __key_get()
keys: Provide a key_try_get() function and use it
keys: Add tracepoints for the keyrings facility
Documentation/security/keys/core.rst | 1 -
crypto/asymmetric_keys/restrict.c | 6 +-
include/keys/key_user.h | 35 +++
include/linux/key.h | 16 +-
include/trace/events/key.h | 401 +++++++++++++++++++++++++++
security/keys/gc.c | 4 +
security/keys/internal.h | 41 +--
security/keys/key.c | 70 ++++-
security/keys/keyctl.c | 2 +
security/keys/keyring.c | 45 ++-
security/keys/process_keys.c | 15 +-
11 files changed, 566 insertions(+), 70 deletions(-)
create mode 100644 include/keys/key_user.h
create mode 100644 include/trace/events/key.h
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/7] keys: Out of line key_is_dead() so it can have tracepoints added in
2024-08-21 12:36 [PATCH 0/7] keys: Add tracepoints David Howells
@ 2024-08-21 12:36 ` David Howells
2024-08-27 18:22 ` Jarkko Sakkinen
2024-08-21 12:36 ` [PATCH 2/7] keys: Extract struct key_user to its own header for tracing purposes David Howells
` (5 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: David Howells @ 2024-08-21 12:36 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: David Howells, keyrings, linux-security-module, linux-kernel
Move key_is_dead() out of line so that tracepoints can be added in to it
without incurring circular #includes. Also, it is only used from the file
it is moved into.
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Jarkko Sakkinen <jarkko@kernel.org>
cc: keyrings@vger.kernel.org
cc: linux-security-module@vger.kernel.org
---
security/keys/internal.h | 20 --------------------
security/keys/keyring.c | 20 ++++++++++++++++++++
2 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 2cffa6dc8255..8ba87127e446 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -211,26 +211,6 @@ extern struct key *request_key_auth_new(struct key *target,
extern struct key *key_get_instantiation_authkey(key_serial_t target_id);
-/*
- * Determine whether a key is dead.
- */
-static inline bool key_is_dead(const struct key *key, time64_t limit)
-{
- time64_t expiry = key->expiry;
-
- if (expiry != TIME64_MAX) {
- if (!(key->type->flags & KEY_TYPE_INSTANT_REAP))
- expiry += key_gc_delay;
- if (expiry <= limit)
- return true;
- }
-
- return
- key->flags & ((1 << KEY_FLAG_DEAD) |
- (1 << KEY_FLAG_INVALIDATED)) ||
- key->domain_tag->removed;
-}
-
/*
* keyctl() functions
*/
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 4448758f643a..0eed018448cb 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1687,6 +1687,26 @@ static void keyring_revoke(struct key *keyring)
}
}
+/*
+ * Determine whether a key is dead.
+ */
+static bool key_is_dead(const struct key *key, time64_t limit)
+{
+ time64_t expiry = key->expiry;
+
+ if (expiry != TIME64_MAX) {
+ if (!(key->type->flags & KEY_TYPE_INSTANT_REAP))
+ expiry += key_gc_delay;
+ if (expiry <= limit)
+ return true;
+ }
+
+ return
+ key->flags & ((1 << KEY_FLAG_DEAD) |
+ (1 << KEY_FLAG_INVALIDATED)) ||
+ key->domain_tag->removed;
+}
+
static bool keyring_gc_select_iterator(void *object, void *iterator_data)
{
struct key *key = keyring_ptr_to_key(object);
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/7] keys: Extract struct key_user to its own header for tracing purposes
2024-08-21 12:36 [PATCH 0/7] keys: Add tracepoints David Howells
2024-08-21 12:36 ` [PATCH 1/7] keys: Out of line key_is_dead() so it can have tracepoints added in David Howells
@ 2024-08-21 12:36 ` David Howells
2024-08-27 18:23 ` Jarkko Sakkinen
2024-08-21 12:36 ` [PATCH 3/7] keys: Move key_get() out of line so a tracepoint can be added David Howells
` (4 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: David Howells @ 2024-08-21 12:36 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: David Howells, keyrings, linux-security-module, linux-kernel
Extract the key_user struct to its own header file to make it easier to
access from tracepoints.
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Jarkko Sakkinen <jarkko@kernel.org>
cc: keyrings@vger.kernel.org
cc: linux-security-module@vger.kernel.org
---
include/keys/key_user.h | 35 +++++++++++++++++++++++++++++++++++
security/keys/internal.h | 20 +-------------------
2 files changed, 36 insertions(+), 19 deletions(-)
create mode 100644 include/keys/key_user.h
diff --git a/include/keys/key_user.h b/include/keys/key_user.h
new file mode 100644
index 000000000000..e9c383d8116e
--- /dev/null
+++ b/include/keys/key_user.h
@@ -0,0 +1,35 @@
+/* User quota tracking for keys.
+ *
+ * Copyright (C) 2024 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public Licence
+ * as published by the Free Software Foundation; either version
+ * 2 of the Licence, or (at your option) any later version.
+ */
+
+#ifndef _KEYS_KEY_USER_H
+#define _KEYS_KEY_USER_H
+
+/*
+ * Keep track of keys for a user.
+ *
+ * This needs to be separate to user_struct to avoid a refcount-loop
+ * (user_struct pins some keyrings which pin this struct).
+ *
+ * We also keep track of keys under request from userspace for this UID here.
+ */
+struct key_user {
+ struct rb_node node;
+ struct mutex cons_lock; /* construction initiation lock */
+ spinlock_t lock;
+ refcount_t usage; /* for accessing qnkeys & qnbytes */
+ atomic_t nkeys; /* number of keys */
+ atomic_t nikeys; /* number of instantiated keys */
+ kuid_t uid;
+ int qnkeys; /* number of keys allocated to this user */
+ int qnbytes; /* number of bytes allocated to this user */
+};
+
+#endif /* _KEYS_KEY_USER_H */
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 8ba87127e446..33c929a6bb97 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -19,6 +19,7 @@
#include <linux/compat.h>
#include <linux/mm.h>
#include <linux/vmalloc.h>
+#include <keys/key_user.h>
struct iovec;
@@ -43,25 +44,6 @@ extern struct key_type key_type_user;
extern struct key_type key_type_logon;
/*****************************************************************************/
-/*
- * Keep track of keys for a user.
- *
- * This needs to be separate to user_struct to avoid a refcount-loop
- * (user_struct pins some keyrings which pin this struct).
- *
- * We also keep track of keys under request from userspace for this UID here.
- */
-struct key_user {
- struct rb_node node;
- struct mutex cons_lock; /* construction initiation lock */
- spinlock_t lock;
- refcount_t usage; /* for accessing qnkeys & qnbytes */
- atomic_t nkeys; /* number of keys */
- atomic_t nikeys; /* number of instantiated keys */
- kuid_t uid;
- int qnkeys; /* number of keys allocated to this user */
- int qnbytes; /* number of bytes allocated to this user */
-};
extern struct rb_root key_user_tree;
extern spinlock_t key_user_lock;
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/7] keys: Move key_get() out of line so a tracepoint can be added
2024-08-21 12:36 [PATCH 0/7] keys: Add tracepoints David Howells
2024-08-21 12:36 ` [PATCH 1/7] keys: Out of line key_is_dead() so it can have tracepoints added in David Howells
2024-08-21 12:36 ` [PATCH 2/7] keys: Extract struct key_user to its own header for tracing purposes David Howells
@ 2024-08-21 12:36 ` David Howells
2024-08-27 18:23 ` Jarkko Sakkinen
2024-08-21 12:36 ` [PATCH 4/7] keys: Add a key_ref_get() wrapper David Howells
` (3 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: David Howells @ 2024-08-21 12:36 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: David Howells, keyrings, linux-security-module, linux-kernel
Move key_get() out of line so that a tracepoint can be added into it
without incurring circular header dependencies or overly expanding the
callers.
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Jarkko Sakkinen <jarkko@kernel.org>
cc: keyrings@vger.kernel.org
cc: linux-security-module@vger.kernel.org
---
include/linux/key.h | 6 +-----
security/keys/key.c | 14 ++++++++++++++
2 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/include/linux/key.h b/include/linux/key.h
index 943a432da3ae..08062b4f807c 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -299,6 +299,7 @@ extern struct key *key_alloc(struct key_type *type,
extern void key_revoke(struct key *key);
extern void key_invalidate(struct key *key);
+struct key *key_get(struct key *key);
extern void key_put(struct key *key);
extern bool key_put_tag(struct key_tag *tag);
extern void key_remove_domain(struct key_tag *domain_tag);
@@ -309,11 +310,6 @@ static inline struct key *__key_get(struct key *key)
return key;
}
-static inline struct key *key_get(struct key *key)
-{
- return key ? __key_get(key) : key;
-}
-
static inline void key_ref_put(key_ref_t key_ref)
{
key_put(key_ref_to_ptr(key_ref));
diff --git a/security/keys/key.c b/security/keys/key.c
index 3d7d185019d3..14c7ee77ea15 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -635,6 +635,20 @@ int key_reject_and_link(struct key *key,
}
EXPORT_SYMBOL(key_reject_and_link);
+/**
+ * key_get - Get a reference on a key
+ * @key: The key to get a reference on.
+ *
+ * Get a reference on a key, if not NULL, and return the parameter.
+ */
+struct key *key_get(struct key *key)
+{
+ if (key)
+ refcount_inc(&key->usage);
+ return key;
+}
+EXPORT_SYMBOL(key_get);
+
/**
* key_put - Discard a reference to a key.
* @key: The key to discard a reference from.
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 4/7] keys: Add a key_ref_get() wrapper
2024-08-21 12:36 [PATCH 0/7] keys: Add tracepoints David Howells
` (2 preceding siblings ...)
2024-08-21 12:36 ` [PATCH 3/7] keys: Move key_get() out of line so a tracepoint can be added David Howells
@ 2024-08-21 12:36 ` David Howells
2024-08-27 18:23 ` Jarkko Sakkinen
2024-08-21 12:36 ` [PATCH 5/7] keys: Use key_get() instead of __key_get() David Howells
` (2 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: David Howells @ 2024-08-21 12:36 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: David Howells, keyrings, linux-security-module, linux-kernel
Add a key_ref_get() wrapper function to go with key_ref_put() and use it
for the place that needs it.
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Jarkko Sakkinen <jarkko@kernel.org>
cc: keyrings@vger.kernel.org
cc: linux-security-module@vger.kernel.org
---
include/linux/key.h | 5 +++++
security/keys/keyring.c | 2 +-
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/include/linux/key.h b/include/linux/key.h
index 08062b4f807c..50a19e5f9e45 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -315,6 +315,11 @@ static inline void key_ref_put(key_ref_t key_ref)
key_put(key_ref_to_ptr(key_ref));
}
+static inline void key_ref_get(key_ref_t key_ref)
+{
+ key_get(key_ref_to_ptr(key_ref));
+}
+
extern struct key *request_key_tag(struct key_type *type,
const char *description,
struct key_tag *domain_tag,
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 0eed018448cb..7f02b913c560 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -920,7 +920,7 @@ key_ref_t keyring_search_rcu(key_ref_t keyring_ref,
ctx->now = ktime_get_real_seconds();
if (search_nested_keyrings(keyring, ctx))
- __key_get(key_ref_to_ptr(ctx->result));
+ key_ref_get(ctx->result);
return ctx->result;
}
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 5/7] keys: Use key_get() instead of __key_get()
2024-08-21 12:36 [PATCH 0/7] keys: Add tracepoints David Howells
` (3 preceding siblings ...)
2024-08-21 12:36 ` [PATCH 4/7] keys: Add a key_ref_get() wrapper David Howells
@ 2024-08-21 12:36 ` David Howells
2024-08-27 18:24 ` Jarkko Sakkinen
2024-08-21 12:36 ` [PATCH 6/7] keys: Provide a key_try_get() function and use it David Howells
2024-08-21 12:36 ` [PATCH 7/7] keys: Add tracepoints for the keyrings facility David Howells
6 siblings, 1 reply; 16+ messages in thread
From: David Howells @ 2024-08-21 12:36 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: David Howells, keyrings, linux-security-module, linux-kernel
Switch users of __key_get() over to key_get() so that they benefit from the
future tracepointage thereof also and remove __key_get().
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Jarkko Sakkinen <jarkko@kernel.org>
cc: keyrings@vger.kernel.org
cc: linux-security-module@vger.kernel.org
---
Documentation/security/keys/core.rst | 1 -
crypto/asymmetric_keys/restrict.c | 6 +++---
include/linux/key.h | 6 ------
security/keys/keyring.c | 4 ++--
security/keys/process_keys.c | 15 ++++++---------
5 files changed, 11 insertions(+), 21 deletions(-)
diff --git a/Documentation/security/keys/core.rst b/Documentation/security/keys/core.rst
index 326b8a973828..0b179540d885 100644
--- a/Documentation/security/keys/core.rst
+++ b/Documentation/security/keys/core.rst
@@ -1217,7 +1217,6 @@ payload contents" for more information.
* Extra references can be made to a key by calling one of the following
functions::
- struct key *__key_get(struct key *key);
struct key *key_get(struct key *key);
Keys so references will need to be disposed of by calling key_put() when
diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
index afcd4d101ac5..1ea7bfd4e5d7 100644
--- a/crypto/asymmetric_keys/restrict.c
+++ b/crypto/asymmetric_keys/restrict.c
@@ -267,20 +267,20 @@ static int key_or_keyring_common(struct key *dest_keyring,
if (!sig->auth_ids[0] && !sig->auth_ids[1]) {
if (asymmetric_key_id_same(signer_ids[2],
sig->auth_ids[2]))
- key = __key_get(trusted);
+ key = key_get(trusted);
} else if (!sig->auth_ids[0] || !sig->auth_ids[1]) {
const struct asymmetric_key_id *auth_id;
auth_id = sig->auth_ids[0] ?: sig->auth_ids[1];
if (match_either_id(signer_ids, auth_id))
- key = __key_get(trusted);
+ key = key_get(trusted);
} else if (asymmetric_key_id_same(signer_ids[1],
sig->auth_ids[1]) &&
match_either_id(signer_ids,
sig->auth_ids[0])) {
- key = __key_get(trusted);
+ key = key_get(trusted);
}
} else {
return -EOPNOTSUPP;
diff --git a/include/linux/key.h b/include/linux/key.h
index 50a19e5f9e45..80d736813b89 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -304,12 +304,6 @@ extern void key_put(struct key *key);
extern bool key_put_tag(struct key_tag *tag);
extern void key_remove_domain(struct key_tag *domain_tag);
-static inline struct key *__key_get(struct key *key)
-{
- refcount_inc(&key->usage);
- return key;
-}
-
static inline void key_ref_put(key_ref_t key_ref)
{
key_put(key_ref_to_ptr(key_ref));
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 7f02b913c560..e77d927f1d4d 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1122,7 +1122,7 @@ key_ref_t find_key_to_update(key_ref_t keyring_ref,
kleave(" = NULL [x]");
return NULL;
}
- __key_get(key);
+ key_get(key);
kleave(" = {%d}", key->serial);
return make_key_ref(key, is_key_possessed(keyring_ref));
}
@@ -1367,7 +1367,7 @@ int __key_link_check_live_key(struct key *keyring, struct key *key)
void __key_link(struct key *keyring, struct key *key,
struct assoc_array_edit **_edit)
{
- __key_get(key);
+ key_get(key);
assoc_array_insert_set_object(*_edit, keyring_key_to_ptr(key));
assoc_array_apply_edit(*_edit);
*_edit = NULL;
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index b5d5333ab330..01291b2d0888 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -333,7 +333,7 @@ int install_session_keyring_to_cred(struct cred *cred, struct key *keyring)
if (IS_ERR(keyring))
return PTR_ERR(keyring);
} else {
- __key_get(keyring);
+ key_get(keyring);
}
/* install the keyring */
@@ -641,7 +641,7 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
}
key = ctx.cred->thread_keyring;
- __key_get(key);
+ key_get(key);
key_ref = make_key_ref(key, 1);
break;
@@ -658,8 +658,7 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
goto reget_creds;
}
- key = ctx.cred->process_keyring;
- __key_get(key);
+ key = key_get(ctx.cred->process_keyring);
key_ref = make_key_ref(key, 1);
break;
@@ -688,8 +687,7 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
goto reget_creds;
}
- key = ctx.cred->session_keyring;
- __key_get(key);
+ key = key_get(ctx.cred->session_keyring);
key_ref = make_key_ref(key, 1);
break;
@@ -717,7 +715,7 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
if (!key)
goto error;
- __key_get(key);
+ key_get(key);
key_ref = make_key_ref(key, 1);
break;
@@ -732,8 +730,7 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
key = NULL;
} else {
rka = ctx.cred->request_key_auth->payload.data[0];
- key = rka->dest_keyring;
- __key_get(key);
+ key = key_get(rka->dest_keyring);
}
up_read(&ctx.cred->request_key_auth->sem);
if (!key)
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 6/7] keys: Provide a key_try_get() function and use it
2024-08-21 12:36 [PATCH 0/7] keys: Add tracepoints David Howells
` (4 preceding siblings ...)
2024-08-21 12:36 ` [PATCH 5/7] keys: Use key_get() instead of __key_get() David Howells
@ 2024-08-21 12:36 ` David Howells
2024-08-27 18:24 ` Jarkko Sakkinen
2024-08-21 12:36 ` [PATCH 7/7] keys: Add tracepoints for the keyrings facility David Howells
6 siblings, 1 reply; 16+ messages in thread
From: David Howells @ 2024-08-21 12:36 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: David Howells, keyrings, linux-security-module, linux-kernel
Add a key_try_get() function to try to get a ref on a key and switch code
that's manipulating the key refcount directly to use it. This will allow a
tracepoint to be emplaced there later.
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Jarkko Sakkinen <jarkko@kernel.org>
cc: keyrings@vger.kernel.org
cc: linux-security-module@vger.kernel.org
---
include/linux/key.h | 1 +
security/keys/key.c | 16 +++++++++++++++-
security/keys/keyring.c | 2 +-
3 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/include/linux/key.h b/include/linux/key.h
index 80d736813b89..4e5baf3e7286 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -300,6 +300,7 @@ extern struct key *key_alloc(struct key_type *type,
extern void key_revoke(struct key *key);
extern void key_invalidate(struct key *key);
struct key *key_get(struct key *key);
+struct key *key_try_get(struct key *key);
extern void key_put(struct key *key);
extern bool key_put_tag(struct key_tag *tag);
extern void key_remove_domain(struct key_tag *domain_tag);
diff --git a/security/keys/key.c b/security/keys/key.c
index 14c7ee77ea15..59cffb6f9b94 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -649,6 +649,20 @@ struct key *key_get(struct key *key)
}
EXPORT_SYMBOL(key_get);
+/**
+ * key_try_get - Get a ref on a key if its refcount is not non-zero.
+ * @key: The key to get a reference on.
+ *
+ * Get a reference on a key unless it has no references and return true if
+ * successful. @key must not be NULL.
+ */
+struct key *key_try_get(struct key *key)
+{
+ if (!refcount_inc_not_zero(&key->usage))
+ return NULL;
+ return key;
+}
+
/**
* key_put - Discard a reference to a key.
* @key: The key to discard a reference from.
@@ -709,7 +723,7 @@ struct key *key_lookup(key_serial_t id)
/* A key is allowed to be looked up only if someone still owns a
* reference to it - otherwise it's awaiting the gc.
*/
- if (!refcount_inc_not_zero(&key->usage))
+ if (!key_try_get(key))
goto not_found;
error:
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index e77d927f1d4d..a09a4c2b1bcb 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1174,7 +1174,7 @@ struct key *find_keyring_by_name(const char *name, bool uid_keyring)
/* we've got a match but we might end up racing with
* key_cleanup() if the keyring is currently 'dead'
* (ie. it has a zero usage count) */
- if (!refcount_inc_not_zero(&keyring->usage))
+ if (!key_try_get(keyring))
continue;
keyring->last_used_at = ktime_get_real_seconds();
goto out;
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 7/7] keys: Add tracepoints for the keyrings facility
2024-08-21 12:36 [PATCH 0/7] keys: Add tracepoints David Howells
` (5 preceding siblings ...)
2024-08-21 12:36 ` [PATCH 6/7] keys: Provide a key_try_get() function and use it David Howells
@ 2024-08-21 12:36 ` David Howells
2024-08-27 18:27 ` Jarkko Sakkinen
2024-09-28 2:03 ` Justin Stitt
6 siblings, 2 replies; 16+ messages in thread
From: David Howells @ 2024-08-21 12:36 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: David Howells, keyrings, linux-security-module, linux-kernel
Add some tracepoints to aid in debuggin the keyrings facility and
applications that use it. A number of events and operations are traceable,
including:
- Allocation
- Refcounting
- Instantiation and negative instantiation/rejection
- Update
- Detection of key being dead
- Key quota changes
- Key quota failure
- Link, unlink and move
- Keyring clearance
- Revocation and invalidation
- Garbage collection
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Jarkko Sakkinen <jarkko@kernel.org>
cc: keyrings@vger.kernel.org
cc: linux-security-module@vger.kernel.org
---
include/trace/events/key.h | 401 +++++++++++++++++++++++++++++++++++++
security/keys/gc.c | 4 +
security/keys/internal.h | 1 +
security/keys/key.c | 50 ++++-
security/keys/keyctl.c | 2 +
security/keys/keyring.c | 27 ++-
6 files changed, 472 insertions(+), 13 deletions(-)
create mode 100644 include/trace/events/key.h
diff --git a/include/trace/events/key.h b/include/trace/events/key.h
new file mode 100644
index 000000000000..b3f8c39cc0e8
--- /dev/null
+++ b/include/trace/events/key.h
@@ -0,0 +1,401 @@
+/* Keyrings tracepoints
+ *
+ * Copyright (C) 2024 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public Licence
+ * as published by the Free Software Foundation; either version
+ * 2 of the Licence, or (at your option) any later version.
+ */
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM key
+
+#if !defined(_TRACE_KEY_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_KEY_H
+
+#include <linux/tracepoint.h>
+
+struct key;
+
+/*
+ * Declare tracing information enums and their string mappings for display.
+ */
+#define key_ref_traces \
+ EM(key_trace_ref_alloc, "ALLOC") \
+ EM(key_trace_ref_free, "FREE ") \
+ EM(key_trace_ref_gc, "GC ") \
+ EM(key_trace_ref_get, "GET ") \
+ EM(key_trace_ref_put, "PUT ") \
+ E_(key_trace_ref_try_get, "GET ")
+
+#define key_dead_traces \
+ EM(key_trace_dead_type_removed, "TYPR") \
+ EM(key_trace_dead_domain_removed, "DOMR") \
+ EM(key_trace_dead_expired, "EXPD") \
+ E_(key_trace_dead_invalidated, "INVL")
+
+/*
+ * Generate enums for tracing information.
+ */
+#ifndef __NETFS_DECLARE_TRACE_ENUMS_ONCE_ONLY
+#define __NETFS_DECLARE_TRACE_ENUMS_ONCE_ONLY
+
+#undef EM
+#undef E_
+#define EM(a, b) a,
+#define E_(a, b) a
+
+enum key_dead_trace { key_dead_traces } __mode(byte);
+enum key_ref_trace { key_ref_traces } __mode(byte);
+
+#endif /* end __RXRPC_DECLARE_TRACE_ENUMS_ONCE_ONLY */
+
+/*
+ * Export enum symbols via userspace.
+ */
+#undef EM
+#undef E_
+#define EM(a, b) TRACE_DEFINE_ENUM(a);
+#define E_(a, b) TRACE_DEFINE_ENUM(a);
+
+key_dead_traces;
+key_ref_traces;
+
+/*
+ * Now redefine the EM() and E_() macros to map the enums to the strings that
+ * will be printed in the output.
+ */
+#undef EM
+#undef E_
+#define EM(a, b) { a, b },
+#define E_(a, b) { a, b }
+
+TRACE_EVENT(key_alloc,
+ TP_PROTO(const struct key *key),
+
+ TP_ARGS(key),
+
+ TP_STRUCT__entry(
+ __field(key_serial_t, key)
+ __field(uid_t, uid)
+ __array(char, type, 8)
+ __array(char, desc, 24)
+ ),
+
+ TP_fast_assign(
+ __entry->key = key->serial;
+ __entry->uid = from_kuid(&init_user_ns, key->uid);
+ strncpy(__entry->type, key->type->name, sizeof(__entry->type) - 1);
+ strncpy(__entry->desc, key->description ?: "", sizeof(__entry->desc) - 1);
+ __entry->type[sizeof(__entry->type) - 1] = 0;
+ __entry->desc[sizeof(__entry->desc) - 1] = 0;
+ ),
+
+ TP_printk("key=%08x uid=%08x t=%s d=%s",
+ __entry->key,
+ __entry->uid,
+ __entry->type,
+ __entry->desc)
+ );
+
+TRACE_EVENT(key_ref,
+ TP_PROTO(key_serial_t key, unsigned int ref, enum key_ref_trace trace,
+ const void *where),
+
+ TP_ARGS(key, ref, trace, where),
+
+ TP_STRUCT__entry(
+ __field(key_serial_t, key)
+ __field(enum key_ref_trace, trace)
+ __field(unsigned int, ref)
+ __field(const void *, where)
+ ),
+
+ TP_fast_assign(
+ __entry->key = key;
+ __entry->trace = trace;
+ __entry->ref = ref;
+ __entry->where = where;
+ ),
+
+ TP_printk("key=%08x %s r=%d pc=%pSR",
+ __entry->key,
+ __print_symbolic(__entry->trace, key_ref_traces),
+ __entry->ref,
+ __entry->where)
+ );
+
+TRACE_EVENT(key_instantiate,
+ TP_PROTO(const struct key *key, const struct key_preparsed_payload *prep),
+
+ TP_ARGS(key, prep),
+
+ TP_STRUCT__entry(
+ __field(key_serial_t, key)
+ __field(unsigned int, datalen)
+ __field(unsigned int, quotalen)
+ ),
+
+ TP_fast_assign(
+ __entry->key = key->serial;
+ __entry->datalen = prep->datalen;
+ __entry->quotalen = prep->quotalen;
+ ),
+
+ TP_printk("key=%08x dlen=%u qlen=%u",
+ __entry->key,
+ __entry->datalen,
+ __entry->quotalen)
+ );
+
+TRACE_EVENT(key_reject,
+ TP_PROTO(const struct key *key, int error),
+
+ TP_ARGS(key, error),
+
+ TP_STRUCT__entry(
+ __field(key_serial_t, key)
+ __field(int, error)
+ ),
+
+ TP_fast_assign(
+ __entry->key = key->serial;
+ __entry->error = error;
+ ),
+
+ TP_printk("key=%08x err=%d",
+ __entry->key,
+ __entry->error)
+ );
+
+TRACE_EVENT(key_update,
+ TP_PROTO(const struct key *key, const struct key_preparsed_payload *prep),
+
+ TP_ARGS(key, prep),
+
+ TP_STRUCT__entry(
+ __field(key_serial_t, key)
+ __field(unsigned int, datalen)
+ __field(unsigned int, quotalen)
+ ),
+
+ TP_fast_assign(
+ __entry->key = key->serial;
+ __entry->datalen = prep->datalen;
+ __entry->quotalen = prep->quotalen;
+ ),
+
+ TP_printk("key=%08x dlen=%u qlen=%u",
+ __entry->key,
+ __entry->datalen,
+ __entry->quotalen)
+ );
+
+TRACE_EVENT(key_dead,
+ TP_PROTO(const struct key *key, enum key_dead_trace trace),
+
+ TP_ARGS(key, trace),
+
+ TP_STRUCT__entry(
+ __field(key_serial_t, key)
+ __field(enum key_dead_trace, trace)
+ ),
+
+ TP_fast_assign(
+ __entry->key = key->serial;
+ __entry->trace = trace;
+ ),
+
+ TP_printk("key=%08x %s",
+ __entry->key,
+ __print_symbolic(__entry->trace, key_dead_traces))
+ );
+
+TRACE_EVENT(key_quota,
+ TP_PROTO(const struct key_user *user, int change_keys, int change_bytes),
+
+ TP_ARGS(user, change_keys, change_bytes),
+
+ TP_STRUCT__entry(
+ __field(uid_t, uid)
+ __field(unsigned int, nkeys)
+ __field(unsigned int, nikeys)
+ __field(unsigned int, qnkeys)
+ __field(unsigned int, qnbytes)
+ __field(int, change_keys)
+ __field(int, change_bytes)
+ ),
+
+ TP_fast_assign(
+ __entry->uid = from_kuid(&init_user_ns, user->uid);
+ __entry->nkeys = atomic_read(&user->nkeys);
+ __entry->nikeys = atomic_read(&user->nikeys);
+ __entry->qnkeys = user->qnkeys;
+ __entry->qnbytes = user->qnbytes;
+ __entry->change_keys = change_keys;
+ __entry->change_bytes = change_bytes;
+ ),
+
+ TP_printk("uid=%d nkeys=%u/%u qkeys=%u qpay=%u ckeys=%d cpay=%d",
+ __entry->uid,
+ __entry->nikeys, __entry->nkeys,
+ __entry->qnkeys,
+ __entry->qnbytes,
+ __entry->change_keys, __entry->change_bytes)
+ );
+
+TRACE_EVENT(key_edquot,
+ TP_PROTO(const struct key_user *user, unsigned int maxkeys,
+ unsigned int maxbytes, unsigned int reqbytes),
+
+ TP_ARGS(user, maxkeys, maxbytes, reqbytes),
+
+ TP_STRUCT__entry(
+ __field(uid_t, uid)
+ __field(unsigned int, nkeys)
+ __field(unsigned int, nikeys)
+ __field(unsigned int, qnkeys)
+ __field(unsigned int, qnbytes)
+ __field(unsigned int, maxkeys)
+ __field(unsigned int, maxbytes)
+ __field(unsigned int, reqbytes)
+ ),
+
+ TP_fast_assign(
+ __entry->uid = from_kuid(&init_user_ns, user->uid);
+ __entry->nkeys = atomic_read(&user->nkeys);
+ __entry->nikeys = atomic_read(&user->nikeys);
+ __entry->qnkeys = user->qnkeys;
+ __entry->qnbytes = user->qnbytes;
+ __entry->maxkeys = maxkeys;
+ __entry->maxbytes = maxbytes;
+ __entry->reqbytes = reqbytes;
+ ),
+
+ TP_printk("u=%d nkeys=%u/%u qkeys=%u/%u qpay=%u/%u cpay=%u",
+ __entry->uid,
+ __entry->nikeys, __entry->nkeys,
+ __entry->qnkeys, __entry->maxkeys,
+ __entry->qnbytes, __entry->maxbytes,
+ __entry->reqbytes)
+ );
+
+TRACE_EVENT(key_link,
+ TP_PROTO(const struct key *keyring, const struct key *key),
+
+ TP_ARGS(keyring, key),
+
+ TP_STRUCT__entry(
+ __field(key_serial_t, keyring)
+ __field(key_serial_t, key)
+ ),
+
+ TP_fast_assign(
+ __entry->keyring = keyring->serial;
+ __entry->key = key->serial;
+ ),
+
+ TP_printk("key=%08x to=%08x",
+ __entry->key, __entry->keyring)
+ );
+
+TRACE_EVENT(key_unlink,
+ TP_PROTO(const struct key *keyring, const struct key *key),
+
+ TP_ARGS(keyring, key),
+
+ TP_STRUCT__entry(
+ __field(key_serial_t, keyring)
+ __field(key_serial_t, key)
+ ),
+
+ TP_fast_assign(
+ __entry->keyring = keyring->serial;
+ __entry->key = key->serial;
+ ),
+
+ TP_printk("key=%08x from=%08x",
+ __entry->key, __entry->keyring)
+ );
+
+TRACE_EVENT(key_move,
+ TP_PROTO(const struct key *from, const struct key *to,
+ const struct key *key),
+
+ TP_ARGS(from, to, key),
+
+ TP_STRUCT__entry(
+ __field(key_serial_t, from)
+ __field(key_serial_t, to)
+ __field(key_serial_t, key)
+ ),
+
+ TP_fast_assign(
+ __entry->from = from->serial;
+ __entry->to = to->serial;
+ __entry->key = key->serial;
+ ),
+
+ TP_printk("key=%08x from=%08x to=%08x",
+ __entry->key, __entry->from, __entry->to)
+ );
+
+TRACE_EVENT(key_clear,
+ TP_PROTO(const struct key *keyring),
+ TP_ARGS(keyring),
+ TP_STRUCT__entry(
+ __field(key_serial_t, keyring)
+ ),
+ TP_fast_assign(
+ __entry->keyring = keyring->serial;
+ ),
+ TP_printk("key=%08x",
+ __entry->keyring)
+ );
+
+TRACE_EVENT(key_revoke,
+ TP_PROTO(const struct key *key),
+ TP_ARGS(key),
+ TP_STRUCT__entry(
+ __field(key_serial_t, key)
+ ),
+ TP_fast_assign(
+ __entry->key = key->serial;
+ ),
+ TP_printk("key=%08x",
+ __entry->key)
+ );
+
+TRACE_EVENT(key_invalidate,
+ TP_PROTO(const struct key *key),
+ TP_ARGS(key),
+ TP_STRUCT__entry(
+ __field(key_serial_t, key)
+ ),
+ TP_fast_assign(
+ __entry->key = key->serial;
+ ),
+ TP_printk("key=%08x",
+ __entry->key)
+ );
+
+TRACE_EVENT(key_gc,
+ TP_PROTO(const struct key *key),
+ TP_ARGS(key),
+ TP_STRUCT__entry(
+ __field(key_serial_t, key)
+ ),
+ TP_fast_assign(
+ __entry->key = key->serial;
+ ),
+ TP_printk("key=%08x",
+ __entry->key)
+ );
+
+#undef EM
+#undef E_
+#endif /* _TRACE_KEY_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
diff --git a/security/keys/gc.c b/security/keys/gc.c
index 7d687b0962b1..47f857653854 100644
--- a/security/keys/gc.c
+++ b/security/keys/gc.c
@@ -141,6 +141,7 @@ static noinline void key_gc_unused_keys(struct list_head *keys)
list_del(&key->graveyard_link);
+ trace_key_gc(key);
kdebug("- %u", key->serial);
key_check(key);
@@ -163,6 +164,8 @@ static noinline void key_gc_unused_keys(struct list_head *keys)
key_put_tag(key->domain_tag);
kfree(key->description);
+ trace_key_ref(key->serial, refcount_read(&key->usage),
+ key_trace_ref_free, 0);
memzero_explicit(key, sizeof(*key));
kmem_cache_free(key_jar, key);
}
@@ -331,6 +334,7 @@ static void key_garbage_collector(struct work_struct *work)
*/
found_unreferenced_key:
kdebug("unrefd key %d", key->serial);
+ trace_key_ref(key->serial, refcount_read(&key->usage), key_trace_ref_gc, 0);
rb_erase(&key->serial_node, &key_serial_tree);
spin_unlock(&key_serial_lock);
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 33c929a6bb97..87600683a7f5 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -20,6 +20,7 @@
#include <linux/mm.h>
#include <linux/vmalloc.h>
#include <keys/key_user.h>
+#include <trace/events/key.h>
struct iovec;
diff --git a/security/keys/key.c b/security/keys/key.c
index 59cffb6f9b94..abb8d60a0dc2 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -14,6 +14,7 @@
#include <linux/workqueue.h>
#include <linux/random.h>
#include <linux/err.h>
+#define CREATE_TRACE_POINTS
#include "internal.h"
struct kmem_cache *key_jar;
@@ -264,12 +265,15 @@ struct key *key_alloc(struct key_type *type, const char *desc,
if (!(flags & KEY_ALLOC_QUOTA_OVERRUN)) {
if (user->qnkeys + 1 > maxkeys ||
user->qnbytes + quotalen > maxbytes ||
- user->qnbytes + quotalen < user->qnbytes)
+ user->qnbytes + quotalen < user->qnbytes) {
+ trace_key_edquot(user, maxkeys, maxbytes, quotalen);
goto no_quota;
+ }
}
user->qnkeys++;
user->qnbytes += quotalen;
+ trace_key_quota(user, +1, quotalen);
spin_unlock_irqrestore(&user->lock, irqflags);
}
@@ -319,7 +323,10 @@ struct key *key_alloc(struct key_type *type, const char *desc,
/* publish the key by giving it a serial number */
refcount_inc(&key->domain_tag->usage);
atomic_inc(&user->nkeys);
+ trace_key_quota(user, 0, 0);
key_alloc_serial(key);
+ trace_key_alloc(key);
+ trace_key_ref(key->serial, 1, key_trace_ref_alloc, __builtin_return_address(0));
error:
return key;
@@ -331,6 +338,7 @@ struct key *key_alloc(struct key_type *type, const char *desc,
spin_lock_irqsave(&user->lock, irqflags);
user->qnkeys--;
user->qnbytes -= quotalen;
+ trace_key_quota(user, -1, -quotalen);
spin_unlock_irqrestore(&user->lock, irqflags);
}
key_user_put(user);
@@ -344,6 +352,7 @@ struct key *key_alloc(struct key_type *type, const char *desc,
spin_lock_irqsave(&user->lock, irqflags);
user->qnkeys--;
user->qnbytes -= quotalen;
+ trace_key_quota(user, -1, quotalen);
spin_unlock_irqrestore(&user->lock, irqflags);
}
key_user_put(user);
@@ -388,11 +397,13 @@ int key_payload_reserve(struct key *key, size_t datalen)
if (delta > 0 &&
(key->user->qnbytes + delta > maxbytes ||
key->user->qnbytes + delta < key->user->qnbytes)) {
+ trace_key_edquot(key->user, 0, maxbytes, datalen);
ret = -EDQUOT;
}
else {
key->user->qnbytes += delta;
key->quotalen += delta;
+ trace_key_quota(key->user, 0, delta);
}
spin_unlock_irqrestore(&key->user->lock, flags);
}
@@ -447,6 +458,7 @@ static int __key_instantiate_and_link(struct key *key,
if (ret == 0) {
/* mark the key as being instantiated */
atomic_inc(&key->user->nikeys);
+ trace_key_instantiate(key, prep);
mark_key_instantiated(key, 0);
notify_key(key, NOTIFY_KEY_INSTANTIATED, 0);
@@ -604,6 +616,7 @@ int key_reject_and_link(struct key *key,
if (key->state == KEY_IS_UNINSTANTIATED) {
/* mark the key as being negatively instantiated */
atomic_inc(&key->user->nikeys);
+ trace_key_reject(key, -error);
mark_key_instantiated(key, -error);
notify_key(key, NOTIFY_KEY_INSTANTIATED, -error);
key_set_expiry(key, ktime_get_real_seconds() + timeout);
@@ -641,10 +654,15 @@ EXPORT_SYMBOL(key_reject_and_link);
*
* Get a reference on a key, if not NULL, and return the parameter.
*/
-struct key *key_get(struct key *key)
+noinline struct key *key_get(struct key *key)
{
- if (key)
- refcount_inc(&key->usage);
+ if (key) {
+ int r;
+
+ __refcount_inc(&key->usage, &r);
+ trace_key_ref(key->serial, r + 1, key_trace_ref_get,
+ __builtin_return_address(0));
+ }
return key;
}
EXPORT_SYMBOL(key_get);
@@ -656,10 +674,14 @@ EXPORT_SYMBOL(key_get);
* Get a reference on a key unless it has no references and return true if
* successful. @key must not be NULL.
*/
-struct key *key_try_get(struct key *key)
+noinline struct key *key_try_get(struct key *key)
{
- if (!refcount_inc_not_zero(&key->usage))
+ int r;
+
+ if (!__refcount_inc_not_zero(&key->usage, &r))
return NULL;
+ trace_key_ref(key->serial, r + 1, key_trace_ref_try_get,
+ __builtin_return_address(0));
return key;
}
@@ -671,12 +693,19 @@ struct key *key_try_get(struct key *key)
* schedule the cleanup task to come and pull it out of the tree in process
* context at some later time.
*/
-void key_put(struct key *key)
+noinline void key_put(struct key *key)
{
if (key) {
+ key_serial_t serial = key->serial;
+ bool zero;
+ int r;
+
key_check(key);
- if (refcount_dec_and_test(&key->usage)) {
+ zero = __refcount_dec_and_test(&key->usage, &r);
+ trace_key_ref(serial, r - 1, key_trace_ref_put,
+ __builtin_return_address(0));
+ if (zero) {
unsigned long flags;
/* deal with the user's key tracking and quota */
@@ -684,6 +713,7 @@ void key_put(struct key *key)
spin_lock_irqsave(&key->user->lock, flags);
key->user->qnkeys--;
key->user->qnbytes -= key->quotalen;
+ trace_key_quota(key->user, -1, -key->quotalen);
spin_unlock_irqrestore(&key->user->lock, flags);
}
schedule_work(&key_gc_work);
@@ -807,6 +837,7 @@ static inline key_ref_t __key_update(key_ref_t key_ref,
ret = key->type->update(key, prep);
if (ret == 0) {
/* Updating a negative key positively instantiates it */
+ trace_key_update(key, prep);
mark_key_instantiated(key, 0);
notify_key(key, NOTIFY_KEY_UPDATED, 0);
}
@@ -1131,6 +1162,7 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen)
ret = key->type->update(key, &prep);
if (ret == 0) {
/* Updating a negative key positively instantiates it */
+ trace_key_update(key, &prep);
mark_key_instantiated(key, 0);
notify_key(key, NOTIFY_KEY_UPDATED, 0);
}
@@ -1166,6 +1198,7 @@ void key_revoke(struct key *key)
*/
down_write_nested(&key->sem, 1);
if (!test_and_set_bit(KEY_FLAG_REVOKED, &key->flags)) {
+ trace_key_revoke(key);
notify_key(key, NOTIFY_KEY_REVOKED, 0);
if (key->type->revoke)
key->type->revoke(key);
@@ -1198,6 +1231,7 @@ void key_invalidate(struct key *key)
if (!test_bit(KEY_FLAG_INVALIDATED, &key->flags)) {
down_write_nested(&key->sem, 1);
if (!test_and_set_bit(KEY_FLAG_INVALIDATED, &key->flags)) {
+ trace_key_invalidate(key);
notify_key(key, NOTIFY_KEY_INVALIDATED, 0);
key_schedule_gc_links();
}
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index ab927a142f51..f2ef898209e6 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -1020,11 +1020,13 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
newowner->qnkeys++;
newowner->qnbytes += key->quotalen;
+ trace_key_quota(newowner, +1, key->quotalen);
spin_unlock_irqrestore(&newowner->lock, flags);
spin_lock_irqsave(&key->user->lock, flags);
key->user->qnkeys--;
key->user->qnbytes -= key->quotalen;
+ trace_key_quota(newowner, -1, -key->quotalen);
spin_unlock_irqrestore(&key->user->lock, flags);
}
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index a09a4c2b1bcb..d67a60142318 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1368,6 +1368,7 @@ void __key_link(struct key *keyring, struct key *key,
struct assoc_array_edit **_edit)
{
key_get(key);
+ trace_key_link(keyring, key);
assoc_array_insert_set_object(*_edit, keyring_key_to_ptr(key));
assoc_array_apply_edit(*_edit);
*_edit = NULL;
@@ -1506,6 +1507,7 @@ static int __key_unlink_begin(struct key *keyring, struct key *key,
static void __key_unlink(struct key *keyring, struct key *key,
struct assoc_array_edit **_edit)
{
+ trace_key_unlink(keyring, key);
assoc_array_apply_edit(*_edit);
notify_key(keyring, NOTIFY_KEY_UNLINKED, key_serial(key));
*_edit = NULL;
@@ -1625,6 +1627,7 @@ int key_move(struct key *key,
if (ret < 0)
goto error;
+ trace_key_move(from_keyring, to_keyring, key);
__key_unlink(from_keyring, key, &from_edit);
__key_link(to_keyring, key, &to_edit);
error:
@@ -1654,6 +1657,7 @@ int keyring_clear(struct key *keyring)
down_write(&keyring->sem);
+ trace_key_clear(keyring);
edit = assoc_array_clear(&keyring->keys, &keyring_assoc_array_ops);
if (IS_ERR(edit)) {
ret = PTR_ERR(edit);
@@ -1697,14 +1701,27 @@ static bool key_is_dead(const struct key *key, time64_t limit)
if (expiry != TIME64_MAX) {
if (!(key->type->flags & KEY_TYPE_INSTANT_REAP))
expiry += key_gc_delay;
- if (expiry <= limit)
+ if (expiry <= limit) {
+ trace_key_dead(key, key_trace_dead_expired);
return true;
+ }
+ }
+
+ if (test_bit(KEY_FLAG_DEAD, &key->flags)) {
+ trace_key_dead(key, key_trace_dead_type_removed);
+ return true;
+ }
+
+ if (test_bit(KEY_FLAG_INVALIDATED, &key->flags)) {
+ trace_key_dead(key, key_trace_dead_invalidated);
+ return true;
}
- return
- key->flags & ((1 << KEY_FLAG_DEAD) |
- (1 << KEY_FLAG_INVALIDATED)) ||
- key->domain_tag->removed;
+ if (key->domain_tag->removed) {
+ trace_key_dead(key, key_trace_dead_domain_removed);
+ return true;
+ }
+ return false;
}
static bool keyring_gc_select_iterator(void *object, void *iterator_data)
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/7] keys: Out of line key_is_dead() so it can have tracepoints added in
2024-08-21 12:36 ` [PATCH 1/7] keys: Out of line key_is_dead() so it can have tracepoints added in David Howells
@ 2024-08-27 18:22 ` Jarkko Sakkinen
0 siblings, 0 replies; 16+ messages in thread
From: Jarkko Sakkinen @ 2024-08-27 18:22 UTC (permalink / raw)
To: David Howells; +Cc: keyrings, linux-security-module, linux-kernel
On Wed Aug 21, 2024 at 3:36 PM EEST, David Howells wrote:
> Move key_is_dead() out of line so that tracepoints can be added in to it
> without incurring circular #includes. Also, it is only used from the file
> it is moved into.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Jarkko Sakkinen <jarkko@kernel.org>
> cc: keyrings@vger.kernel.org
> cc: linux-security-module@vger.kernel.org
> ---
> security/keys/internal.h | 20 --------------------
> security/keys/keyring.c | 20 ++++++++++++++++++++
> 2 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/security/keys/internal.h b/security/keys/internal.h
> index 2cffa6dc8255..8ba87127e446 100644
> --- a/security/keys/internal.h
> +++ b/security/keys/internal.h
> @@ -211,26 +211,6 @@ extern struct key *request_key_auth_new(struct key *target,
>
> extern struct key *key_get_instantiation_authkey(key_serial_t target_id);
>
> -/*
> - * Determine whether a key is dead.
> - */
> -static inline bool key_is_dead(const struct key *key, time64_t limit)
> -{
> - time64_t expiry = key->expiry;
> -
> - if (expiry != TIME64_MAX) {
> - if (!(key->type->flags & KEY_TYPE_INSTANT_REAP))
> - expiry += key_gc_delay;
> - if (expiry <= limit)
> - return true;
> - }
> -
> - return
> - key->flags & ((1 << KEY_FLAG_DEAD) |
> - (1 << KEY_FLAG_INVALIDATED)) ||
> - key->domain_tag->removed;
> -}
> -
> /*
> * keyctl() functions
> */
> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> index 4448758f643a..0eed018448cb 100644
> --- a/security/keys/keyring.c
> +++ b/security/keys/keyring.c
> @@ -1687,6 +1687,26 @@ static void keyring_revoke(struct key *keyring)
> }
> }
>
> +/*
> + * Determine whether a key is dead.
> + */
> +static bool key_is_dead(const struct key *key, time64_t limit)
> +{
> + time64_t expiry = key->expiry;
> +
> + if (expiry != TIME64_MAX) {
> + if (!(key->type->flags & KEY_TYPE_INSTANT_REAP))
> + expiry += key_gc_delay;
> + if (expiry <= limit)
> + return true;
> + }
> +
> + return
> + key->flags & ((1 << KEY_FLAG_DEAD) |
> + (1 << KEY_FLAG_INVALIDATED)) ||
> + key->domain_tag->removed;
> +}
> +
> static bool keyring_gc_select_iterator(void *object, void *iterator_data)
> {
> struct key *key = keyring_ptr_to_key(object);
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
BR, Jarkko
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/7] keys: Extract struct key_user to its own header for tracing purposes
2024-08-21 12:36 ` [PATCH 2/7] keys: Extract struct key_user to its own header for tracing purposes David Howells
@ 2024-08-27 18:23 ` Jarkko Sakkinen
0 siblings, 0 replies; 16+ messages in thread
From: Jarkko Sakkinen @ 2024-08-27 18:23 UTC (permalink / raw)
To: David Howells; +Cc: keyrings, linux-security-module, linux-kernel
On Wed Aug 21, 2024 at 3:36 PM EEST, David Howells wrote:
> Extract the key_user struct to its own header file to make it easier to
> access from tracepoints.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Jarkko Sakkinen <jarkko@kernel.org>
> cc: keyrings@vger.kernel.org
> cc: linux-security-module@vger.kernel.org
> ---
> include/keys/key_user.h | 35 +++++++++++++++++++++++++++++++++++
> security/keys/internal.h | 20 +-------------------
> 2 files changed, 36 insertions(+), 19 deletions(-)
> create mode 100644 include/keys/key_user.h
>
> diff --git a/include/keys/key_user.h b/include/keys/key_user.h
> new file mode 100644
> index 000000000000..e9c383d8116e
> --- /dev/null
> +++ b/include/keys/key_user.h
> @@ -0,0 +1,35 @@
> +/* User quota tracking for keys.
> + *
> + * Copyright (C) 2024 Red Hat, Inc. All Rights Reserved.
> + * Written by David Howells (dhowells@redhat.com)
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public Licence
> + * as published by the Free Software Foundation; either version
> + * 2 of the Licence, or (at your option) any later version.
> + */
> +
> +#ifndef _KEYS_KEY_USER_H
> +#define _KEYS_KEY_USER_H
> +
> +/*
> + * Keep track of keys for a user.
> + *
> + * This needs to be separate to user_struct to avoid a refcount-loop
> + * (user_struct pins some keyrings which pin this struct).
> + *
> + * We also keep track of keys under request from userspace for this UID here.
> + */
> +struct key_user {
> + struct rb_node node;
> + struct mutex cons_lock; /* construction initiation lock */
> + spinlock_t lock;
> + refcount_t usage; /* for accessing qnkeys & qnbytes */
> + atomic_t nkeys; /* number of keys */
> + atomic_t nikeys; /* number of instantiated keys */
> + kuid_t uid;
> + int qnkeys; /* number of keys allocated to this user */
> + int qnbytes; /* number of bytes allocated to this user */
> +};
> +
> +#endif /* _KEYS_KEY_USER_H */
> diff --git a/security/keys/internal.h b/security/keys/internal.h
> index 8ba87127e446..33c929a6bb97 100644
> --- a/security/keys/internal.h
> +++ b/security/keys/internal.h
> @@ -19,6 +19,7 @@
> #include <linux/compat.h>
> #include <linux/mm.h>
> #include <linux/vmalloc.h>
> +#include <keys/key_user.h>
>
> struct iovec;
>
> @@ -43,25 +44,6 @@ extern struct key_type key_type_user;
> extern struct key_type key_type_logon;
>
> /*****************************************************************************/
> -/*
> - * Keep track of keys for a user.
> - *
> - * This needs to be separate to user_struct to avoid a refcount-loop
> - * (user_struct pins some keyrings which pin this struct).
> - *
> - * We also keep track of keys under request from userspace for this UID here.
> - */
> -struct key_user {
> - struct rb_node node;
> - struct mutex cons_lock; /* construction initiation lock */
> - spinlock_t lock;
> - refcount_t usage; /* for accessing qnkeys & qnbytes */
> - atomic_t nkeys; /* number of keys */
> - atomic_t nikeys; /* number of instantiated keys */
> - kuid_t uid;
> - int qnkeys; /* number of keys allocated to this user */
> - int qnbytes; /* number of bytes allocated to this user */
> -};
>
> extern struct rb_root key_user_tree;
> extern spinlock_t key_user_lock;
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
BR, Jarkko
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/7] keys: Move key_get() out of line so a tracepoint can be added
2024-08-21 12:36 ` [PATCH 3/7] keys: Move key_get() out of line so a tracepoint can be added David Howells
@ 2024-08-27 18:23 ` Jarkko Sakkinen
0 siblings, 0 replies; 16+ messages in thread
From: Jarkko Sakkinen @ 2024-08-27 18:23 UTC (permalink / raw)
To: David Howells; +Cc: keyrings, linux-security-module, linux-kernel
On Wed Aug 21, 2024 at 3:36 PM EEST, David Howells wrote:
> Move key_get() out of line so that a tracepoint can be added into it
> without incurring circular header dependencies or overly expanding the
> callers.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Jarkko Sakkinen <jarkko@kernel.org>
> cc: keyrings@vger.kernel.org
> cc: linux-security-module@vger.kernel.org
> ---
> include/linux/key.h | 6 +-----
> security/keys/key.c | 14 ++++++++++++++
> 2 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/key.h b/include/linux/key.h
> index 943a432da3ae..08062b4f807c 100644
> --- a/include/linux/key.h
> +++ b/include/linux/key.h
> @@ -299,6 +299,7 @@ extern struct key *key_alloc(struct key_type *type,
>
> extern void key_revoke(struct key *key);
> extern void key_invalidate(struct key *key);
> +struct key *key_get(struct key *key);
> extern void key_put(struct key *key);
> extern bool key_put_tag(struct key_tag *tag);
> extern void key_remove_domain(struct key_tag *domain_tag);
> @@ -309,11 +310,6 @@ static inline struct key *__key_get(struct key *key)
> return key;
> }
>
> -static inline struct key *key_get(struct key *key)
> -{
> - return key ? __key_get(key) : key;
> -}
> -
> static inline void key_ref_put(key_ref_t key_ref)
> {
> key_put(key_ref_to_ptr(key_ref));
> diff --git a/security/keys/key.c b/security/keys/key.c
> index 3d7d185019d3..14c7ee77ea15 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -635,6 +635,20 @@ int key_reject_and_link(struct key *key,
> }
> EXPORT_SYMBOL(key_reject_and_link);
>
> +/**
> + * key_get - Get a reference on a key
> + * @key: The key to get a reference on.
> + *
> + * Get a reference on a key, if not NULL, and return the parameter.
> + */
> +struct key *key_get(struct key *key)
> +{
> + if (key)
> + refcount_inc(&key->usage);
> + return key;
> +}
> +EXPORT_SYMBOL(key_get);
> +
> /**
> * key_put - Discard a reference to a key.
> * @key: The key to discard a reference from.
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
BR, Jarkko
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 4/7] keys: Add a key_ref_get() wrapper
2024-08-21 12:36 ` [PATCH 4/7] keys: Add a key_ref_get() wrapper David Howells
@ 2024-08-27 18:23 ` Jarkko Sakkinen
0 siblings, 0 replies; 16+ messages in thread
From: Jarkko Sakkinen @ 2024-08-27 18:23 UTC (permalink / raw)
To: David Howells; +Cc: keyrings, linux-security-module, linux-kernel
On Wed Aug 21, 2024 at 3:36 PM EEST, David Howells wrote:
> Add a key_ref_get() wrapper function to go with key_ref_put() and use it
> for the place that needs it.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Jarkko Sakkinen <jarkko@kernel.org>
> cc: keyrings@vger.kernel.org
> cc: linux-security-module@vger.kernel.org
> ---
> include/linux/key.h | 5 +++++
> security/keys/keyring.c | 2 +-
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/key.h b/include/linux/key.h
> index 08062b4f807c..50a19e5f9e45 100644
> --- a/include/linux/key.h
> +++ b/include/linux/key.h
> @@ -315,6 +315,11 @@ static inline void key_ref_put(key_ref_t key_ref)
> key_put(key_ref_to_ptr(key_ref));
> }
>
> +static inline void key_ref_get(key_ref_t key_ref)
> +{
> + key_get(key_ref_to_ptr(key_ref));
> +}
> +
> extern struct key *request_key_tag(struct key_type *type,
> const char *description,
> struct key_tag *domain_tag,
> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> index 0eed018448cb..7f02b913c560 100644
> --- a/security/keys/keyring.c
> +++ b/security/keys/keyring.c
> @@ -920,7 +920,7 @@ key_ref_t keyring_search_rcu(key_ref_t keyring_ref,
>
> ctx->now = ktime_get_real_seconds();
> if (search_nested_keyrings(keyring, ctx))
> - __key_get(key_ref_to_ptr(ctx->result));
> + key_ref_get(ctx->result);
> return ctx->result;
> }
>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
BR, Jarkko
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 5/7] keys: Use key_get() instead of __key_get()
2024-08-21 12:36 ` [PATCH 5/7] keys: Use key_get() instead of __key_get() David Howells
@ 2024-08-27 18:24 ` Jarkko Sakkinen
0 siblings, 0 replies; 16+ messages in thread
From: Jarkko Sakkinen @ 2024-08-27 18:24 UTC (permalink / raw)
To: David Howells; +Cc: keyrings, linux-security-module, linux-kernel
On Wed Aug 21, 2024 at 3:36 PM EEST, David Howells wrote:
> Switch users of __key_get() over to key_get() so that they benefit from the
> future tracepointage thereof also and remove __key_get().
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Jarkko Sakkinen <jarkko@kernel.org>
> cc: keyrings@vger.kernel.org
> cc: linux-security-module@vger.kernel.org
> ---
> Documentation/security/keys/core.rst | 1 -
> crypto/asymmetric_keys/restrict.c | 6 +++---
> include/linux/key.h | 6 ------
> security/keys/keyring.c | 4 ++--
> security/keys/process_keys.c | 15 ++++++---------
> 5 files changed, 11 insertions(+), 21 deletions(-)
>
> diff --git a/Documentation/security/keys/core.rst b/Documentation/security/keys/core.rst
> index 326b8a973828..0b179540d885 100644
> --- a/Documentation/security/keys/core.rst
> +++ b/Documentation/security/keys/core.rst
> @@ -1217,7 +1217,6 @@ payload contents" for more information.
> * Extra references can be made to a key by calling one of the following
> functions::
>
> - struct key *__key_get(struct key *key);
> struct key *key_get(struct key *key);
>
> Keys so references will need to be disposed of by calling key_put() when
> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
> index afcd4d101ac5..1ea7bfd4e5d7 100644
> --- a/crypto/asymmetric_keys/restrict.c
> +++ b/crypto/asymmetric_keys/restrict.c
> @@ -267,20 +267,20 @@ static int key_or_keyring_common(struct key *dest_keyring,
> if (!sig->auth_ids[0] && !sig->auth_ids[1]) {
> if (asymmetric_key_id_same(signer_ids[2],
> sig->auth_ids[2]))
> - key = __key_get(trusted);
> + key = key_get(trusted);
>
> } else if (!sig->auth_ids[0] || !sig->auth_ids[1]) {
> const struct asymmetric_key_id *auth_id;
>
> auth_id = sig->auth_ids[0] ?: sig->auth_ids[1];
> if (match_either_id(signer_ids, auth_id))
> - key = __key_get(trusted);
> + key = key_get(trusted);
>
> } else if (asymmetric_key_id_same(signer_ids[1],
> sig->auth_ids[1]) &&
> match_either_id(signer_ids,
> sig->auth_ids[0])) {
> - key = __key_get(trusted);
> + key = key_get(trusted);
> }
> } else {
> return -EOPNOTSUPP;
> diff --git a/include/linux/key.h b/include/linux/key.h
> index 50a19e5f9e45..80d736813b89 100644
> --- a/include/linux/key.h
> +++ b/include/linux/key.h
> @@ -304,12 +304,6 @@ extern void key_put(struct key *key);
> extern bool key_put_tag(struct key_tag *tag);
> extern void key_remove_domain(struct key_tag *domain_tag);
>
> -static inline struct key *__key_get(struct key *key)
> -{
> - refcount_inc(&key->usage);
> - return key;
> -}
> -
> static inline void key_ref_put(key_ref_t key_ref)
> {
> key_put(key_ref_to_ptr(key_ref));
> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> index 7f02b913c560..e77d927f1d4d 100644
> --- a/security/keys/keyring.c
> +++ b/security/keys/keyring.c
> @@ -1122,7 +1122,7 @@ key_ref_t find_key_to_update(key_ref_t keyring_ref,
> kleave(" = NULL [x]");
> return NULL;
> }
> - __key_get(key);
> + key_get(key);
> kleave(" = {%d}", key->serial);
> return make_key_ref(key, is_key_possessed(keyring_ref));
> }
> @@ -1367,7 +1367,7 @@ int __key_link_check_live_key(struct key *keyring, struct key *key)
> void __key_link(struct key *keyring, struct key *key,
> struct assoc_array_edit **_edit)
> {
> - __key_get(key);
> + key_get(key);
> assoc_array_insert_set_object(*_edit, keyring_key_to_ptr(key));
> assoc_array_apply_edit(*_edit);
> *_edit = NULL;
> diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
> index b5d5333ab330..01291b2d0888 100644
> --- a/security/keys/process_keys.c
> +++ b/security/keys/process_keys.c
> @@ -333,7 +333,7 @@ int install_session_keyring_to_cred(struct cred *cred, struct key *keyring)
> if (IS_ERR(keyring))
> return PTR_ERR(keyring);
> } else {
> - __key_get(keyring);
> + key_get(keyring);
> }
>
> /* install the keyring */
> @@ -641,7 +641,7 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
> }
>
> key = ctx.cred->thread_keyring;
> - __key_get(key);
> + key_get(key);
> key_ref = make_key_ref(key, 1);
> break;
>
> @@ -658,8 +658,7 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
> goto reget_creds;
> }
>
> - key = ctx.cred->process_keyring;
> - __key_get(key);
> + key = key_get(ctx.cred->process_keyring);
> key_ref = make_key_ref(key, 1);
> break;
>
> @@ -688,8 +687,7 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
> goto reget_creds;
> }
>
> - key = ctx.cred->session_keyring;
> - __key_get(key);
> + key = key_get(ctx.cred->session_keyring);
> key_ref = make_key_ref(key, 1);
> break;
>
> @@ -717,7 +715,7 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
> if (!key)
> goto error;
>
> - __key_get(key);
> + key_get(key);
> key_ref = make_key_ref(key, 1);
> break;
>
> @@ -732,8 +730,7 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
> key = NULL;
> } else {
> rka = ctx.cred->request_key_auth->payload.data[0];
> - key = rka->dest_keyring;
> - __key_get(key);
> + key = key_get(rka->dest_keyring);
> }
> up_read(&ctx.cred->request_key_auth->sem);
> if (!key)
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
BR, Jarkko
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 6/7] keys: Provide a key_try_get() function and use it
2024-08-21 12:36 ` [PATCH 6/7] keys: Provide a key_try_get() function and use it David Howells
@ 2024-08-27 18:24 ` Jarkko Sakkinen
0 siblings, 0 replies; 16+ messages in thread
From: Jarkko Sakkinen @ 2024-08-27 18:24 UTC (permalink / raw)
To: David Howells; +Cc: keyrings, linux-security-module, linux-kernel
On Wed Aug 21, 2024 at 3:36 PM EEST, David Howells wrote:
> Add a key_try_get() function to try to get a ref on a key and switch code
> that's manipulating the key refcount directly to use it. This will allow a
> tracepoint to be emplaced there later.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Jarkko Sakkinen <jarkko@kernel.org>
> cc: keyrings@vger.kernel.org
> cc: linux-security-module@vger.kernel.org
> ---
> include/linux/key.h | 1 +
> security/keys/key.c | 16 +++++++++++++++-
> security/keys/keyring.c | 2 +-
> 3 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/key.h b/include/linux/key.h
> index 80d736813b89..4e5baf3e7286 100644
> --- a/include/linux/key.h
> +++ b/include/linux/key.h
> @@ -300,6 +300,7 @@ extern struct key *key_alloc(struct key_type *type,
> extern void key_revoke(struct key *key);
> extern void key_invalidate(struct key *key);
> struct key *key_get(struct key *key);
> +struct key *key_try_get(struct key *key);
> extern void key_put(struct key *key);
> extern bool key_put_tag(struct key_tag *tag);
> extern void key_remove_domain(struct key_tag *domain_tag);
> diff --git a/security/keys/key.c b/security/keys/key.c
> index 14c7ee77ea15..59cffb6f9b94 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -649,6 +649,20 @@ struct key *key_get(struct key *key)
> }
> EXPORT_SYMBOL(key_get);
>
> +/**
> + * key_try_get - Get a ref on a key if its refcount is not non-zero.
> + * @key: The key to get a reference on.
> + *
> + * Get a reference on a key unless it has no references and return true if
> + * successful. @key must not be NULL.
> + */
> +struct key *key_try_get(struct key *key)
> +{
> + if (!refcount_inc_not_zero(&key->usage))
> + return NULL;
> + return key;
> +}
> +
> /**
> * key_put - Discard a reference to a key.
> * @key: The key to discard a reference from.
> @@ -709,7 +723,7 @@ struct key *key_lookup(key_serial_t id)
> /* A key is allowed to be looked up only if someone still owns a
> * reference to it - otherwise it's awaiting the gc.
> */
> - if (!refcount_inc_not_zero(&key->usage))
> + if (!key_try_get(key))
> goto not_found;
>
> error:
> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> index e77d927f1d4d..a09a4c2b1bcb 100644
> --- a/security/keys/keyring.c
> +++ b/security/keys/keyring.c
> @@ -1174,7 +1174,7 @@ struct key *find_keyring_by_name(const char *name, bool uid_keyring)
> /* we've got a match but we might end up racing with
> * key_cleanup() if the keyring is currently 'dead'
> * (ie. it has a zero usage count) */
> - if (!refcount_inc_not_zero(&keyring->usage))
> + if (!key_try_get(keyring))
> continue;
> keyring->last_used_at = ktime_get_real_seconds();
> goto out;
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
BR, Jarkko
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 7/7] keys: Add tracepoints for the keyrings facility
2024-08-21 12:36 ` [PATCH 7/7] keys: Add tracepoints for the keyrings facility David Howells
@ 2024-08-27 18:27 ` Jarkko Sakkinen
2024-09-28 2:03 ` Justin Stitt
1 sibling, 0 replies; 16+ messages in thread
From: Jarkko Sakkinen @ 2024-08-27 18:27 UTC (permalink / raw)
To: David Howells; +Cc: keyrings, linux-security-module, linux-kernel
On Wed Aug 21, 2024 at 3:36 PM EEST, David Howells wrote:
> Add some tracepoints to aid in debuggin the keyrings facility and
> applications that use it. A number of events and operations are traceable,
> including:
>
> - Allocation
> - Refcounting
> - Instantiation and negative instantiation/rejection
> - Update
> - Detection of key being dead
> - Key quota changes
> - Key quota failure
> - Link, unlink and move
> - Keyring clearance
> - Revocation and invalidation
> - Garbage collection
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Jarkko Sakkinen <jarkko@kernel.org>
> cc: keyrings@vger.kernel.org
> cc: linux-security-module@vger.kernel.org
I have immediate use for this when I take the time and refine [1] so
I guess that goes for existential reason :-)
> ---
> include/trace/events/key.h | 401 +++++++++++++++++++++++++++++++++++++
> security/keys/gc.c | 4 +
> security/keys/internal.h | 1 +
> security/keys/key.c | 50 ++++-
> security/keys/keyctl.c | 2 +
> security/keys/keyring.c | 27 ++-
> 6 files changed, 472 insertions(+), 13 deletions(-)
> create mode 100644 include/trace/events/key.h
>
> diff --git a/include/trace/events/key.h b/include/trace/events/key.h
> new file mode 100644
> index 000000000000..b3f8c39cc0e8
> --- /dev/null
> +++ b/include/trace/events/key.h
> @@ -0,0 +1,401 @@
> +/* Keyrings tracepoints
> + *
> + * Copyright (C) 2024 Red Hat, Inc. All Rights Reserved.
> + * Written by David Howells (dhowells@redhat.com)
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public Licence
> + * as published by the Free Software Foundation; either version
> + * 2 of the Licence, or (at your option) any later version.
> + */
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM key
> +
> +#if !defined(_TRACE_KEY_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_KEY_H
> +
> +#include <linux/tracepoint.h>
> +
> +struct key;
> +
> +/*
> + * Declare tracing information enums and their string mappings for display.
> + */
> +#define key_ref_traces \
> + EM(key_trace_ref_alloc, "ALLOC") \
> + EM(key_trace_ref_free, "FREE ") \
> + EM(key_trace_ref_gc, "GC ") \
> + EM(key_trace_ref_get, "GET ") \
> + EM(key_trace_ref_put, "PUT ") \
> + E_(key_trace_ref_try_get, "GET ")
> +
> +#define key_dead_traces \
> + EM(key_trace_dead_type_removed, "TYPR") \
> + EM(key_trace_dead_domain_removed, "DOMR") \
> + EM(key_trace_dead_expired, "EXPD") \
> + E_(key_trace_dead_invalidated, "INVL")
> +
> +/*
> + * Generate enums for tracing information.
> + */
> +#ifndef __NETFS_DECLARE_TRACE_ENUMS_ONCE_ONLY
> +#define __NETFS_DECLARE_TRACE_ENUMS_ONCE_ONLY
> +
> +#undef EM
> +#undef E_
> +#define EM(a, b) a,
> +#define E_(a, b) a
> +
> +enum key_dead_trace { key_dead_traces } __mode(byte);
> +enum key_ref_trace { key_ref_traces } __mode(byte);
> +
> +#endif /* end __RXRPC_DECLARE_TRACE_ENUMS_ONCE_ONLY */
> +
> +/*
> + * Export enum symbols via userspace.
> + */
> +#undef EM
> +#undef E_
> +#define EM(a, b) TRACE_DEFINE_ENUM(a);
> +#define E_(a, b) TRACE_DEFINE_ENUM(a);
> +
> +key_dead_traces;
> +key_ref_traces;
> +
> +/*
> + * Now redefine the EM() and E_() macros to map the enums to the strings that
> + * will be printed in the output.
> + */
> +#undef EM
> +#undef E_
> +#define EM(a, b) { a, b },
> +#define E_(a, b) { a, b }
> +
> +TRACE_EVENT(key_alloc,
> + TP_PROTO(const struct key *key),
> +
> + TP_ARGS(key),
> +
> + TP_STRUCT__entry(
> + __field(key_serial_t, key)
> + __field(uid_t, uid)
> + __array(char, type, 8)
> + __array(char, desc, 24)
> + ),
> +
> + TP_fast_assign(
> + __entry->key = key->serial;
> + __entry->uid = from_kuid(&init_user_ns, key->uid);
> + strncpy(__entry->type, key->type->name, sizeof(__entry->type) - 1);
> + strncpy(__entry->desc, key->description ?: "", sizeof(__entry->desc) - 1);
> + __entry->type[sizeof(__entry->type) - 1] = 0;
> + __entry->desc[sizeof(__entry->desc) - 1] = 0;
> + ),
> +
> + TP_printk("key=%08x uid=%08x t=%s d=%s",
> + __entry->key,
> + __entry->uid,
> + __entry->type,
> + __entry->desc)
> + );
> +
> +TRACE_EVENT(key_ref,
> + TP_PROTO(key_serial_t key, unsigned int ref, enum key_ref_trace trace,
> + const void *where),
> +
> + TP_ARGS(key, ref, trace, where),
> +
> + TP_STRUCT__entry(
> + __field(key_serial_t, key)
> + __field(enum key_ref_trace, trace)
> + __field(unsigned int, ref)
> + __field(const void *, where)
> + ),
> +
> + TP_fast_assign(
> + __entry->key = key;
> + __entry->trace = trace;
> + __entry->ref = ref;
> + __entry->where = where;
> + ),
> +
> + TP_printk("key=%08x %s r=%d pc=%pSR",
> + __entry->key,
> + __print_symbolic(__entry->trace, key_ref_traces),
> + __entry->ref,
> + __entry->where)
> + );
> +
> +TRACE_EVENT(key_instantiate,
> + TP_PROTO(const struct key *key, const struct key_preparsed_payload *prep),
> +
> + TP_ARGS(key, prep),
> +
> + TP_STRUCT__entry(
> + __field(key_serial_t, key)
> + __field(unsigned int, datalen)
> + __field(unsigned int, quotalen)
> + ),
> +
> + TP_fast_assign(
> + __entry->key = key->serial;
> + __entry->datalen = prep->datalen;
> + __entry->quotalen = prep->quotalen;
> + ),
> +
> + TP_printk("key=%08x dlen=%u qlen=%u",
> + __entry->key,
> + __entry->datalen,
> + __entry->quotalen)
> + );
> +
> +TRACE_EVENT(key_reject,
> + TP_PROTO(const struct key *key, int error),
> +
> + TP_ARGS(key, error),
> +
> + TP_STRUCT__entry(
> + __field(key_serial_t, key)
> + __field(int, error)
> + ),
> +
> + TP_fast_assign(
> + __entry->key = key->serial;
> + __entry->error = error;
> + ),
> +
> + TP_printk("key=%08x err=%d",
> + __entry->key,
> + __entry->error)
> + );
> +
> +TRACE_EVENT(key_update,
> + TP_PROTO(const struct key *key, const struct key_preparsed_payload *prep),
> +
> + TP_ARGS(key, prep),
> +
> + TP_STRUCT__entry(
> + __field(key_serial_t, key)
> + __field(unsigned int, datalen)
> + __field(unsigned int, quotalen)
> + ),
> +
> + TP_fast_assign(
> + __entry->key = key->serial;
> + __entry->datalen = prep->datalen;
> + __entry->quotalen = prep->quotalen;
> + ),
> +
> + TP_printk("key=%08x dlen=%u qlen=%u",
> + __entry->key,
> + __entry->datalen,
> + __entry->quotalen)
> + );
> +
> +TRACE_EVENT(key_dead,
> + TP_PROTO(const struct key *key, enum key_dead_trace trace),
> +
> + TP_ARGS(key, trace),
> +
> + TP_STRUCT__entry(
> + __field(key_serial_t, key)
> + __field(enum key_dead_trace, trace)
> + ),
> +
> + TP_fast_assign(
> + __entry->key = key->serial;
> + __entry->trace = trace;
> + ),
> +
> + TP_printk("key=%08x %s",
> + __entry->key,
> + __print_symbolic(__entry->trace, key_dead_traces))
> + );
> +
> +TRACE_EVENT(key_quota,
> + TP_PROTO(const struct key_user *user, int change_keys, int change_bytes),
> +
> + TP_ARGS(user, change_keys, change_bytes),
> +
> + TP_STRUCT__entry(
> + __field(uid_t, uid)
> + __field(unsigned int, nkeys)
> + __field(unsigned int, nikeys)
> + __field(unsigned int, qnkeys)
> + __field(unsigned int, qnbytes)
> + __field(int, change_keys)
> + __field(int, change_bytes)
> + ),
> +
> + TP_fast_assign(
> + __entry->uid = from_kuid(&init_user_ns, user->uid);
> + __entry->nkeys = atomic_read(&user->nkeys);
> + __entry->nikeys = atomic_read(&user->nikeys);
> + __entry->qnkeys = user->qnkeys;
> + __entry->qnbytes = user->qnbytes;
> + __entry->change_keys = change_keys;
> + __entry->change_bytes = change_bytes;
> + ),
> +
> + TP_printk("uid=%d nkeys=%u/%u qkeys=%u qpay=%u ckeys=%d cpay=%d",
> + __entry->uid,
> + __entry->nikeys, __entry->nkeys,
> + __entry->qnkeys,
> + __entry->qnbytes,
> + __entry->change_keys, __entry->change_bytes)
> + );
> +
> +TRACE_EVENT(key_edquot,
> + TP_PROTO(const struct key_user *user, unsigned int maxkeys,
> + unsigned int maxbytes, unsigned int reqbytes),
> +
> + TP_ARGS(user, maxkeys, maxbytes, reqbytes),
> +
> + TP_STRUCT__entry(
> + __field(uid_t, uid)
> + __field(unsigned int, nkeys)
> + __field(unsigned int, nikeys)
> + __field(unsigned int, qnkeys)
> + __field(unsigned int, qnbytes)
> + __field(unsigned int, maxkeys)
> + __field(unsigned int, maxbytes)
> + __field(unsigned int, reqbytes)
> + ),
> +
> + TP_fast_assign(
> + __entry->uid = from_kuid(&init_user_ns, user->uid);
> + __entry->nkeys = atomic_read(&user->nkeys);
> + __entry->nikeys = atomic_read(&user->nikeys);
> + __entry->qnkeys = user->qnkeys;
> + __entry->qnbytes = user->qnbytes;
> + __entry->maxkeys = maxkeys;
> + __entry->maxbytes = maxbytes;
> + __entry->reqbytes = reqbytes;
> + ),
> +
> + TP_printk("u=%d nkeys=%u/%u qkeys=%u/%u qpay=%u/%u cpay=%u",
> + __entry->uid,
> + __entry->nikeys, __entry->nkeys,
> + __entry->qnkeys, __entry->maxkeys,
> + __entry->qnbytes, __entry->maxbytes,
> + __entry->reqbytes)
> + );
> +
> +TRACE_EVENT(key_link,
> + TP_PROTO(const struct key *keyring, const struct key *key),
> +
> + TP_ARGS(keyring, key),
> +
> + TP_STRUCT__entry(
> + __field(key_serial_t, keyring)
> + __field(key_serial_t, key)
> + ),
> +
> + TP_fast_assign(
> + __entry->keyring = keyring->serial;
> + __entry->key = key->serial;
> + ),
> +
> + TP_printk("key=%08x to=%08x",
> + __entry->key, __entry->keyring)
> + );
> +
> +TRACE_EVENT(key_unlink,
> + TP_PROTO(const struct key *keyring, const struct key *key),
> +
> + TP_ARGS(keyring, key),
> +
> + TP_STRUCT__entry(
> + __field(key_serial_t, keyring)
> + __field(key_serial_t, key)
> + ),
> +
> + TP_fast_assign(
> + __entry->keyring = keyring->serial;
> + __entry->key = key->serial;
> + ),
> +
> + TP_printk("key=%08x from=%08x",
> + __entry->key, __entry->keyring)
> + );
> +
> +TRACE_EVENT(key_move,
> + TP_PROTO(const struct key *from, const struct key *to,
> + const struct key *key),
> +
> + TP_ARGS(from, to, key),
> +
> + TP_STRUCT__entry(
> + __field(key_serial_t, from)
> + __field(key_serial_t, to)
> + __field(key_serial_t, key)
> + ),
> +
> + TP_fast_assign(
> + __entry->from = from->serial;
> + __entry->to = to->serial;
> + __entry->key = key->serial;
> + ),
> +
> + TP_printk("key=%08x from=%08x to=%08x",
> + __entry->key, __entry->from, __entry->to)
> + );
> +
> +TRACE_EVENT(key_clear,
> + TP_PROTO(const struct key *keyring),
> + TP_ARGS(keyring),
> + TP_STRUCT__entry(
> + __field(key_serial_t, keyring)
> + ),
> + TP_fast_assign(
> + __entry->keyring = keyring->serial;
> + ),
> + TP_printk("key=%08x",
> + __entry->keyring)
> + );
> +
> +TRACE_EVENT(key_revoke,
> + TP_PROTO(const struct key *key),
> + TP_ARGS(key),
> + TP_STRUCT__entry(
> + __field(key_serial_t, key)
> + ),
> + TP_fast_assign(
> + __entry->key = key->serial;
> + ),
> + TP_printk("key=%08x",
> + __entry->key)
> + );
> +
> +TRACE_EVENT(key_invalidate,
> + TP_PROTO(const struct key *key),
> + TP_ARGS(key),
> + TP_STRUCT__entry(
> + __field(key_serial_t, key)
> + ),
> + TP_fast_assign(
> + __entry->key = key->serial;
> + ),
> + TP_printk("key=%08x",
> + __entry->key)
> + );
> +
> +TRACE_EVENT(key_gc,
> + TP_PROTO(const struct key *key),
> + TP_ARGS(key),
> + TP_STRUCT__entry(
> + __field(key_serial_t, key)
> + ),
> + TP_fast_assign(
> + __entry->key = key->serial;
> + ),
> + TP_printk("key=%08x",
> + __entry->key)
> + );
> +
> +#undef EM
> +#undef E_
> +#endif /* _TRACE_KEY_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
> diff --git a/security/keys/gc.c b/security/keys/gc.c
> index 7d687b0962b1..47f857653854 100644
> --- a/security/keys/gc.c
> +++ b/security/keys/gc.c
> @@ -141,6 +141,7 @@ static noinline void key_gc_unused_keys(struct list_head *keys)
>
> list_del(&key->graveyard_link);
>
> + trace_key_gc(key);
> kdebug("- %u", key->serial);
> key_check(key);
>
> @@ -163,6 +164,8 @@ static noinline void key_gc_unused_keys(struct list_head *keys)
> key_put_tag(key->domain_tag);
> kfree(key->description);
>
> + trace_key_ref(key->serial, refcount_read(&key->usage),
> + key_trace_ref_free, 0);
> memzero_explicit(key, sizeof(*key));
> kmem_cache_free(key_jar, key);
> }
> @@ -331,6 +334,7 @@ static void key_garbage_collector(struct work_struct *work)
> */
> found_unreferenced_key:
> kdebug("unrefd key %d", key->serial);
> + trace_key_ref(key->serial, refcount_read(&key->usage), key_trace_ref_gc, 0);
> rb_erase(&key->serial_node, &key_serial_tree);
> spin_unlock(&key_serial_lock);
>
> diff --git a/security/keys/internal.h b/security/keys/internal.h
> index 33c929a6bb97..87600683a7f5 100644
> --- a/security/keys/internal.h
> +++ b/security/keys/internal.h
> @@ -20,6 +20,7 @@
> #include <linux/mm.h>
> #include <linux/vmalloc.h>
> #include <keys/key_user.h>
> +#include <trace/events/key.h>
>
> struct iovec;
>
> diff --git a/security/keys/key.c b/security/keys/key.c
> index 59cffb6f9b94..abb8d60a0dc2 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -14,6 +14,7 @@
> #include <linux/workqueue.h>
> #include <linux/random.h>
> #include <linux/err.h>
> +#define CREATE_TRACE_POINTS
> #include "internal.h"
>
> struct kmem_cache *key_jar;
> @@ -264,12 +265,15 @@ struct key *key_alloc(struct key_type *type, const char *desc,
> if (!(flags & KEY_ALLOC_QUOTA_OVERRUN)) {
> if (user->qnkeys + 1 > maxkeys ||
> user->qnbytes + quotalen > maxbytes ||
> - user->qnbytes + quotalen < user->qnbytes)
> + user->qnbytes + quotalen < user->qnbytes) {
> + trace_key_edquot(user, maxkeys, maxbytes, quotalen);
> goto no_quota;
> + }
> }
>
> user->qnkeys++;
> user->qnbytes += quotalen;
> + trace_key_quota(user, +1, quotalen);
> spin_unlock_irqrestore(&user->lock, irqflags);
> }
>
> @@ -319,7 +323,10 @@ struct key *key_alloc(struct key_type *type, const char *desc,
> /* publish the key by giving it a serial number */
> refcount_inc(&key->domain_tag->usage);
> atomic_inc(&user->nkeys);
> + trace_key_quota(user, 0, 0);
> key_alloc_serial(key);
> + trace_key_alloc(key);
> + trace_key_ref(key->serial, 1, key_trace_ref_alloc, __builtin_return_address(0));
>
> error:
> return key;
> @@ -331,6 +338,7 @@ struct key *key_alloc(struct key_type *type, const char *desc,
> spin_lock_irqsave(&user->lock, irqflags);
> user->qnkeys--;
> user->qnbytes -= quotalen;
> + trace_key_quota(user, -1, -quotalen);
> spin_unlock_irqrestore(&user->lock, irqflags);
> }
> key_user_put(user);
> @@ -344,6 +352,7 @@ struct key *key_alloc(struct key_type *type, const char *desc,
> spin_lock_irqsave(&user->lock, irqflags);
> user->qnkeys--;
> user->qnbytes -= quotalen;
> + trace_key_quota(user, -1, quotalen);
> spin_unlock_irqrestore(&user->lock, irqflags);
> }
> key_user_put(user);
> @@ -388,11 +397,13 @@ int key_payload_reserve(struct key *key, size_t datalen)
> if (delta > 0 &&
> (key->user->qnbytes + delta > maxbytes ||
> key->user->qnbytes + delta < key->user->qnbytes)) {
> + trace_key_edquot(key->user, 0, maxbytes, datalen);
> ret = -EDQUOT;
> }
> else {
> key->user->qnbytes += delta;
> key->quotalen += delta;
> + trace_key_quota(key->user, 0, delta);
> }
> spin_unlock_irqrestore(&key->user->lock, flags);
> }
> @@ -447,6 +458,7 @@ static int __key_instantiate_and_link(struct key *key,
> if (ret == 0) {
> /* mark the key as being instantiated */
> atomic_inc(&key->user->nikeys);
> + trace_key_instantiate(key, prep);
> mark_key_instantiated(key, 0);
> notify_key(key, NOTIFY_KEY_INSTANTIATED, 0);
>
> @@ -604,6 +616,7 @@ int key_reject_and_link(struct key *key,
> if (key->state == KEY_IS_UNINSTANTIATED) {
> /* mark the key as being negatively instantiated */
> atomic_inc(&key->user->nikeys);
> + trace_key_reject(key, -error);
> mark_key_instantiated(key, -error);
> notify_key(key, NOTIFY_KEY_INSTANTIATED, -error);
> key_set_expiry(key, ktime_get_real_seconds() + timeout);
> @@ -641,10 +654,15 @@ EXPORT_SYMBOL(key_reject_and_link);
> *
> * Get a reference on a key, if not NULL, and return the parameter.
> */
> -struct key *key_get(struct key *key)
> +noinline struct key *key_get(struct key *key)
> {
> - if (key)
> - refcount_inc(&key->usage);
> + if (key) {
> + int r;
> +
> + __refcount_inc(&key->usage, &r);
> + trace_key_ref(key->serial, r + 1, key_trace_ref_get,
> + __builtin_return_address(0));
> + }
> return key;
> }
> EXPORT_SYMBOL(key_get);
> @@ -656,10 +674,14 @@ EXPORT_SYMBOL(key_get);
> * Get a reference on a key unless it has no references and return true if
> * successful. @key must not be NULL.
> */
> -struct key *key_try_get(struct key *key)
> +noinline struct key *key_try_get(struct key *key)
> {
> - if (!refcount_inc_not_zero(&key->usage))
> + int r;
> +
> + if (!__refcount_inc_not_zero(&key->usage, &r))
> return NULL;
> + trace_key_ref(key->serial, r + 1, key_trace_ref_try_get,
> + __builtin_return_address(0));
> return key;
> }
>
> @@ -671,12 +693,19 @@ struct key *key_try_get(struct key *key)
> * schedule the cleanup task to come and pull it out of the tree in process
> * context at some later time.
> */
> -void key_put(struct key *key)
> +noinline void key_put(struct key *key)
> {
> if (key) {
> + key_serial_t serial = key->serial;
> + bool zero;
> + int r;
> +
> key_check(key);
>
> - if (refcount_dec_and_test(&key->usage)) {
> + zero = __refcount_dec_and_test(&key->usage, &r);
> + trace_key_ref(serial, r - 1, key_trace_ref_put,
> + __builtin_return_address(0));
> + if (zero) {
> unsigned long flags;
>
> /* deal with the user's key tracking and quota */
> @@ -684,6 +713,7 @@ void key_put(struct key *key)
> spin_lock_irqsave(&key->user->lock, flags);
> key->user->qnkeys--;
> key->user->qnbytes -= key->quotalen;
> + trace_key_quota(key->user, -1, -key->quotalen);
> spin_unlock_irqrestore(&key->user->lock, flags);
> }
> schedule_work(&key_gc_work);
> @@ -807,6 +837,7 @@ static inline key_ref_t __key_update(key_ref_t key_ref,
> ret = key->type->update(key, prep);
> if (ret == 0) {
> /* Updating a negative key positively instantiates it */
> + trace_key_update(key, prep);
> mark_key_instantiated(key, 0);
> notify_key(key, NOTIFY_KEY_UPDATED, 0);
> }
> @@ -1131,6 +1162,7 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen)
> ret = key->type->update(key, &prep);
> if (ret == 0) {
> /* Updating a negative key positively instantiates it */
> + trace_key_update(key, &prep);
> mark_key_instantiated(key, 0);
> notify_key(key, NOTIFY_KEY_UPDATED, 0);
> }
> @@ -1166,6 +1198,7 @@ void key_revoke(struct key *key)
> */
> down_write_nested(&key->sem, 1);
> if (!test_and_set_bit(KEY_FLAG_REVOKED, &key->flags)) {
> + trace_key_revoke(key);
> notify_key(key, NOTIFY_KEY_REVOKED, 0);
> if (key->type->revoke)
> key->type->revoke(key);
> @@ -1198,6 +1231,7 @@ void key_invalidate(struct key *key)
> if (!test_bit(KEY_FLAG_INVALIDATED, &key->flags)) {
> down_write_nested(&key->sem, 1);
> if (!test_and_set_bit(KEY_FLAG_INVALIDATED, &key->flags)) {
> + trace_key_invalidate(key);
> notify_key(key, NOTIFY_KEY_INVALIDATED, 0);
> key_schedule_gc_links();
> }
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index ab927a142f51..f2ef898209e6 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -1020,11 +1020,13 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
>
> newowner->qnkeys++;
> newowner->qnbytes += key->quotalen;
> + trace_key_quota(newowner, +1, key->quotalen);
> spin_unlock_irqrestore(&newowner->lock, flags);
>
> spin_lock_irqsave(&key->user->lock, flags);
> key->user->qnkeys--;
> key->user->qnbytes -= key->quotalen;
> + trace_key_quota(newowner, -1, -key->quotalen);
> spin_unlock_irqrestore(&key->user->lock, flags);
> }
>
> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> index a09a4c2b1bcb..d67a60142318 100644
> --- a/security/keys/keyring.c
> +++ b/security/keys/keyring.c
> @@ -1368,6 +1368,7 @@ void __key_link(struct key *keyring, struct key *key,
> struct assoc_array_edit **_edit)
> {
> key_get(key);
> + trace_key_link(keyring, key);
> assoc_array_insert_set_object(*_edit, keyring_key_to_ptr(key));
> assoc_array_apply_edit(*_edit);
> *_edit = NULL;
> @@ -1506,6 +1507,7 @@ static int __key_unlink_begin(struct key *keyring, struct key *key,
> static void __key_unlink(struct key *keyring, struct key *key,
> struct assoc_array_edit **_edit)
> {
> + trace_key_unlink(keyring, key);
> assoc_array_apply_edit(*_edit);
> notify_key(keyring, NOTIFY_KEY_UNLINKED, key_serial(key));
> *_edit = NULL;
> @@ -1625,6 +1627,7 @@ int key_move(struct key *key,
> if (ret < 0)
> goto error;
>
> + trace_key_move(from_keyring, to_keyring, key);
> __key_unlink(from_keyring, key, &from_edit);
> __key_link(to_keyring, key, &to_edit);
> error:
> @@ -1654,6 +1657,7 @@ int keyring_clear(struct key *keyring)
>
> down_write(&keyring->sem);
>
> + trace_key_clear(keyring);
> edit = assoc_array_clear(&keyring->keys, &keyring_assoc_array_ops);
> if (IS_ERR(edit)) {
> ret = PTR_ERR(edit);
> @@ -1697,14 +1701,27 @@ static bool key_is_dead(const struct key *key, time64_t limit)
> if (expiry != TIME64_MAX) {
> if (!(key->type->flags & KEY_TYPE_INSTANT_REAP))
> expiry += key_gc_delay;
> - if (expiry <= limit)
> + if (expiry <= limit) {
> + trace_key_dead(key, key_trace_dead_expired);
> return true;
> + }
> + }
> +
> + if (test_bit(KEY_FLAG_DEAD, &key->flags)) {
> + trace_key_dead(key, key_trace_dead_type_removed);
> + return true;
> + }
> +
> + if (test_bit(KEY_FLAG_INVALIDATED, &key->flags)) {
> + trace_key_dead(key, key_trace_dead_invalidated);
> + return true;
> }
>
> - return
> - key->flags & ((1 << KEY_FLAG_DEAD) |
> - (1 << KEY_FLAG_INVALIDATED)) ||
> - key->domain_tag->removed;
> + if (key->domain_tag->removed) {
> + trace_key_dead(key, key_trace_dead_domain_removed);
> + return true;
> + }
> + return false;
> }
>
> static bool keyring_gc_select_iterator(void *object, void *iterator_data)
Splits matter because you can "set and forget" stuff now there is not
much to review in this main patch because the change is so dead
obvious and at sight:
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
I also gave quick test:
Tested-by: Jarkko Sakkinen <jarkko@kernel.org>
[1] TPM2 asymmetric keys:
https://lore.kernel.org/linux-integrity/20240528210823.28798-1-jarkko@kernel.org/
BR, Jarkko
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 7/7] keys: Add tracepoints for the keyrings facility
2024-08-21 12:36 ` [PATCH 7/7] keys: Add tracepoints for the keyrings facility David Howells
2024-08-27 18:27 ` Jarkko Sakkinen
@ 2024-09-28 2:03 ` Justin Stitt
1 sibling, 0 replies; 16+ messages in thread
From: Justin Stitt @ 2024-09-28 2:03 UTC (permalink / raw)
To: David Howells
Cc: Jarkko Sakkinen, keyrings, linux-security-module, linux-kernel
Hi,
On Wed, Aug 21, 2024 at 01:36:15PM GMT, David Howells wrote:
> Add some tracepoints to aid in debuggin the keyrings facility and
> applications that use it. A number of events and operations are traceable,
> including:
>
> - Allocation
> - Refcounting
> - Instantiation and negative instantiation/rejection
> - Update
> - Detection of key being dead
> - Key quota changes
> - Key quota failure
> - Link, unlink and move
> - Keyring clearance
> - Revocation and invalidation
> - Garbage collection
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Jarkko Sakkinen <jarkko@kernel.org>
> cc: keyrings@vger.kernel.org
> cc: linux-security-module@vger.kernel.org
> ---
> include/trace/events/key.h | 401 +++++++++++++++++++++++++++++++++++++
> security/keys/gc.c | 4 +
> security/keys/internal.h | 1 +
> security/keys/key.c | 50 ++++-
> security/keys/keyctl.c | 2 +
> security/keys/keyring.c | 27 ++-
> 6 files changed, 472 insertions(+), 13 deletions(-)
> create mode 100644 include/trace/events/key.h
>
> diff --git a/include/trace/events/key.h b/include/trace/events/key.h
> new file mode 100644
> index 000000000000..b3f8c39cc0e8
> --- /dev/null
> +++ b/include/trace/events/key.h
> @@ -0,0 +1,401 @@
<snip>
> + TP_STRUCT__entry(
> + __field(key_serial_t, key)
> + __field(uid_t, uid)
> + __array(char, type, 8)
> + __array(char, desc, 24)
> + ),
> +
> + TP_fast_assign(
> + __entry->key = key->serial;
> + __entry->uid = from_kuid(&init_user_ns, key->uid);
> + strncpy(__entry->type, key->type->name, sizeof(__entry->type) - 1);
> + strncpy(__entry->desc, key->description ?: "", sizeof(__entry->desc) - 1);
> + __entry->type[sizeof(__entry->type) - 1] = 0;
> + __entry->desc[sizeof(__entry->desc) - 1] = 0;
Looks like these want to be NUL-terminated. Can we use strscpy or
strscpy_pad since strncpy is deprecated [1] for use on NUL-terminated
strings.
> + ),
> +
> + TP_printk("key=%08x uid=%08x t=%s d=%s",
> + __entry->key,
> + __entry->uid,
> + __entry->type,
> + __entry->desc)
> + );
> +
<snip>
[1]: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
Thanks
Justin
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2024-09-28 2:03 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-21 12:36 [PATCH 0/7] keys: Add tracepoints David Howells
2024-08-21 12:36 ` [PATCH 1/7] keys: Out of line key_is_dead() so it can have tracepoints added in David Howells
2024-08-27 18:22 ` Jarkko Sakkinen
2024-08-21 12:36 ` [PATCH 2/7] keys: Extract struct key_user to its own header for tracing purposes David Howells
2024-08-27 18:23 ` Jarkko Sakkinen
2024-08-21 12:36 ` [PATCH 3/7] keys: Move key_get() out of line so a tracepoint can be added David Howells
2024-08-27 18:23 ` Jarkko Sakkinen
2024-08-21 12:36 ` [PATCH 4/7] keys: Add a key_ref_get() wrapper David Howells
2024-08-27 18:23 ` Jarkko Sakkinen
2024-08-21 12:36 ` [PATCH 5/7] keys: Use key_get() instead of __key_get() David Howells
2024-08-27 18:24 ` Jarkko Sakkinen
2024-08-21 12:36 ` [PATCH 6/7] keys: Provide a key_try_get() function and use it David Howells
2024-08-27 18:24 ` Jarkko Sakkinen
2024-08-21 12:36 ` [PATCH 7/7] keys: Add tracepoints for the keyrings facility David Howells
2024-08-27 18:27 ` Jarkko Sakkinen
2024-09-28 2:03 ` Justin Stitt
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 NNTP newsgroup(s).