* [PATCH] leds: Add mutex protection in brightness_show() @ 2016-11-04 7:52 Jacek Anaszewski 2016-11-04 11:53 ` Hans de Goede 2016-11-09 12:38 ` Pavel Machek 0 siblings, 2 replies; 11+ messages in thread From: Jacek Anaszewski @ 2016-11-04 7:52 UTC (permalink / raw) To: linux-leds Cc: linux-kernel, Jacek Anaszewski, Hans de Goede, Sakari Ailus, Pavel Machek, Andrew Lunn 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] leds: Add mutex protection in brightness_show() 2016-11-04 7:52 [PATCH] leds: Add mutex protection in brightness_show() Jacek Anaszewski @ 2016-11-04 11:53 ` Hans de Goede 2016-11-04 16:06 ` Jacek Anaszewski 2016-11-09 12:38 ` Pavel Machek 1 sibling, 1 reply; 11+ messages in thread From: Hans de Goede @ 2016-11-04 11:53 UTC (permalink / raw) To: Jacek Anaszewski, linux-leds Cc: linux-kernel, Sakari Ailus, Pavel Machek, Andrew Lunn 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] leds: Add mutex protection in brightness_show() 2016-11-04 11:53 ` Hans de Goede @ 2016-11-04 16:06 ` Jacek Anaszewski 2016-11-04 16:46 ` Hans de Goede 0 siblings, 1 reply; 11+ messages in thread From: Jacek Anaszewski @ 2016-11-04 16:06 UTC (permalink / raw) To: Hans de Goede, linux-leds Cc: linux-kernel, Sakari Ailus, Pavel Machek, Andrew Lunn 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] leds: Add mutex protection in brightness_show() 2016-11-04 16:06 ` Jacek Anaszewski @ 2016-11-04 16:46 ` Hans de Goede 2016-11-06 14:52 ` Hans de Goede 0 siblings, 1 reply; 11+ messages in thread From: Hans de Goede @ 2016-11-04 16:46 UTC (permalink / raw) To: Jacek Anaszewski, linux-leds Cc: linux-kernel, Sakari Ailus, Pavel Machek, Andrew Lunn 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] leds: Add mutex protection in brightness_show() 2016-11-04 16:46 ` Hans de Goede @ 2016-11-06 14:52 ` Hans de Goede 2016-11-07 9:11 ` Jacek Anaszewski 0 siblings, 1 reply; 11+ messages in thread From: Hans de Goede @ 2016-11-06 14:52 UTC (permalink / raw) To: Jacek Anaszewski, linux-leds Cc: linux-kernel, Sakari Ailus, Pavel Machek, Andrew Lunn 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] leds: Add mutex protection in brightness_show() 2016-11-06 14:52 ` Hans de Goede @ 2016-11-07 9:11 ` Jacek Anaszewski 2016-11-09 12:37 ` Pavel Machek 0 siblings, 1 reply; 11+ messages in thread From: Jacek Anaszewski @ 2016-11-07 9:11 UTC (permalink / raw) To: Hans de Goede, linux-leds Cc: linux-kernel, Sakari Ailus, Pavel Machek, Andrew Lunn 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] leds: Add mutex protection in brightness_show() 2016-11-07 9:11 ` Jacek Anaszewski @ 2016-11-09 12:37 ` Pavel Machek 2016-11-09 20:26 ` Pavel Machek 0 siblings, 1 reply; 11+ messages in thread From: Pavel Machek @ 2016-11-09 12:37 UTC (permalink / raw) To: Jacek Anaszewski Cc: Hans de Goede, linux-leds, linux-kernel, Sakari Ailus, Andrew Lunn [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] leds: Add mutex protection in brightness_show() 2016-11-09 12:37 ` Pavel Machek @ 2016-11-09 20:26 ` Pavel Machek 2016-11-09 21:23 ` Jacek Anaszewski 0 siblings, 1 reply; 11+ messages in thread From: Pavel Machek @ 2016-11-09 20:26 UTC (permalink / raw) To: Jacek Anaszewski Cc: Hans de Goede, linux-leds, linux-kernel, Sakari Ailus, Andrew Lunn [-- 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 --] ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] leds: Add mutex protection in brightness_show() 2016-11-09 20:26 ` Pavel Machek @ 2016-11-09 21:23 ` Jacek Anaszewski 0 siblings, 0 replies; 11+ messages in thread From: Jacek Anaszewski @ 2016-11-09 21:23 UTC (permalink / raw) To: Pavel Machek, Jacek Anaszewski Cc: Hans de Goede, linux-leds, linux-kernel, Sakari Ailus, Andrew Lunn 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] leds: Add mutex protection in brightness_show() 2016-11-04 7:52 [PATCH] leds: Add mutex protection in brightness_show() Jacek Anaszewski 2016-11-04 11:53 ` Hans de Goede @ 2016-11-09 12:38 ` Pavel Machek 2016-11-09 21:23 ` Jacek Anaszewski 1 sibling, 1 reply; 11+ messages in thread From: Pavel Machek @ 2016-11-09 12:38 UTC (permalink / raw) To: Jacek Anaszewski Cc: linux-leds, linux-kernel, Hans de Goede, Sakari Ailus, Andrew Lunn [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] leds: Add mutex protection in brightness_show() 2016-11-09 12:38 ` Pavel Machek @ 2016-11-09 21:23 ` Jacek Anaszewski 0 siblings, 0 replies; 11+ messages in thread From: Jacek Anaszewski @ 2016-11-09 21:23 UTC (permalink / raw) To: Pavel Machek, Jacek Anaszewski Cc: linux-leds, linux-kernel, Hans de Goede, Sakari Ailus, Andrew Lunn 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-11-09 21:24 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-04 7:52 [PATCH] leds: Add mutex protection in brightness_show() Jacek Anaszewski 2016-11-04 11:53 ` Hans de Goede 2016-11-04 16:06 ` Jacek Anaszewski 2016-11-04 16:46 ` Hans de Goede 2016-11-06 14:52 ` Hans de Goede 2016-11-07 9:11 ` Jacek Anaszewski 2016-11-09 12:37 ` Pavel Machek 2016-11-09 20:26 ` Pavel Machek 2016-11-09 21:23 ` Jacek Anaszewski 2016-11-09 12:38 ` Pavel Machek 2016-11-09 21:23 ` Jacek Anaszewski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).