linux-kernel - Re: [PATCH] module: add kset_obj

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87vc7o8r7o.fsf@rustcorp.com.au>
Date:	Mon, 15 Apr 2013 11:56:03 +0930
From:	Rusty Russell <rusty@...tcorp.com.au>
To:	Veaceslav Falico <vfalico@...hat.com>
Cc:	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
	gregkh@...uxfoundation.org, bhelgaas@...gle.com
Subject: Re: [PATCH] module: add kset_obj_exists() and use it

Veaceslav Falico <vfalico@...hat.com> writes:
> On Wed, Apr 10, 2013 at 04:47:34PM +0930, Rusty Russell wrote:
>>That's a bug.  We should be cleaning up sysfs before we unlike the
>>removed module from the list.
>>
>>Because the same thing applies to ddebug info, which is also keyed by
>>module name.
>>
>>Something like this (untested!):
>
> Sorry for the late response - I wanted to test it for a longer time.
>
> Your patch works flawlessly and fixes this race, with just a small
> addition, cause otherwise we could BUG() in show_initstate().
>
> Can you apply this patch or should I (re-)send it somehow?
>
> Thank you!
>
> diff --git a/kernel/module.c b/kernel/module.c
> index d0afe23..8be6e97 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1063,6 +1063,9 @@ static ssize_t show_initstate(struct module_attribute *mattr,
>   	case MODULE_STATE_GOING:
>   		state = "going";
>   		break;
> +	case MODULE_STATE_UNFORMED:
> +		state = "unformed";
> +		break;
>   	default:
>   		BUG();
>   	}

Prefer to remove from sysfs before marking it unformed, like so:

diff --git a/kernel/module.c b/kernel/module.c
index 2468fda..2e7189f 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1862,12 +1862,12 @@ static void free_module(struct module *mod)
 {
 	trace_module_free(mod);
 
-	/* We leave it in list to prevent duplicate loads while we clean
-	 * up sysfs, ddebug and any other external exposure. */
-	mod->state = MODULE_STATE_UNFORMED;
-
 	mod_sysfs_teardown(mod);
 
+	/* We leave it in list to prevent duplicate loads, but make sure
+	 * that noone uses it while it's being deconstructed. */
+	mod->state = MODULE_STATE_UNFORMED;
+
 	/* Remove dynamic debug info */
 	ddebug_remove_module(mod->name);
 
Here's the updated total patch:

diff --git a/kernel/module.c b/kernel/module.c
index 69d2600..2e7189f 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1862,12 +1862,12 @@ static void free_module(struct module *mod)
 {
 	trace_module_free(mod);
 
-	/* Delete from various lists */
-	mutex_lock(&module_mutex);
-	stop_machine(__unlink_module, mod, NULL);
-	mutex_unlock(&module_mutex);
 	mod_sysfs_teardown(mod);
 
+	/* We leave it in list to prevent duplicate loads, but make sure
+	 * that noone uses it while it's being deconstructed. */
+	mod->state = MODULE_STATE_UNFORMED;
+
 	/* Remove dynamic debug info */
 	ddebug_remove_module(mod->name);
 
@@ -1880,6 +1880,11 @@ static void free_module(struct module *mod)
 	/* Free any allocated parameters. */
 	destroy_params(mod->kp, mod->num_kp);
 
+	/* Now we can delete it from the lists */
+	mutex_lock(&module_mutex);
+	stop_machine(__unlink_module, mod, NULL);
+	mutex_unlock(&module_mutex);
+
 	/* This may be NULL, but that's OK */
 	unset_module_init_ro_nx(mod);
 	module_free(mod, mod->module_init);

Thanks,
Rusty.
--
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/