linux-kernel - Re: [PATCH 01/18] led-triggers: create a trigger for CPU activity

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: <CAK5ve-JFPEqkECrEu28XHuegnEZ9Ao0vhGqxYpz0_2YJkAmuiA@mail.gmail.com>
Date:	Fri, 20 Apr 2012 15:26:13 +0800
From:	Bryan Wu <bryan.wu@...onical.com>
To:	Stephen Boyd <sboyd@...eaurora.org>
Cc:	linux@....linux.org.uk, rpurdie@...ys.net,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linus.walleij@...aro.org, akpm@...ux-foundation.org,
	arnd.bergmann@...aro.org, nicolas.pitre@...aro.org,
	tim.gardner@...onical.com
Subject: Re: [PATCH 01/18] led-triggers: create a trigger for CPU activity

On Fri, Apr 20, 2012 at 2:59 PM, Stephen Boyd <sboyd@...eaurora.org> wrote:
> On 4/17/2012 3:53 AM, Bryan Wu wrote:
>> +static int __init ledtrig_cpu_init(void)
>> +{
>> +     int cpu;
>> +
>> +     /* Supports up to 9999 cpu cores */
>> +     BUILD_BUG_ON(CONFIG_NR_CPUS > 9999);
>> +
>> +     /*
>> +      * Registering CPU led trigger for each CPU cores here
>> +      * ignores CPU hotplug, but after this CPU hotplug works
>> +      * fine with this trigger.
>> +      */
>> +     for_each_possible_cpu(cpu) {
>> +             struct led_trigger *trig;
>> +             char *name = per_cpu(trig_name, cpu);
>> +             struct rw_semaphore *lock = &per_cpu(trig_lock, cpu);
>> +
>> +             init_rwsem(lock);
>
> What is this lock protecting?
>
>> +
>> +             snprintf(name, MAX_NAME_LEN, "cpu%d", cpu);
>> +
>> +             down_write(lock);
>> +             led_trigger_register_simple(name, &trig);
>> +             per_cpu(cpu_trig, cpu) = trig;
>> +             up_write(lock);
>
> This is the only place it is locked and unlocked.
>
> I think the previous comment was about how led_trigger_register_simple()
> exposed a live trigger but it wasn't assigned to the per_cpu variable
> yet. Can we not just reorder the assignment so we have
>
>  struct led_trigger *trig = NULL;
>  ...
>  per_cpu(cpu_trig, cpu) = trig;
>  led_trigger_register_simple();
>

I thinked about reordering like you posted here before, but
led_trigger_register_simple(const char *name, struct led_trigger **tp)
will modify the value of pointer trig here, because it use **tp to do
that.

So a lock here just protects the critical region, looks like safer to
kill race condition pointed out by Tim.

Thanks,
-- 
Bryan Wu <bryan.wu@...onical.com>
Kernel Developer    +86.186-168-78255 Mobile
Canonical Ltd.      www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com
--
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/