| Message ID | 153186087257.27463.13382652631526471099.stgit@djiang5-desk3.ch.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 17, 2018 at 01:54:32PM -0700, Dave Jiang wrote: > Adding key_put_sync() that calls flush_work on key_gc_work to ensure the > key we want to remove is gone. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> Why is this needed? Isn't key_unlink() or key_invalidate() enough? - Eric > --- > include/linux/key.h | 1 + > security/keys/key.c | 35 +++++++++++++++++++++++++++++------ > 2 files changed, 30 insertions(+), 6 deletions(-) > > diff --git a/include/linux/key.h b/include/linux/key.h > index e58ee10f6e58..4dffbd23d4e4 100644 > --- a/include/linux/key.h > +++ b/include/linux/key.h > @@ -253,6 +253,7 @@ extern struct key *key_alloc(struct key_type *type, > extern void key_revoke(struct key *key); > extern void key_invalidate(struct key *key); > extern void key_put(struct key *key); > +extern void key_put_sync(struct key *key); > > static inline struct key *__key_get(struct key *key) > { > diff --git a/security/keys/key.c b/security/keys/key.c > index d97c9394b5dd..b6f3ec19ab0f 100644 > --- a/security/keys/key.c > +++ b/security/keys/key.c > @@ -622,6 +622,19 @@ int key_reject_and_link(struct key *key, > } > EXPORT_SYMBOL(key_reject_and_link); > > +static void __key_put(struct key *key, bool sync) > +{ > + if (key) { > + key_check(key); > + > + if (refcount_dec_and_test(&key->usage)) { > + schedule_work(&key_gc_work); > + if (sync) > + flush_work(&key_gc_work); > + } > + } > +} > + > /** > * key_put - Discard a reference to a key. > * @key: The key to discard a reference from. > @@ -632,15 +645,25 @@ EXPORT_SYMBOL(key_reject_and_link); > */ > void key_put(struct key *key) > { > - if (key) { > - key_check(key); > - > - if (refcount_dec_and_test(&key->usage)) > - schedule_work(&key_gc_work); > - } > + __key_put(key, false); > } > EXPORT_SYMBOL(key_put); > > +/** > + * key_put - Discard a reference to a key and flush the key gc work item. > + * @key: The key to discard a reference from. > + * > + * Discard a reference to a key, and when all the references are gone, we > + * schedule the cleanup task to come and pull it out of the tree in process > + * context at some later time. This call flushes the clean up so we are sure > + * that the key is gone. > + */ > +void key_put_sync(struct key *key) > +{ > + __key_put(key, true); > +} > +EXPORT_SYMBOL(key_put_sync); > + > /* > * Find a key by its serial number. > */ > > -- > To unsubscribe from this list: send the line "unsubscribe keyrings" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/17/2018 04:53 PM, Eric Biggers wrote: > On Tue, Jul 17, 2018 at 01:54:32PM -0700, Dave Jiang wrote: >> Adding key_put_sync() that calls flush_work on key_gc_work to ensure the >> key we want to remove is gone. >> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> > > Why is this needed? Isn't key_unlink() or key_invalidate() enough? Without this, I was getting -EKEYREVOKED when calling request_key() and it wasn't pulling from userspace, even with key_invalidate() called on the key. I assume it has something to do with the garbage collector not run yet and it's still finding the old key? With this I don't hit that anymore. Please let me know if I'm doing something wrong with the sequence. > > - Eric > >> --- >> include/linux/key.h | 1 + >> security/keys/key.c | 35 +++++++++++++++++++++++++++++------ >> 2 files changed, 30 insertions(+), 6 deletions(-) >> >> diff --git a/include/linux/key.h b/include/linux/key.h >> index e58ee10f6e58..4dffbd23d4e4 100644 >> --- a/include/linux/key.h >> +++ b/include/linux/key.h >> @@ -253,6 +253,7 @@ extern struct key *key_alloc(struct key_type *type, >> extern void key_revoke(struct key *key); >> extern void key_invalidate(struct key *key); >> extern void key_put(struct key *key); >> +extern void key_put_sync(struct key *key); >> >> static inline struct key *__key_get(struct key *key) >> { >> diff --git a/security/keys/key.c b/security/keys/key.c >> index d97c9394b5dd..b6f3ec19ab0f 100644 >> --- a/security/keys/key.c >> +++ b/security/keys/key.c >> @@ -622,6 +622,19 @@ int key_reject_and_link(struct key *key, >> } >> EXPORT_SYMBOL(key_reject_and_link); >> >> +static void __key_put(struct key *key, bool sync) >> +{ >> + if (key) { >> + key_check(key); >> + >> + if (refcount_dec_and_test(&key->usage)) { >> + schedule_work(&key_gc_work); >> + if (sync) >> + flush_work(&key_gc_work); >> + } >> + } >> +} >> + >> /** >> * key_put - Discard a reference to a key. >> * @key: The key to discard a reference from. >> @@ -632,15 +645,25 @@ EXPORT_SYMBOL(key_reject_and_link); >> */ >> void key_put(struct key *key) >> { >> - if (key) { >> - key_check(key); >> - >> - if (refcount_dec_and_test(&key->usage)) >> - schedule_work(&key_gc_work); >> - } >> + __key_put(key, false); >> } >> EXPORT_SYMBOL(key_put); >> >> +/** >> + * key_put - Discard a reference to a key and flush the key gc work item. >> + * @key: The key to discard a reference from. >> + * >> + * Discard a reference to a key, and when all the references are gone, we >> + * schedule the cleanup task to come and pull it out of the tree in process >> + * context at some later time. This call flushes the clean up so we are sure >> + * that the key is gone. >> + */ >> +void key_put_sync(struct key *key) >> +{ >> + __key_put(key, true); >> +} >> +EXPORT_SYMBOL(key_put_sync); >> + >> /* >> * Find a key by its serial number. >> */ >> >> -- >> To unsubscribe from this list: send the line "unsubscribe keyrings" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html
Dave Jiang <dave.jiang@intel.com> wrote: > +static void __key_put(struct key *key, bool sync) > +{ > + if (key) { > + key_check(key); > + > + if (refcount_dec_and_test(&key->usage)) { > + schedule_work(&key_gc_work); > + if (sync) > + flush_work(&key_gc_work); > + } > + } > +} I'm not sure this is a good way to go about it. This assumes that the gc will do an entire pass before returning to the dispatcher, but that won't necessarily always be the case. Further, it might take two passes to get rid of a key that has expired, been revoked or been invalidated: the first pass removes the links and then a second pass might be needed to remove the key, depending on the serial number of the key relative to the serial numbers of the keyrings linking to it. RCU synchronisation is then performed before the key can actually be cleaned up. A better way might be to add a global waitqueue and then use that to watch for an indication that a key is about to be finally destroyed. A flag can be set on the key to ask for the waitqueue to be poked David.
David Howells <dhowells@redhat.com> wrote: > A better way might be to add a global waitqueue and then use that to watch for > an indication that a key is about to be finally destroyed. A flag can be set > on the key to ask for the waitqueue to be poked Hmmm... This wouldn't work unless you added yourself to the waitqueue before decrementing the key usage count:-/ In fact, if someone creates a link to your key, sync'ing the gc isn't sufficient. David
diff --git a/include/linux/key.h b/include/linux/key.h index e58ee10f6e58..4dffbd23d4e4 100644 --- a/include/linux/key.h +++ b/include/linux/key.h @@ -253,6 +253,7 @@ extern struct key *key_alloc(struct key_type *type, extern void key_revoke(struct key *key); extern void key_invalidate(struct key *key); extern void key_put(struct key *key); +extern void key_put_sync(struct key *key); static inline struct key *__key_get(struct key *key) { diff --git a/security/keys/key.c b/security/keys/key.c index d97c9394b5dd..b6f3ec19ab0f 100644 --- a/security/keys/key.c +++ b/security/keys/key.c @@ -622,6 +622,19 @@ int key_reject_and_link(struct key *key, } EXPORT_SYMBOL(key_reject_and_link); +static void __key_put(struct key *key, bool sync) +{ + if (key) { + key_check(key); + + if (refcount_dec_and_test(&key->usage)) { + schedule_work(&key_gc_work); + if (sync) + flush_work(&key_gc_work); + } + } +} + /** * key_put - Discard a reference to a key. * @key: The key to discard a reference from. @@ -632,15 +645,25 @@ EXPORT_SYMBOL(key_reject_and_link); */ void key_put(struct key *key) { - if (key) { - key_check(key); - - if (refcount_dec_and_test(&key->usage)) - schedule_work(&key_gc_work); - } + __key_put(key, false); } EXPORT_SYMBOL(key_put); +/** + * key_put - Discard a reference to a key and flush the key gc work item. + * @key: The key to discard a reference from. + * + * Discard a reference to a key, and when all the references are gone, we + * schedule the cleanup task to come and pull it out of the tree in process + * context at some later time. This call flushes the clean up so we are sure + * that the key is gone. + */ +void key_put_sync(struct key *key) +{ + __key_put(key, true); +} +EXPORT_SYMBOL(key_put_sync); + /* * Find a key by its serial number. */
Adding key_put_sync() that calls flush_work on key_gc_work to ensure the key we want to remove is gone. Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- include/linux/key.h | 1 + security/keys/key.c | 35 +++++++++++++++++++++++++++++------ 2 files changed, 30 insertions(+), 6 deletions(-)