Re: [RFC PATCH] slow-work: add (module*)work->owner to fix races with module clients

[Gregory Haskins <ghaskins@novell.com>]

[-- Attachment #1: Type: text/plain, Size: 5247 bytes --]

Gregory Haskins wrote:
> (Applies to Linus' git master:626f380d)
>
> Hi All,
>   I found this while working on KVM.  I actually posted this patch with
> a KVM
> series yesterday and standalone earlier today, but neither seems to have
> made it to the lists.  I suspect there is an issue with git-mail/postfix
> on my system.
>
> I digress. This is a repost with the patch by itself, and rebased to
> Linus' tree instead of kvm.git.  Apologies if the system finally
> corrects itself and the others show up.
>
> Thoughts?
>
> Regards,
> -Greg
>
> -----------------------------
>
> slow-work: add (module*)work->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 transparently mapping "struct
> module* owner" to the slow_work 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@novell.com>
> CC: David Howells <dhowells@redhat.com>
> ---
>
>  include/linux/slow-work.h |    4 ++++
>  kernel/slow-work.c        |    6 ++++++
>  2 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/slow-work.h b/include/linux/slow-work.h
> index b65c888..9f48dab 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;
>  
> @@ -42,6 +43,7 @@ struct slow_work_ops {
>   *   queued
>   */
>  struct slow_work {
> +    struct module          *owner;
>      unsigned long        flags;
>  #define SLOW_WORK_PENDING    0    /* item pending (further) execution */
>  #define SLOW_WORK_EXECUTING    1    /* item currently executing */
> @@ -61,6 +63,7 @@ struct slow_work {
>  static inline void slow_work_init(struct slow_work *work,
>                    const struct slow_work_ops *ops)
>  {
> +    work->owner = THIS_MODULE;
>      work->flags = 0;
>      work->ops = ops;
>      INIT_LIST_HEAD(&work->link);
> @@ -78,6 +81,7 @@ static inline void slow_work_init(struct slow_work *work,
>  static inline void vslow_work_init(struct slow_work *work,
>                     const struct slow_work_ops *ops)
>  {
> +    work->owner = THIS_MODULE;
>      work->flags = 1 << SLOW_WORK_VERY_SLOW;
>      work->ops = ops;
>      INIT_LIST_HEAD(&work->link);
> diff --git a/kernel/slow-work.c b/kernel/slow-work.c
> index 09d7519..1dc3486 100644
> --- a/kernel/slow-work.c
> +++ b/kernel/slow-work.c
> @@ -220,6 +220,8 @@ static bool slow_work_execute(void)
>      }
>  
>      work->ops->put_ref(work);
>   

On this front: I also wonder if this put_ref is racing since we cannot
guarantee pointer stability if
the object is kfree'd as a result of dropping the last ref.  I do not
know enough about compilers to say whether work or work->ops
invalidation would cause problems with the call-return, but it seems
dangerous at best.  An alternative might be to copy the put_ref pointer
prior to the call.  Something like

    slowwork_putref_t put_ref = work->ops->put_ref;
    ....
    put_ref(work);

might be better.  However, I am not sure if it really matters so I did
not address this issue yet.

-Greg


> +    barrier(); /* ensure that put_ref is not re-ordered with module_put */
> +    module_put(work->owner);
>      return true;
>  
>  auto_requeue:
> @@ -299,6 +301,8 @@ 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 {
> +            if (!try_module_get(work->owner))
> +                goto cant_get_mod;
>              if (work->ops->get_ref(work) < 0)
>                  goto cant_get_ref;
>              if (test_bit(SLOW_WORK_VERY_SLOW, &work->flags))
> @@ -313,6 +317,8 @@ int slow_work_enqueue(struct slow_work *work)
>      return 0;
>  
>  cant_get_ref:
> +    module_put(work->owner);
> +cant_get_mod:
>      spin_unlock_irqrestore(&slow_work_queue_lock, flags);
>      return -EAGAIN;
>  }
>
>
>   



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 266 bytes --]