linux-kernel - RE: [PATCH 1/2] leds: simply LED trigger list management

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: <A874F61F95741C4A9BA573A70FE3998F69E19873@DQHE02.ent.ti.com>
Date:	Fri, 18 Jan 2013 00:00:38 +0000
From:	"Kim, Milo" <Milo.Kim@...com>
To:	Nathan Lynch <ntl@...ox.com>
CC:	Bryan Wu <cooloney@...il.com>,
	"linux-leds@...r.kernel.org" <linux-leds@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 1/2] leds: simply LED trigger list management

> -----Original Message-----
> From: Nathan Lynch [mailto:ntl@...ox.com]
> 
> On Thu, 2013-01-17 at 01:06 +0000, Kim, Milo wrote:
> > @@ -242,17 +233,15 @@ EXPORT_SYMBOL_GPL(led_trigger_unregister);
> >  void led_trigger_event(struct led_trigger *trig,
> >  			enum led_brightness brightness)
> >  {
> > -	struct list_head *entry;
> > +	struct led_classdev *led_cdev;
> >
> >  	if (!trig)
> >  		return;
> >
> >  	read_lock(&trig->leddev_list_lock);
> > -	list_for_each(entry, &trig->led_cdevs) {
> > -		struct led_classdev *led_cdev;
> > -
> > -		led_cdev = list_entry(entry, struct led_classdev,
> trig_list);
> > -		led_set_brightness(led_cdev, brightness);
> > +	list_for_each_entry(led_cdev, &leds_list, node) {
> > +		if (led_cdev->trigger == trig)
> > +			led_set_brightness(led_cdev, brightness);
> >  	}
> >  	read_unlock(&trig->leddev_list_lock);
> 
> Continuing to use trig->leddev_list_lock doesn't seem right.  Shouldn't
> traversal of leds_list be guarded by the leds_list_lock rwsem?  And if
> so, is it safe to use a potentially-blocking lock in this context?
> 

(Sorry for the typo in title: 'simply' -> 'simplify')

Thanks for your opinion. I agree with you.
The read_lock()/unlock() of 'leddev_list_lock' should be replaced with
down_read()/up_read() of 'leds_list_lock'.
Then, RW lock of led_trigger can be removed also.

BTW, I need more education about the concurrency.
We can see complex RW down/up safe code with list management in LED class driver.
RW semaphores are 'leds_list_lock', 'triggers_list_lock' and 'trigger_lock'.
Are those are safe access in case a user-space via sysfs and driver API calls by
 LED device(s) can happen at the same time?
If so, can we make them more simple?

I think *entry point* of access can be wrapped with semaphores or MUTEX
rather than guard code for accessing LED lists one by one.

I would like to have your opinion and others' too.

Best Regards,
Milo