Re: [PATCH] leds: Add mutex protection in brightness_show()

[Pavel Machek]

Hi!

> >>>>On 04-11-16 08:52, Jacek Anaszewski wrote:
> >>>>>Initially the claim about no need for lock in brightness_show()
> >>>>>was valid as the function was just returning unchanged
> >>>>>LED brightness. After the addition of led_update_brightness() this
> >>>>>is no longer true, as the function can change the brightness if
> >>>>>a LED class driver implements brightness_get op. It can lead to
> >>>>>races between led_update_brightness() and led_set_brightness(),
> >>>>>resulting in overwriting new brightness with the old one before
> >>>>>the former is written to the device.
> >>>>>
> >>>>>Signed-off-by: Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx>
> >>>>>Cc: Hans de Goede <hdegoede@xxxxxxxxxx>
> >>>>>Cc: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> >>>>>Cc: Pavel Machek <pavel@xxxxxx>
> >>>>>Cc: Andrew Lunn <andrew@xxxxxxx>
> >>>>>---
> >>>>> drivers/leds/led-class.c | 3 ++-
> >>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>>
> >>>>>diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> >>>>>index 731e4eb..0c2307b 100644
> >>>>>--- a/drivers/leds/led-class.c
> >>>>>+++ b/drivers/leds/led-class.c
> >>>>>@@ -30,8 +30,9 @@ static ssize_t brightness_show(struct device *dev,
> >>>>> {
> >>>>>     struct led_classdev *led_cdev = dev_get_drvdata(dev);
> >>>>>
> >>>>>-    /* no lock needed for this */
> >>>>>+    mutex_lock(&led_cdev->led_access);
> >>>>>     led_update_brightness(led_cdev);
> >>>>>+    mutex_unlock(&led_cdev->led_access);
> >>>>>
> >>>>>     return sprintf(buf, "%u\n", led_cdev->brightness);
> >>>>> }
> >>>>>
> >>>>
> >>>>I'm afraid that this fix is not enough, the led_access lock is only
> >>>>held when the brightness is being updated through sysfs, not for
> >>>>trigger / sw-blinking updates (which cannot take a mutex as they
> >>>>may be called from non blocking contexts).
> >>>>
> >>>>We may need to consider to add a spinlock to the led_classdev and
> >>>>always lock that when calling into the driver, except for when
> >>>>the driver has a brightness_set_blocking callback. Which will need
> >>>>special handling.
> >>>
> >>>led_update_brightness() currently has two users besides LED subsystem
> >>>(at least grep reports those places):
> >>>
> >>>1. v4l2-flash-led-class wrapper, for which led_access mutex was
> >>>   introduced. Its purpose was to disable LED sysfs interface while
> >>>   v4l2-flash wrapper takes over control of LED class device
> >>>   (not saying that the mutex wasn't missing even without this
> >>>    use case). Now I've realized that the call to
> >>>    led_sysfs_is_disabled() is missing in this patch.
> >>>2. /drivers/platform/x86/thinkpad_acpi.c - it calls
> >>>   led_update_brightness() on suspend
> >>>
> >>>I think that the best we can now do is to add
> >>>lockdep_assert_held(&led_cdev->led_access) in led_update_brightness()
> >>>and a description saying that the caller has to acquire led_access
> >>>lock before calling it. Similarly as in case of
> >>>led_sysfs_disable()/led_sysfs_disable().
> >>
> >>The problem is not only callers of led_update_brightness() not holding
> >>led_cdev->led_access, the problem is also callers of led_set_brightness
> >>not holding led_cdev->led_access and they cannot take this lock because
> >>they may be called from a non-blocking context.
> >
> >Thinking more about this, using a spinlock is also not going to work
> >because led_cdev->brightness_set_blocking and led_cdev->brightness_get
> >can both block and thus cannot be called with a spinlock held.
> >
> >I think that we need to just make this a problem of the led drivers
> >and in include/linux/leds.h document that the led-core does not do
> >locking and that the drivers themselves need to protect against
> >their brightness_set / brightness_get callbacks when necessary.
> 
> Thanks for the analysis. Either way, this patch, with the modification
> I mentioned in my previous message is required to assure proper
> LED sysfs locking.
> 
> Regarding the races between user and atomic context, I think that
> it should be system root's responsibility to define LED access
> policy. If a LED is registered for any trigger events then setting
> brightness from user space should be made impossible. Such a hint
> could be even added to the Documentation/leds/leds-class.txt.

No, kernel locking may not depend on "root did not do anything
stupid". Sorry.

Is there problem with led_access becoming a spinlock, and
brightness_set_blocking taking it internally while it reads the
brightness (but not while sleeping)? 

Thanks,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachment: signature.asc
Description: Digital signature