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

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

[Jacek Anaszewski]

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);
 }
-- 
1.9.1

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

[Hans de Goede]

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.

Regards,

Hans

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

[Jacek Anaszewski]

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().

-- 
Best regards,
Jacek Anaszewski

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

[Hans de Goede]

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.

Regards,

Hans

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

[Hans de Goede]

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.

Regards,

Hans

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

[Jacek Anaszewski]

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

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 4800 bytes --]

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.

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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 658 bytes --]

Hi!

> 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

The claim was probably wrong from the day one, unless brightness is of
type "atomic_t".

             /* Ensures consistent access to the LED Flash Class device */
	        struct mutex            led_access;
		};

This should really list fields that are protected by led_access.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2157 bytes --]

Hi!

> > 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)? 

led_timer_function() does not seem to have any locking. IMO it needs
some, as it does not use atomic accesses.

Do we need a spinlock protecting led_classdev.flags and
delayed_set_value?

Would it be good idea to create new "blink_cancel" so brightness_set
is used just for .. brightness and not for timer manipulations?

Should we do something like this for consistency?

BTW how is locking expected to work with userland LED drivers? What if
userland LED driver accesses /sys files for its own LED? I'd really
prefer that patch not to be merged until we get locking right.

Thanks,
								Pavel

diff --git a/include/linux/leds.h b/include/linux/leds.h
index ddfcb2d..60e436d 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -88,11 +88,11 @@ struct led_classdev {
 
 	unsigned long		 blink_delay_on, blink_delay_off;
 	struct timer_list	 blink_timer;
-	int			 blink_brightness;
+	enum led_brightness      blink_brightness;
 	void			(*flash_resume)(struct led_classdev *led_cdev);
 
 	struct work_struct	set_brightness_work;
-	int			delayed_set_value;
+	enum led_brightness     delayed_set_value;
 
 #ifdef CONFIG_LEDS_TRIGGERS
 	/* Protects the trigger data below */

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Jacek Anaszewski]

Hi,

On 11/09/2016 09:26 PM, Pavel Machek wrote:
> Hi!
>
>>> 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)?
>
> led_timer_function() does not seem to have any locking. IMO it needs
> some, as it does not use atomic accesses.

Locking in led_timer_function() wouldn't solve the problem as
there is led_set_brightness_nosleep() called in it which in turn
can schedule set_brightness_work. The problem is that we can't
hold a spinlock in set_brightness_delayed() as it can block
if LED class driver uses brightness_set_blocking op.

> Do we need a spinlock protecting led_classdev.flags and
> delayed_set_value?
>
> Would it be good idea to create new "blink_cancel" so brightness_set
> is used just for .. brightness and not for timer manipulations?
>
> Should we do something like this for consistency?

We would have to change the ABI I'm afraid.

> BTW how is locking expected to work with userland LED drivers? What if
> userland LED driver accesses /sys files for its own LED?

What do you mean by userland LED driver accessing sysfs files?

> I'd really
> prefer that patch not to be merged until we get locking right.


>
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index ddfcb2d..60e436d 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -88,11 +88,11 @@ struct led_classdev {
>
>  	unsigned long		 blink_delay_on, blink_delay_off;
>  	struct timer_list	 blink_timer;
> -	int			 blink_brightness;
> +	enum led_brightness      blink_brightness;
>  	void			(*flash_resume)(struct led_classdev *led_cdev);
>
>  	struct work_struct	set_brightness_work;
> -	int			delayed_set_value;
> +	enum led_brightness     delayed_set_value;

Good point. I'll keep it in mind.

>  #ifdef CONFIG_LEDS_TRIGGERS
>  	/* Protects the trigger data below */
>

-- 
Best regards,
Jacek Anaszewski

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

[Jacek Anaszewski]

Hi,

On 11/09/2016 01:38 PM, Pavel Machek wrote:
> Hi!
>
>> 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
>
> The claim was probably wrong from the day one, unless brightness is of
> type "atomic_t".
>
>              /* Ensures consistent access to the LED Flash Class device */
> 	        struct mutex            led_access;
> 		};
>
> This should really list fields that are protected by led_access.

Originally it was introduced to facilitate disabling LED subsystem
sysfs interface, i.e. it protects flags property of
struct led_classdev.

-- 
Best regards,
Jacek Anaszewski