| Message ID | 20200121172829.15152-1-nramas@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IMA: Use delayed work to free queued keys | expand |
Hi Lackshmi, In the subject line and the patch description please refer to the construct as "delayed workqueue" or even abbreviated as "delayed wq", even though the struct itself is named "delayed_work. On Tue, 2020-01-21 at 09:28 -0800, Lakshmi Ramasubramanian wrote: > A timer is used to free queued keys if a custom IMA policy is not ^to free the queued keys, > loaded within 5 minutes after IMA subsystem is initialized. Timer ^after the IMA subsystem ^The timer handler > handler is called in interrupt context. Due to this a spinlock has > to be used to synchronize access to critical section. A mutex cannot > be used since a mutex can sleep. > > This patch uses a delayed work to free queued keys. Since a delayed > work handler is called in process context a mutex can be used. > > Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> > Fixes: 8f5d2d06f217 ("IMA: Defined timer to free queued keys") Thank you. This is definitely better. Mimi > --- > security/integrity/ima/ima_asymmetric_keys.c | 33 ++++++++++---------- > 1 file changed, 16 insertions(+), 17 deletions(-) > > diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c > index 381f51708e7b..fa1bdd54a9ff 100644 > --- a/security/integrity/ima/ima_asymmetric_keys.c > +++ b/security/integrity/ima/ima_asymmetric_keys.c > @@ -11,7 +11,7 @@ > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > -#include <linux/timer.h> > +#include <linux/workqueue.h> > #include <keys/asymmetric-type.h> > #include "ima.h" > > @@ -24,37 +24,37 @@ static bool ima_process_keys; > /* > * To synchronize access to the list of keys that need to be measured > */ > -static DEFINE_SPINLOCK(ima_keys_lock); > +static DEFINE_MUTEX(ima_keys_lock); > static LIST_HEAD(ima_keys); > > /* > * If custom IMA policy is not loaded then keys queued up > - * for measurement should be freed. This timer is used > + * for measurement should be freed. This worker is used > * for handling this scenario. > */ > static long ima_key_queue_timeout = 300000; /* 5 Minutes */ > -static struct timer_list ima_key_queue_timer; > +static void ima_keys_handler(struct work_struct *work); > +static DECLARE_DELAYED_WORK(ima_keys_delayed_work, ima_keys_handler); > static bool timer_expired; > > /* > - * This timer callback function frees keys that may still be > + * This worker function frees keys that may still be > * queued up in case custom IMA policy was not loaded. > */ > -static void ima_timer_handler(struct timer_list *timer) > +static void ima_keys_handler(struct work_struct *work) > { > timer_expired = true; > ima_process_queued_keys(); > } > > /* > - * This function sets up a timer to free queued keys in case > + * This function sets up a worker to free queued keys in case > * custom IMA policy was never loaded. > */ > void ima_init_key_queue(void) > { > - timer_setup(&ima_key_queue_timer, ima_timer_handler, 0); > - mod_timer(&ima_key_queue_timer, > - jiffies + msecs_to_jiffies(ima_key_queue_timeout)); > + schedule_delayed_work(&ima_keys_delayed_work, > + msecs_to_jiffies(ima_key_queue_timeout)); > } > > static void ima_free_key_entry(struct ima_key_entry *entry) > @@ -103,18 +103,17 @@ static bool ima_queue_key(struct key *keyring, const void *payload, > { > bool queued = false; > struct ima_key_entry *entry; > - unsigned long flags; > > entry = ima_alloc_key_entry(keyring, payload, payload_len); > if (!entry) > return false; > > - spin_lock_irqsave(&ima_keys_lock, flags); > + mutex_lock(&ima_keys_lock); > if (!ima_process_keys) { > list_add_tail(&entry->list, &ima_keys); > queued = true; > } > - spin_unlock_irqrestore(&ima_keys_lock, flags); > + mutex_unlock(&ima_keys_lock); > > if (!queued) > ima_free_key_entry(entry); > @@ -132,7 +131,6 @@ void ima_process_queued_keys(void) > { > struct ima_key_entry *entry, *tmp; > bool process = false; > - unsigned long flags; > > if (ima_process_keys) > return; > @@ -143,17 +141,18 @@ void ima_process_queued_keys(void) > * First one setting the ima_process_keys flag to true will > * process the queued keys. > */ > - spin_lock_irqsave(&ima_keys_lock, flags); > + mutex_lock(&ima_keys_lock); > if (!ima_process_keys) { > ima_process_keys = true; > process = true; > } > - spin_unlock_irqrestore(&ima_keys_lock, flags); > + mutex_unlock(&ima_keys_lock); > > if (!process) > return; > > - del_timer(&ima_key_queue_timer); > + if (!timer_expired) > + cancel_delayed_work_sync(&ima_keys_delayed_work); > > list_for_each_entry_safe(entry, tmp, &ima_keys, list) { > if (!timer_expired)
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c index 381f51708e7b..fa1bdd54a9ff 100644 --- a/security/integrity/ima/ima_asymmetric_keys.c +++ b/security/integrity/ima/ima_asymmetric_keys.c @@ -11,7 +11,7 @@ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt -#include <linux/timer.h> +#include <linux/workqueue.h> #include <keys/asymmetric-type.h> #include "ima.h" @@ -24,37 +24,37 @@ static bool ima_process_keys; /* * To synchronize access to the list of keys that need to be measured */ -static DEFINE_SPINLOCK(ima_keys_lock); +static DEFINE_MUTEX(ima_keys_lock); static LIST_HEAD(ima_keys); /* * If custom IMA policy is not loaded then keys queued up - * for measurement should be freed. This timer is used + * for measurement should be freed. This worker is used * for handling this scenario. */ static long ima_key_queue_timeout = 300000; /* 5 Minutes */ -static struct timer_list ima_key_queue_timer; +static void ima_keys_handler(struct work_struct *work); +static DECLARE_DELAYED_WORK(ima_keys_delayed_work, ima_keys_handler); static bool timer_expired; /* - * This timer callback function frees keys that may still be + * This worker function frees keys that may still be * queued up in case custom IMA policy was not loaded. */ -static void ima_timer_handler(struct timer_list *timer) +static void ima_keys_handler(struct work_struct *work) { timer_expired = true; ima_process_queued_keys(); } /* - * This function sets up a timer to free queued keys in case + * This function sets up a worker to free queued keys in case * custom IMA policy was never loaded. */ void ima_init_key_queue(void) { - timer_setup(&ima_key_queue_timer, ima_timer_handler, 0); - mod_timer(&ima_key_queue_timer, - jiffies + msecs_to_jiffies(ima_key_queue_timeout)); + schedule_delayed_work(&ima_keys_delayed_work, + msecs_to_jiffies(ima_key_queue_timeout)); } static void ima_free_key_entry(struct ima_key_entry *entry) @@ -103,18 +103,17 @@ static bool ima_queue_key(struct key *keyring, const void *payload, { bool queued = false; struct ima_key_entry *entry; - unsigned long flags; entry = ima_alloc_key_entry(keyring, payload, payload_len); if (!entry) return false; - spin_lock_irqsave(&ima_keys_lock, flags); + mutex_lock(&ima_keys_lock); if (!ima_process_keys) { list_add_tail(&entry->list, &ima_keys); queued = true; } - spin_unlock_irqrestore(&ima_keys_lock, flags); + mutex_unlock(&ima_keys_lock); if (!queued) ima_free_key_entry(entry); @@ -132,7 +131,6 @@ void ima_process_queued_keys(void) { struct ima_key_entry *entry, *tmp; bool process = false; - unsigned long flags; if (ima_process_keys) return; @@ -143,17 +141,18 @@ void ima_process_queued_keys(void) * First one setting the ima_process_keys flag to true will * process the queued keys. */ - spin_lock_irqsave(&ima_keys_lock, flags); + mutex_lock(&ima_keys_lock); if (!ima_process_keys) { ima_process_keys = true; process = true; } - spin_unlock_irqrestore(&ima_keys_lock, flags); + mutex_unlock(&ima_keys_lock); if (!process) return; - del_timer(&ima_key_queue_timer); + if (!timer_expired) + cancel_delayed_work_sync(&ima_keys_delayed_work); list_for_each_entry_safe(entry, tmp, &ima_keys, list) { if (!timer_expired)
A timer is used to free queued keys if a custom IMA policy is not loaded within 5 minutes after IMA subsystem is initialized. Timer handler is called in interrupt context. Due to this a spinlock has to be used to synchronize access to critical section. A mutex cannot be used since a mutex can sleep. This patch uses a delayed work to free queued keys. Since a delayed work handler is called in process context a mutex can be used. Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> Fixes: 8f5d2d06f217 ("IMA: Defined timer to free queued keys") --- security/integrity/ima/ima_asymmetric_keys.c | 33 ++++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-)