| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4A491640.9070206@novell.com>
Date: Mon, 29 Jun 2009 15:30:08 -0400
From: Gregory Haskins <ghaskins@...ell.com>
To: linux-kernel@...r.kernel.org
CC: dhowells@...hat.com, mst@...hat.com, swhiteho@...hat.com
Subject: Re: [PATCH v4] slow-work: add (module*)work->ops->owner to fix races
with module clients
Gregory Haskins wrote:
> (Applies to Linus' linux-2.6.git/master:626f380d)
>
> [ Changelog:
>
> v4:
> *) added ".owner = THIS_MODULE" fields to all current slow-work
> clients (fscache, gfs2).
>
> v3:
> *) moved (module*)owner to slow_work_ops
> *) removed useless barrier()
> *) updated documentation/comments
>
> v2:
> *) cache "owner" value to prevent invalid access after put_ref
>
> v1:
> *) initial release
> ]
>
> I've retained Michael's "Reviewed-by:" tag from v3 since v4 is identical
> in every way except the new hunks added to gfs2/fscache that he asked for.
>
> Michael, if you want to recind your tag, please speak up.
>
Or rescind it even. What a dope I am. <sighs>
-Greg
> Otherwise, please consider for inclusion.
>
> Regards,
> -Greg
>
> ---------------------------------
>
> slow-work: add (module*)work->ops->owner to fix races with module clients
>
> The slow_work facility was designed to use reference counting instead of
> barriers for synchronization. The reference counting mechanism is
> implemented as a vtable op (->get_ref, ->put_ref) callback. This is
> problematic for module use of the slow_work facility because it is
> impossible to synchronize against the .text installed in the callbacks:
> There is no way to ensure that the slow-work threads have completely
> exited the .text in question and rmmod may yank it out from under the
> slow_work thread.
>
> This patch attempts to address this issue by mapping "struct module* owner"
> to the slow_work_ops item, and maintaining a module reference
> count coincident with the more externally visible reference count. Since
> the slow_work facility is resident in kernel, it should be a race-free
> location to issue a module_put() call. This will ensure that modules
> can properly cleanup before exiting.
>
> A module_get()/module_put() pair on slow_work_enqueue() and the subsequent
> dequeue technically adds the overhead of the atomic operations for every
> work item scheduled. However, slow_work is designed for deferring
> relatively long-running and/or sleepy tasks to begin with, so this
> overhead will hopefully be negligible.
>
> Signed-off-by: Gregory Haskins <ghaskins@...ell.com>
> Reviewed-by: Michael S. Tsirkin <mst@...hat.com>
> CC: David Howells <dhowells@...hat.com>
> CC: Steven Whitehouse <swhiteho@...hat.com>
> ---
>
> Documentation/slow-work.txt | 6 +++++-
> fs/fscache/object.c | 1 +
> fs/fscache/operation.c | 1 +
> fs/gfs2/recovery.c | 1 +
> include/linux/slow-work.h | 3 +++
> kernel/slow-work.c | 20 +++++++++++++++++++-
> 6 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/slow-work.txt b/Documentation/slow-work.txt
> index ebc50f8..2a38878 100644
> --- a/Documentation/slow-work.txt
> +++ b/Documentation/slow-work.txt
> @@ -80,6 +80,7 @@ Slow work items may then be set up by:
> (2) Declaring the operations to be used for this item:
>
> struct slow_work_ops myitem_ops = {
> + .owner = THIS_MODULE,
> .get_ref = myitem_get_ref,
> .put_ref = myitem_put_ref,
> .execute = myitem_execute,
> @@ -102,7 +103,10 @@ A suitably set up work item can then be enqueued for processing:
> int ret = slow_work_enqueue(&myitem);
>
> This will return a -ve error if the thread pool is unable to gain a reference
> -on the item, 0 otherwise.
> +on the item, 0 otherwise. Loadable modules may only enqueue work if at least
> +one reference to the module is known to be held. The slow-work infrastructure
> +will acquire a reference to the module and hold it until after the item's
> +reference is dropped, assuring the stability of the callback.
>
>
> The items are reference counted, so there ought to be no need for a flush
> diff --git a/fs/fscache/object.c b/fs/fscache/object.c
> index 392a41b..d236eb1 100644
> --- a/fs/fscache/object.c
> +++ b/fs/fscache/object.c
> @@ -45,6 +45,7 @@ static void fscache_enqueue_dependents(struct fscache_object *);
> static void fscache_dequeue_object(struct fscache_object *);
>
> const struct slow_work_ops fscache_object_slow_work_ops = {
> + .owner = THIS_MODULE,
> .get_ref = fscache_object_slow_work_get_ref,
> .put_ref = fscache_object_slow_work_put_ref,
> .execute = fscache_object_slow_work_execute,
> diff --git a/fs/fscache/operation.c b/fs/fscache/operation.c
> index e7f8d53..f1a2857 100644
> --- a/fs/fscache/operation.c
> +++ b/fs/fscache/operation.c
> @@ -453,6 +453,7 @@ static void fscache_op_execute(struct slow_work *work)
> }
>
> const struct slow_work_ops fscache_op_slow_work_ops = {
> + .owner = THIS_MODULE,
> .get_ref = fscache_op_get_ref,
> .put_ref = fscache_op_put_ref,
> .execute = fscache_op_execute,
> diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c
> index 59d2695..0c2a6aa 100644
> --- a/fs/gfs2/recovery.c
> +++ b/fs/gfs2/recovery.c
> @@ -593,6 +593,7 @@ fail:
> }
>
> struct slow_work_ops gfs2_recover_ops = {
> + .owner = THIS_MODULE,
> .get_ref = gfs2_recover_get_ref,
> .put_ref = gfs2_recover_put_ref,
> .execute = gfs2_recover_work,
> diff --git a/include/linux/slow-work.h b/include/linux/slow-work.h
> index b65c888..1382918 100644
> --- a/include/linux/slow-work.h
> +++ b/include/linux/slow-work.h
> @@ -17,6 +17,7 @@
> #ifdef CONFIG_SLOW_WORK
>
> #include <linux/sysctl.h>
> +#include <linux/module.h>
>
> struct slow_work;
>
> @@ -24,6 +25,8 @@ struct slow_work;
> * The operations used to support slow work items
> */
> struct slow_work_ops {
> + struct module *owner;
> +
> /* get a ref on a work item
> * - return 0 if successful, -ve if not
> */
> diff --git a/kernel/slow-work.c b/kernel/slow-work.c
> index 09d7519..18dee34 100644
> --- a/kernel/slow-work.c
> +++ b/kernel/slow-work.c
> @@ -145,6 +145,15 @@ static unsigned slow_work_calc_vsmax(void)
> return min(vsmax, slow_work_max_threads - 1);
> }
>
> +static void slow_work_put(struct slow_work *work)
> +{
> + /* cache values that are needed during/after pointer invalidation */
> + struct module *owner = work->ops->owner;
> +
> + work->ops->put_ref(work);
> + module_put(owner);
> +}
> +
> /*
> * Attempt to execute stuff queued on a slow thread. Return true if we managed
> * it, false if there was nothing to do.
> @@ -219,7 +228,7 @@ static bool slow_work_execute(void)
> spin_unlock_irq(&slow_work_queue_lock);
> }
>
> - work->ops->put_ref(work);
> + slow_work_put(work);
> return true;
>
> auto_requeue:
> @@ -299,6 +308,14 @@ int slow_work_enqueue(struct slow_work *work)
> if (test_bit(SLOW_WORK_EXECUTING, &work->flags)) {
> set_bit(SLOW_WORK_ENQ_DEFERRED, &work->flags);
> } else {
> + /*
> + * Callers must ensure that their module has at least
> + * one reference held while the work is enqueued. We
> + * will acquire another reference here and drop it
> + * once we do the last ops->put_ref()
> + */
> + __module_get(work->ops->owner);
> +
> if (work->ops->get_ref(work) < 0)
> goto cant_get_ref;
> if (test_bit(SLOW_WORK_VERY_SLOW, &work->flags))
> @@ -313,6 +330,7 @@ int slow_work_enqueue(struct slow_work *work)
> return 0;
>
> cant_get_ref:
> + module_put(work->ops->owner);
> spin_unlock_irqrestore(&slow_work_queue_lock, flags);
> return -EAGAIN;
> }
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
Download attachment "signature.asc" of type "application/pgp-signature" (267 bytes)
Powered by blists - more mailing lists