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] [day] [month] [year] [list]

Date:	Tue, 13 Mar 2012 18:16:38 +0800
From:	Cong Wang <xiyou.wangcong@...il.com>
To:	Eric Dumazet <eric.dumazet@...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

On 03/12/2012 01:38 AM, Eric Dumazet wrote:
> 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...

Oops, sorry I missed this part...

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

Fair enough, Rusty gave me the same suggestion, I will split this patch.

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