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

[Jacek Anaszewski <j.anaszewski@samsung.com>]

Hi,

On 11/06/2016 03:52 PM, Hans de Goede wrote:
> Hi,
>
> On 04-11-16 17:46, Hans de Goede wrote:
>> Hi,
>>
>> On 04-11-16 17:06, Jacek Anaszewski wrote:
>>> Hi Hans,
>>>
>>> On 11/04/2016 12:53 PM, Hans de Goede wrote:
>>>> 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@samsung.com>
>>>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>>>> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>>> Cc: Pavel Machek <pavel@ucw.cz>
>>>>> Cc: Andrew Lunn <andrew@lunn.ch>
>>>>> ---
>>>>>  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.

-- 
Best regards,
Jacek Anaszewski