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: <1334704072.616.152.camel@ted>
Date:	Wed, 18 Apr 2012 00:07:52 +0100
From:	Richard Purdie <richard.purdie@...uxfoundation.org>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Bryan Wu <bryan.wu@...onical.com>, linux@....linux.org.uk,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linus.walleij@...aro.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 Tue, 2012-04-17 at 15:52 -0700, Andrew Morton wrote:
> On Tue, 17 Apr 2012 18:53:28 +0800
> Bryan Wu <bryan.wu@...onical.com> wrote:
> > +	 * 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);
> > +
> > +		snprintf(name, MAX_NAME_LEN, "cpu%d", cpu);
> > +
> > +		down_write(lock);
> > +		led_trigger_register_simple(name, &trig);
> 
> OK, problem.
> 
> led_trigger_register_simple() calls kzalloc() and
> led_trigger_register(), both of which can fail. 
> led_trigger_register_simple() just returns void, failing to propagate
> the error back.  This is bad, and we (ie you ;)) should fix
> led_trigger_register_simple() before proceeding to use it.  If at all
> possible.  Please.  Let us not propagate the badness further.  Sorry.

FWIW, this was really the way led_trigger_register_simple() was designed
to work. It's original use was adding a trigger into other subsystems
which didn't want a ton of LED code so it had the simple form:

xxx = led_trigger_register_simple(name, &trig);

where xxx could then be unregistered later equally simply and safely in
one line. It didn't seem to make sense to pass the error around as it
didn't really matter to the code it was being used in.

I guess we could return an error pointer and check for that at
unregister time in led_trigger_unregister_simple().

> 
> > +		char *name = per_cpu(trig_name, cpu);
> > +
> > +		led_trigger_unregister_simple(trig);
> 
> And what happens if led_trigger_register_simple() had silently failed
> to register this trigger?  afacit, nothing: your code handles the
> trig==NULL case OK.  Still, we should be checking for those failures!

FWIW, led_trigger_unregister_simple() will deal with NULL safely.

Cheers,

Richard

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