Peter Chen <peter.c...@freescale.com> writes:
> Currently, if module's refcount is not zero during the unload,
> it waits there until the user decreases that refcount.
Hi Peter,
In practice userspace uses O_NONBLOCK. In fact, I've been
thinking of removing the blocking case altogether, since it's not really
what people want.
That would solve your problem and make the code simpler. Thoughts?
Cheers,
Rusty.
> Assume
> we have two modules (A & B), there are no symbol relationship
> between each other. module B is module A's user, if the end
> user tries to unload module A first wrongly, it will stop there
> until there is another user process to unload B, and this
> process can't be killed.
> One use case is: the QA engineers do error test, they unload
> module wrongly on purpose, after that, they find the console
> is stopped there, and they can't do any thing go on.
>
> Signed-off-by: Peter Chen <peter.c...@freescale.com>
> ---
> include/linux/module.h | 4 +-
> kernel/module.c | 61 ++++++++++++++++++++++++++---------------------
> 2 files changed, 36 insertions(+), 29 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 05f2447..12edf07 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -367,8 +367,8 @@ struct module
> /* What modules do I depend on? */
> struct list_head target_list;
>
> - /* Who is waiting for us to be unloaded */
> - struct task_struct *waiter;
> + /* The work for waiting refcount to zero */
> + struct work_struct wait_refcount_work;
>
> /* Destruction function. */
> void (*exit)(void);
> diff --git a/kernel/module.c b/kernel/module.c
> index dc58274..94abc7e 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -61,6 +61,7 @@
> #include <linux/pfn.h>
> #include <linux/bsearch.h>
> #include <linux/fips.h>
> +#include <linux/delay.h>
> #include <uapi/linux/module.h>
> #include "module-internal.h"
>
> @@ -644,8 +645,6 @@ static int module_unload_init(struct module *mod)
>
> /* Hold reference count during initialization. */
> __this_cpu_write(mod->refptr->incs, 1);
> - /* Backwards compatibility macros put refcount during init. */
> - mod->waiter = current;
>
> return 0;
> }
> @@ -813,19 +812,38 @@ EXPORT_SYMBOL(module_refcount);
> /* This exists whether we can unload or not */
> static void free_module(struct module *mod);
>
> +/* Final destruction now no one is using it. */
> +static void module_final_free(struct module *mod)
> +{
> + if (mod->exit != NULL)
> + mod->exit();
> + blocking_notifier_call_chain(&module_notify_list,
> + MODULE_STATE_GOING, mod);
> + async_synchronize_full();
> +
> + /* Store the name of the last unloaded module for diagnostic purposes */
> + strlcpy(last_unloaded_module, mod->name, sizeof(last_unloaded_module));
> +
> + free_module(mod);
> +}
> +
> static void wait_for_zero_refcount(struct module *mod)
> {
> - /* Since we might sleep for some time, release the mutex first */
> - mutex_unlock(&module_mutex);
> for (;;) {
> pr_debug("Looking at refcount...\n");
> - set_current_state(TASK_UNINTERRUPTIBLE);
> if (module_refcount(mod) == 0)
> break;
> - schedule();
> + msleep(1000);
> }
> - current->state = TASK_RUNNING;
> - mutex_lock(&module_mutex);
> + module_final_free(mod);
> +}
> +
> +static void wait_module_refcount(struct work_struct *work)
> +{
> + struct module *mod = container_of(work,
> + struct module, wait_refcount_work);
> +
> + wait_for_zero_refcount(mod);
> }
>
> SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
> @@ -859,8 +877,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *,
> name_user,
>
> /* Doing init or already dying? */
> if (mod->state != MODULE_STATE_LIVE) {
> - /* FIXME: if (force), slam module count and wake up
> - waiter --RR */
> + /* FIXME: if (force), slam module count */
> pr_debug("%s already dying\n", mod->name);
> ret = -EBUSY;
> goto out;
> @@ -876,30 +893,23 @@ SYSCALL_DEFINE2(delete_module, const char __user *,
> name_user,
> }
> }
>
> - /* Set this up before setting mod->state */
> - mod->waiter = current;
> -
> /* Stop the machine so refcounts can't move and disable module. */
> ret = try_stop_module(mod, flags, &forced);
> if (ret != 0)
> goto out;
>
> /* Never wait if forced. */
> - if (!forced && module_refcount(mod) != 0)
> - wait_for_zero_refcount(mod);
> + if (!forced && module_refcount(mod) != 0) {
> + INIT_WORK(&mod->wait_refcount_work, wait_module_refcount);
> + schedule_work(&mod->wait_refcount_work);
> + ret = -EBUSY;
> + goto out;
> + }
>
> mutex_unlock(&module_mutex);
> - /* Final destruction now no one is using it. */
> - if (mod->exit != NULL)
> - mod->exit();
> - blocking_notifier_call_chain(&module_notify_list,
> - MODULE_STATE_GOING, mod);
> - async_synchronize_full();
>
> - /* Store the name of the last unloaded module for diagnostic purposes */
> - strlcpy(last_unloaded_module, mod->name, sizeof(last_unloaded_module));
> + module_final_free(mod);
>
> - free_module(mod);
> return 0;
> out:
> mutex_unlock(&module_mutex);
> @@ -1005,9 +1015,6 @@ void module_put(struct module *module)
> __this_cpu_inc(module->refptr->decs);
>
> trace_module_put(module, _RET_IP_);
> - /* Maybe they're waiting for us to drop reference? */
> - if (unlikely(!module_is_live(module)))
> - wake_up_process(module->waiter);
> preempt_enable();
> }
> }
> --
> 1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
