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 <[email protected]>
Cc: Hans de Goede <[email protected]>
Cc: Sakari Ailus <[email protected]>
Cc: Pavel Machek <[email protected]>
Cc: Andrew Lunn <[email protected]>
---
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
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 <[email protected]>
> Cc: Hans de Goede <[email protected]>
> Cc: Sakari Ailus <[email protected]>
> Cc: Pavel Machek <[email protected]>
> Cc: Andrew Lunn <[email protected]>
> ---
> 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
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 <[email protected]>
>> Cc: Hans de Goede <[email protected]>
>> Cc: Sakari Ailus <[email protected]>
>> Cc: Pavel Machek <[email protected]>
>> Cc: Andrew Lunn <[email protected]>
>> ---
>> 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
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 <[email protected]>
>>> Cc: Hans de Goede <[email protected]>
>>> Cc: Sakari Ailus <[email protected]>
>>> Cc: Pavel Machek <[email protected]>
>>> Cc: Andrew Lunn <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
>>>> Cc: Hans de Goede <[email protected]>
>>>> Cc: Sakari Ailus <[email protected]>
>>>> Cc: Pavel Machek <[email protected]>
>>>> Cc: Andrew Lunn <[email protected]>
>>>> ---
>>>> 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
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 <[email protected]>
>>>>> Cc: Hans de Goede <[email protected]>
>>>>> Cc: Sakari Ailus <[email protected]>
>>>>> Cc: Pavel Machek <[email protected]>
>>>>> Cc: Andrew Lunn <[email protected]>
>>>>> ---
>>>>> 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
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 <[email protected]>
> >>>>>Cc: Hans de Goede <[email protected]>
> >>>>>Cc: Sakari Ailus <[email protected]>
> >>>>>Cc: Pavel Machek <[email protected]>
> >>>>>Cc: Andrew Lunn <[email protected]>
> >>>>>---
> >>>>> 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
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
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
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
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