netdev - Re: [PATCH v3 7/8] cgroup: Assign subsystem IDs during compile time

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: <504FA9D6.4050201@monom.org>
Date:	Tue, 11 Sep 2012 23:15:02 +0200
From:	Daniel Wagner <wagi@...om.org>
To:	Tejun Heo <tj@...nel.org>
CC:	netdev@...r.kernel.org, cgroups@...r.kernel.org,
	Daniel Wagner <daniel.wagner@...-carit.de>,
	"David S. Miller" <davem@...emloft.net>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Eric Dumazet <edumazet@...gle.com>,
	Gao feng <gaofeng@...fujitsu.com>,
	Glauber Costa <glommer@...allels.com>,
	Jamal Hadi Salim <jhs@...atatu.com>,
	John Fastabend <john.r.fastabend@...el.com>,
	Kamezawa Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Li Zefan <lizefan@...wei.com>,
	Neil Horman <nhorman@...driver.com>
Subject: Re: [PATCH v3 7/8] cgroup: Assign subsystem IDs during compile time

Hi Tejun,

On 09/11/2012 11:01 PM, Tejun Heo wrote:
> Hello, Daniel.
>
> I generally like this but I still think it's too big a patch.

Yes, I agree it is a bit too big.

>> diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
>> index c75e3f9..6bc460c 100644
>> --- a/net/core/netprio_cgroup.c
>> +++ b/net/core/netprio_cgroup.c
>> @@ -326,9 +326,7 @@ struct cgroup_subsys net_prio_subsys = {
>>   	.create		= cgrp_create,
>>   	.destroy	= cgrp_destroy,
>>   	.attach		= net_prio_attach,
>> -#ifdef CONFIG_NETPRIO_CGROUP
>>   	.subsys_id	= net_prio_subsys_id,
>> -#endif
>>   	.base_cftypes	= ss_files,
>>   	.module		= THIS_MODULE
>>   };
>> @@ -366,10 +364,6 @@ static int __init init_cgroup_netprio(void)
>>   	ret = cgroup_load_subsys(&net_prio_subsys);
>>   	if (ret)
>>   		goto out;
>> -#ifndef CONFIG_NETPRIO_CGROUP
>> -	smp_wmb();
>> -	net_prio_subsys_id = net_prio_subsys.subsys_id;
>> -#endif
>>
>>   	register_netdevice_notifier(&netprio_device_notifier);
>>
>> @@ -386,11 +380,6 @@ static void __exit exit_cgroup_netprio(void)
>>
>>   	cgroup_unload_subsys(&net_prio_subsys);
>>
>> -#ifndef CONFIG_NETPRIO_CGROUP
>> -	net_prio_subsys_id = -1;
>> -	synchronize_rcu();
>
> For example, it's not evident the above is correct and it's burried
> with all other changes.  Can't we just assign the fixed subsys ID to
> net_prio_subsys_id in this patch?  This patch would be correct without
> any netprio changes, no?

If net_prio_subsys_id is changed to be an enum, then the compiler will 
report an error:

error: lvalue required as left operand of assignment

that was the reason why I kept this change here. I think I just don't 
get what you are trying to tell me.

> Please separate these changes and explain them.

I will do that as soon I figured out what you are telling me.

> BTW, people who use barriers of any kind without explicitly explaining
> what's going on need to be kicked hard in the ass.  :(

I looked up the commit message when the synchronze_rcu() was added. It 
was not really explaining it. I really spend a few hours starring at 
this code.

thanks for the review,
daniel
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html