linux-kernel - Re: [V2 PATCH 1/3] module: use rcu to protect module list read

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <1331487502.2449.20.camel@edumazet-laptop>
Date:	Sun, 11 Mar 2012 10:38:22 -0700
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Cong Wang <xiyou.wangcong@...il.com>
Cc:	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Rusty Russell <rusty@...tcorp.com.au>
Subject: Re: [V2 PATCH 1/3] module: use rcu to protect module list read

Le dimanche 11 mars 2012 à 18:54 +0800, Cong Wang a écrit :
> V2:
...
>  
> @@ -1639,7 +1639,7 @@ static void mod_sysfs_teardown(struct module *mod)
>  static int __unlink_module(void *_mod)
>  {
>  	struct module *mod = _mod;
> -	list_del(&mod->list);
> +	list_del_rcu(&mod->list);
>  	module_bug_cleanup(mod);
>  	return 0;
>  }
> @@

You mix too many different things in a single patch. 

For example, lets review this __unlink_module() change...

If this was really needed, it should be a single patch so that it can be
a stable submission.

As it is not needed (since its called from stop_machine()), this makes
your whole patch looking suspicious.

I suggest you make a 100% cleanup patch, changing the title as well,
because module code _already_ uses RCU.

"module: use rcu to protect module list read" makes no sense at all.

To meet current RCU API best pratices, you change preempt_enable() by
rcu_read_unlock_sched() and preempt_disable() by rcu_read_lock_sched()

Then, if you believe other changes are needed, submit them with an
explicit changelog explaining the change, not hiding them in a big
cleanup patch.

Ref: Documentation/SubmittingPatches section 3)

--
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/