* sysfs permissions on dynamic attributes (led delay_on and delay_off) @ 2012-07-21 0:46 Colin Cross 2012-07-21 4:08 ` Greg KH 0 siblings, 1 reply; 14+ messages in thread From: Colin Cross @ 2012-07-21 0:46 UTC (permalink / raw) To: lkml, Greg KH, Bryan Wu, Richard Purdie I'm trying to use the standard ledtrig-timer.c code to handle led blinking for notifications on an Android device, and I'm hitting some issues with setting permissions on the dynamically created delay_on and delay_off attributes. For most sysfs files, we have userspace uevent parser that watches for device add notifications and chowns/chmods attributes. This doesn't work for delay_on and delay_off, because they are created later, when "timer" is written to the trigger attribute. There is no uevent when the new files are created, and sysfs doesn't support inotify, so I don't see any way to receive an event to set the permissions. This issue exists any time that device_create_file is called after device_add. What is the appropriate way to get an event to set the permissions? Add inotify support for sysfs file creation? Send a KOBJ_CHANGE uevent in device_create_file? Send a KOBJ_CHANGE uevent from the driver after calling device_create_file? Dynamically create a timer device under /sys/class/leds/<led> so a new add uevent gets sent? Promote blinking to be a core led feature instead of a trigger, so the files are always present? ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: sysfs permissions on dynamic attributes (led delay_on and delay_off) 2012-07-21 0:46 sysfs permissions on dynamic attributes (led delay_on and delay_off) Colin Cross @ 2012-07-21 4:08 ` Greg KH 2012-07-21 5:14 ` Colin Cross 2012-07-21 7:33 ` Richard Purdie 0 siblings, 2 replies; 14+ messages in thread From: Greg KH @ 2012-07-21 4:08 UTC (permalink / raw) To: Colin Cross; +Cc: lkml, Bryan Wu, Richard Purdie On Fri, Jul 20, 2012 at 05:46:14PM -0700, Colin Cross wrote: > I'm trying to use the standard ledtrig-timer.c code to handle led > blinking for notifications on an Android device, and I'm hitting some > issues with setting permissions on the dynamically created delay_on > and delay_off attributes. For most sysfs files, we have userspace > uevent parser that watches for device add notifications and > chowns/chmods attributes. This doesn't work for delay_on and > delay_off, because they are created later, when "timer" is written to > the trigger attribute. There is no uevent when the new files are > created, and sysfs doesn't support inotify, so I don't see any way to > receive an event to set the permissions. This issue exists any time > that device_create_file is called after device_add. > > What is the appropriate way to get an event to set the permissions? > Add inotify support for sysfs file creation? Send a KOBJ_CHANGE > uevent in device_create_file? No. > Send a KOBJ_CHANGE uevent from the driver after calling > device_create_file? Yes. > Dynamically create a timer device under /sys/class/leds/<led> so a new > add uevent gets sent? Ick. > Promote blinking to be a core led feature instead of a trigger, so the > files are always present? That's the best thing, why not just do that? thanks, greg k-h ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: sysfs permissions on dynamic attributes (led delay_on and delay_off) 2012-07-21 4:08 ` Greg KH @ 2012-07-21 5:14 ` Colin Cross 2012-07-21 15:07 ` Greg KH 2012-07-21 7:33 ` Richard Purdie 1 sibling, 1 reply; 14+ messages in thread From: Colin Cross @ 2012-07-21 5:14 UTC (permalink / raw) To: Greg KH; +Cc: lkml, Bryan Wu, Richard Purdie On Fri, Jul 20, 2012 at 9:08 PM, Greg KH <gregkh@linuxfoundation.org> wrote: > On Fri, Jul 20, 2012 at 05:46:14PM -0700, Colin Cross wrote: >> I'm trying to use the standard ledtrig-timer.c code to handle led >> blinking for notifications on an Android device, and I'm hitting some >> issues with setting permissions on the dynamically created delay_on >> and delay_off attributes. For most sysfs files, we have userspace >> uevent parser that watches for device add notifications and >> chowns/chmods attributes. This doesn't work for delay_on and >> delay_off, because they are created later, when "timer" is written to >> the trigger attribute. There is no uevent when the new files are >> created, and sysfs doesn't support inotify, so I don't see any way to >> receive an event to set the permissions. This issue exists any time >> that device_create_file is called after device_add. >> >> What is the appropriate way to get an event to set the permissions? >> Add inotify support for sysfs file creation? Send a KOBJ_CHANGE >> uevent in device_create_file? > > No. > >> Send a KOBJ_CHANGE uevent from the driver after calling >> device_create_file? > > Yes. > >> Dynamically create a timer device under /sys/class/leds/<led> so a new >> add uevent gets sent? > > Ick. > >> Promote blinking to be a core led feature instead of a trigger, so the >> files are always present? > > That's the best thing, why not just do that? It doesn't solve the general case. For example, any driver that is loaded as a module and then calls device_create_file will suffer the same problem. But since it solves the one I care about, I'll look into it. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: sysfs permissions on dynamic attributes (led delay_on and delay_off) 2012-07-21 5:14 ` Colin Cross @ 2012-07-21 15:07 ` Greg KH 0 siblings, 0 replies; 14+ messages in thread From: Greg KH @ 2012-07-21 15:07 UTC (permalink / raw) To: Colin Cross; +Cc: lkml, Bryan Wu, Richard Purdie On Fri, Jul 20, 2012 at 10:14:27PM -0700, Colin Cross wrote: > On Fri, Jul 20, 2012 at 9:08 PM, Greg KH <gregkh@linuxfoundation.org> wrote: > > On Fri, Jul 20, 2012 at 05:46:14PM -0700, Colin Cross wrote: > >> I'm trying to use the standard ledtrig-timer.c code to handle led > >> blinking for notifications on an Android device, and I'm hitting some > >> issues with setting permissions on the dynamically created delay_on > >> and delay_off attributes. For most sysfs files, we have userspace > >> uevent parser that watches for device add notifications and > >> chowns/chmods attributes. This doesn't work for delay_on and > >> delay_off, because they are created later, when "timer" is written to > >> the trigger attribute. There is no uevent when the new files are > >> created, and sysfs doesn't support inotify, so I don't see any way to > >> receive an event to set the permissions. This issue exists any time > >> that device_create_file is called after device_add. > >> > >> What is the appropriate way to get an event to set the permissions? > >> Add inotify support for sysfs file creation? Send a KOBJ_CHANGE > >> uevent in device_create_file? > > > > No. > > > >> Send a KOBJ_CHANGE uevent from the driver after calling > >> device_create_file? > > > > Yes. > > > >> Dynamically create a timer device under /sys/class/leds/<led> so a new > >> add uevent gets sent? > > > > Ick. > > > >> Promote blinking to be a core led feature instead of a trigger, so the > >> files are always present? > > > > That's the best thing, why not just do that? > > It doesn't solve the general case. For example, any driver that is > loaded as a module and then calls device_create_file will suffer the > same problem. That's very true, and is why they shouldn't be doing that :) thanks, greg k-h ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: sysfs permissions on dynamic attributes (led delay_on and delay_off) 2012-07-21 4:08 ` Greg KH 2012-07-21 5:14 ` Colin Cross @ 2012-07-21 7:33 ` Richard Purdie 2012-07-21 8:26 ` Colin Cross 1 sibling, 1 reply; 14+ messages in thread From: Richard Purdie @ 2012-07-21 7:33 UTC (permalink / raw) To: Greg KH; +Cc: Colin Cross, lkml, Bryan Wu On Fri, 2012-07-20 at 21:08 -0700, Greg KH wrote: > On Fri, Jul 20, 2012 at 05:46:14PM -0700, Colin Cross wrote: > > I'm trying to use the standard ledtrig-timer.c code to handle led > > blinking for notifications on an Android device, and I'm hitting some > > issues with setting permissions on the dynamically created delay_on > > and delay_off attributes. For most sysfs files, we have userspace > > uevent parser that watches for device add notifications and > > chowns/chmods attributes. This doesn't work for delay_on and > > delay_off, because they are created later, when "timer" is written to > > the trigger attribute. There is no uevent when the new files are > > created, and sysfs doesn't support inotify, so I don't see any way to > > receive an event to set the permissions. This issue exists any time > > that device_create_file is called after device_add. > > > > What is the appropriate way to get an event to set the permissions? > > Add inotify support for sysfs file creation? Send a KOBJ_CHANGE > > uevent in device_create_file? > > No. > > > Send a KOBJ_CHANGE uevent from the driver after calling > > device_create_file? > > Yes. > > > Dynamically create a timer device under /sys/class/leds/<led> so a new > > add uevent gets sent? > > Ick. > > > Promote blinking to be a core led feature instead of a trigger, so the > > files are always present? > > That's the best thing, why not just do that? This implies we should make every trigger a core led feature and effectively do away with triggers. I'm not sure that makes sense. Cheers, Richard ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: sysfs permissions on dynamic attributes (led delay_on and delay_off) 2012-07-21 7:33 ` Richard Purdie @ 2012-07-21 8:26 ` Colin Cross 2012-07-21 11:21 ` Richard Purdie 0 siblings, 1 reply; 14+ messages in thread From: Colin Cross @ 2012-07-21 8:26 UTC (permalink / raw) To: Richard Purdie; +Cc: Greg KH, lkml, Bryan Wu On Sat, Jul 21, 2012 at 12:33 AM, Richard Purdie <richard.purdie@linuxfoundation.org> wrote: > On Fri, 2012-07-20 at 21:08 -0700, Greg KH wrote: >> On Fri, Jul 20, 2012 at 05:46:14PM -0700, Colin Cross wrote: >> > I'm trying to use the standard ledtrig-timer.c code to handle led >> > blinking for notifications on an Android device, and I'm hitting some >> > issues with setting permissions on the dynamically created delay_on >> > and delay_off attributes. For most sysfs files, we have userspace >> > uevent parser that watches for device add notifications and >> > chowns/chmods attributes. This doesn't work for delay_on and >> > delay_off, because they are created later, when "timer" is written to >> > the trigger attribute. There is no uevent when the new files are >> > created, and sysfs doesn't support inotify, so I don't see any way to >> > receive an event to set the permissions. This issue exists any time >> > that device_create_file is called after device_add. >> > >> > What is the appropriate way to get an event to set the permissions? >> > Add inotify support for sysfs file creation? Send a KOBJ_CHANGE >> > uevent in device_create_file? >> >> No. >> >> > Send a KOBJ_CHANGE uevent from the driver after calling >> > device_create_file? >> >> Yes. >> >> > Dynamically create a timer device under /sys/class/leds/<led> so a new >> > add uevent gets sent? >> >> Ick. >> >> > Promote blinking to be a core led feature instead of a trigger, so the >> > files are always present? >> >> That's the best thing, why not just do that? > > This implies we should make every trigger a core led feature and > effectively do away with triggers. I'm not sure that makes sense. Blinking is already effectively a core feature. It is implemented in led-core.c so it can be used by other triggers besides timer, it's state is stored in the led_classdev structure, not in the trigger data, and the only thing left in ledtrig-timer.c is the sysfs files. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: sysfs permissions on dynamic attributes (led delay_on and delay_off) 2012-07-21 8:26 ` Colin Cross @ 2012-07-21 11:21 ` Richard Purdie 2012-07-21 14:31 ` Henrique de Moraes Holschuh 2012-07-21 15:42 ` Colin Cross 0 siblings, 2 replies; 14+ messages in thread From: Richard Purdie @ 2012-07-21 11:21 UTC (permalink / raw) To: Colin Cross; +Cc: Greg KH, lkml, Bryan Wu On Sat, 2012-07-21 at 01:26 -0700, Colin Cross wrote: > On Sat, Jul 21, 2012 at 12:33 AM, Richard Purdie > <richard.purdie@linuxfoundation.org> wrote: > > On Fri, 2012-07-20 at 21:08 -0700, Greg KH wrote: > >> On Fri, Jul 20, 2012 at 05:46:14PM -0700, Colin Cross wrote: > >> > I'm trying to use the standard ledtrig-timer.c code to handle led > >> > blinking for notifications on an Android device, and I'm hitting some > >> > issues with setting permissions on the dynamically created delay_on > >> > and delay_off attributes. For most sysfs files, we have userspace > >> > uevent parser that watches for device add notifications and > >> > chowns/chmods attributes. This doesn't work for delay_on and > >> > delay_off, because they are created later, when "timer" is written to > >> > the trigger attribute. There is no uevent when the new files are > >> > created, and sysfs doesn't support inotify, so I don't see any way to > >> > receive an event to set the permissions. This issue exists any time > >> > that device_create_file is called after device_add. > >> > > >> > What is the appropriate way to get an event to set the permissions? > >> > Add inotify support for sysfs file creation? Send a KOBJ_CHANGE > >> > uevent in device_create_file? > >> > >> No. > >> > >> > Send a KOBJ_CHANGE uevent from the driver after calling > >> > device_create_file? > >> > >> Yes. > >> > >> > Dynamically create a timer device under /sys/class/leds/<led> so a new > >> > add uevent gets sent? > >> > >> Ick. > >> > >> > Promote blinking to be a core led feature instead of a trigger, so the > >> > files are always present? > >> > >> That's the best thing, why not just do that? > > > > This implies we should make every trigger a core led feature and > > effectively do away with triggers. I'm not sure that makes sense. > > Blinking is already effectively a core feature. It is implemented in > led-core.c so it can be used by other triggers besides timer, it's > state is stored in the led_classdev structure, not in the trigger > data, and the only thing left in ledtrig-timer.c is the sysfs files. Having the attributes present all the time leads to some nasty questions like how the on/off delays interact with things like say a network activity trigger. Not all triggers are going to respect these delay values and I can imagine a whole new set of nasty bug reports with no easy solutions if this change is made... Cheers, Richard ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: sysfs permissions on dynamic attributes (led delay_on and delay_off) 2012-07-21 11:21 ` Richard Purdie @ 2012-07-21 14:31 ` Henrique de Moraes Holschuh 2012-07-21 15:42 ` Colin Cross 1 sibling, 0 replies; 14+ messages in thread From: Henrique de Moraes Holschuh @ 2012-07-21 14:31 UTC (permalink / raw) To: Richard Purdie; +Cc: Colin Cross, Greg KH, lkml, Bryan Wu On Sat, 21 Jul 2012, Richard Purdie wrote: > On Sat, 2012-07-21 at 01:26 -0700, Colin Cross wrote: > > On Sat, Jul 21, 2012 at 12:33 AM, Richard Purdie > > <richard.purdie@linuxfoundation.org> wrote: > > > On Fri, 2012-07-20 at 21:08 -0700, Greg KH wrote: > > >> On Fri, Jul 20, 2012 at 05:46:14PM -0700, Colin Cross wrote: > > >> > I'm trying to use the standard ledtrig-timer.c code to handle led > > >> > blinking for notifications on an Android device, and I'm hitting some > > >> > issues with setting permissions on the dynamically created delay_on > > >> > and delay_off attributes. For most sysfs files, we have userspace > > >> > uevent parser that watches for device add notifications and > > >> > chowns/chmods attributes. This doesn't work for delay_on and > > >> > delay_off, because they are created later, when "timer" is written to > > >> > the trigger attribute. There is no uevent when the new files are > > >> > created, and sysfs doesn't support inotify, so I don't see any way to > > >> > receive an event to set the permissions. This issue exists any time > > >> > that device_create_file is called after device_add. > > >> > > > >> > What is the appropriate way to get an event to set the permissions? > > >> > Add inotify support for sysfs file creation? Send a KOBJ_CHANGE > > >> > uevent in device_create_file? > > >> > > >> No. > > >> > > >> > Send a KOBJ_CHANGE uevent from the driver after calling > > >> > device_create_file? > > >> > > >> Yes. ... > > Blinking is already effectively a core feature. It is implemented in > > led-core.c so it can be used by other triggers besides timer, it's > > state is stored in the led_classdev structure, not in the trigger > > data, and the only thing left in ledtrig-timer.c is the sysfs files. > > Having the attributes present all the time leads to some nasty questions > like how the on/off delays interact with things like say a network > activity trigger. Not all triggers are going to respect these delay > values and I can imagine a whole new set of nasty bug reports with no > easy solutions if this change is made... Agreed. Let's just fix this properly and issue KOBJ_CHANGE when any sysfs attributes are added or removed to an already registered LED class, please? It is the proper thing to do, and it is a valid fix for all trigger types. -- "One disk to rule them all, One disk to find them. One disk to bring them all and in the darkness grind them. In the Land of Redmond where the shadows lie." -- The Silicon Valley Tarot Henrique Holschuh ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: sysfs permissions on dynamic attributes (led delay_on and delay_off) 2012-07-21 11:21 ` Richard Purdie 2012-07-21 14:31 ` Henrique de Moraes Holschuh @ 2012-07-21 15:42 ` Colin Cross 2012-07-21 16:08 ` Henrique de Moraes Holschuh 2012-07-21 16:13 ` Greg KH 1 sibling, 2 replies; 14+ messages in thread From: Colin Cross @ 2012-07-21 15:42 UTC (permalink / raw) To: Richard Purdie; +Cc: Greg KH, lkml, Bryan Wu On Sat, Jul 21, 2012 at 4:21 AM, Richard Purdie <richard.purdie@linuxfoundation.org> wrote: > On Sat, 2012-07-21 at 01:26 -0700, Colin Cross wrote: >> On Sat, Jul 21, 2012 at 12:33 AM, Richard Purdie >> <richard.purdie@linuxfoundation.org> wrote: >> > On Fri, 2012-07-20 at 21:08 -0700, Greg KH wrote: >> >> On Fri, Jul 20, 2012 at 05:46:14PM -0700, Colin Cross wrote: >> >> > I'm trying to use the standard ledtrig-timer.c code to handle led >> >> > blinking for notifications on an Android device, and I'm hitting some >> >> > issues with setting permissions on the dynamically created delay_on >> >> > and delay_off attributes. For most sysfs files, we have userspace >> >> > uevent parser that watches for device add notifications and >> >> > chowns/chmods attributes. This doesn't work for delay_on and >> >> > delay_off, because they are created later, when "timer" is written to >> >> > the trigger attribute. There is no uevent when the new files are >> >> > created, and sysfs doesn't support inotify, so I don't see any way to >> >> > receive an event to set the permissions. This issue exists any time >> >> > that device_create_file is called after device_add. >> >> > >> >> > What is the appropriate way to get an event to set the permissions? >> >> > Add inotify support for sysfs file creation? Send a KOBJ_CHANGE >> >> > uevent in device_create_file? >> >> >> >> No. >> >> >> >> > Send a KOBJ_CHANGE uevent from the driver after calling >> >> > device_create_file? >> >> >> >> Yes. >> >> >> >> > Dynamically create a timer device under /sys/class/leds/<led> so a new >> >> > add uevent gets sent? >> >> >> >> Ick. >> >> >> >> > Promote blinking to be a core led feature instead of a trigger, so the >> >> > files are always present? >> >> >> >> That's the best thing, why not just do that? >> > >> > This implies we should make every trigger a core led feature and >> > effectively do away with triggers. I'm not sure that makes sense. >> >> Blinking is already effectively a core feature. It is implemented in >> led-core.c so it can be used by other triggers besides timer, it's >> state is stored in the led_classdev structure, not in the trigger >> data, and the only thing left in ledtrig-timer.c is the sysfs files. > > Having the attributes present all the time leads to some nasty questions > like how the on/off delays interact with things like say a network > activity trigger. Not all triggers are going to respect these delay > values and I can imagine a whole new set of nasty bug reports with no > easy solutions if this change is made... The delay_on and delay_off files could easily override the values from the trigger. Sending a KOBJ_CHANGE uevent is not a great solution, it's still horribly racy in userspace. This script would never work reliably: echo timer > trigger echo 1000 > delay_on echo 1000 > delay_off echo 255 > brightness ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: sysfs permissions on dynamic attributes (led delay_on and delay_off) 2012-07-21 15:42 ` Colin Cross @ 2012-07-21 16:08 ` Henrique de Moraes Holschuh 2012-07-21 16:15 ` Greg KH 2012-07-21 16:13 ` Greg KH 1 sibling, 1 reply; 14+ messages in thread From: Henrique de Moraes Holschuh @ 2012-07-21 16:08 UTC (permalink / raw) To: Colin Cross; +Cc: Richard Purdie, Greg KH, lkml, Bryan Wu On Sat, 21 Jul 2012, Colin Cross wrote: > The delay_on and delay_off files could easily override the values from > the trigger. > > Sending a KOBJ_CHANGE uevent is not a great solution, it's still > horribly racy in userspace. This script would never work reliably: > echo timer > trigger > echo 1000 > delay_on > echo 1000 > delay_off > echo 255 > brightness Yes, and the proper fix is to instead use a fully async userspace based on uevent callbacks. Nasty as all hell. Or the quick fix, which is to wait for the system to settle after every sysfs operation that could create new sysfs nodes. You could make sure that (1) no sysfs operation will return control to userspace until it is complete, so you'd have all new sysfs nodes available at the time the first echo returns [I believe it already works like that], and (2) either enhance sysfs to create nodes with the desired ownership and permissions -- which requires feeding policy rules to it beforehand at the very least; or do it as whichever priviledged user/group has default write access to sysfs nodes. This is a general problem. I have no idea whether Richard will allow the triggers hack to work around it or not, and I really don't care. But the bad sideffects of the hack that he pointed out are all very true, and far reaching in time. -- "One disk to rule them all, One disk to find them. One disk to bring them all and in the darkness grind them. In the Land of Redmond where the shadows lie." -- The Silicon Valley Tarot Henrique Holschuh ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: sysfs permissions on dynamic attributes (led delay_on and delay_off) 2012-07-21 16:08 ` Henrique de Moraes Holschuh @ 2012-07-21 16:15 ` Greg KH 2012-07-21 16:23 ` Colin Cross 0 siblings, 1 reply; 14+ messages in thread From: Greg KH @ 2012-07-21 16:15 UTC (permalink / raw) To: Henrique de Moraes Holschuh; +Cc: Colin Cross, Richard Purdie, lkml, Bryan Wu On Sat, Jul 21, 2012 at 01:08:55PM -0300, Henrique de Moraes Holschuh wrote: > On Sat, 21 Jul 2012, Colin Cross wrote: > > The delay_on and delay_off files could easily override the values from > > the trigger. > > > > Sending a KOBJ_CHANGE uevent is not a great solution, it's still > > horribly racy in userspace. This script would never work reliably: > > echo timer > trigger > > echo 1000 > delay_on > > echo 1000 > delay_off > > echo 255 > brightness > > Yes, and the proper fix is to instead use a fully async userspace based on > uevent callbacks. Nasty as all hell. Or the quick fix, which is to wait > for the system to settle after every sysfs operation that could create new > sysfs nodes. > > You could make sure that (1) no sysfs operation will return control to > userspace until it is complete, so you'd have all new sysfs nodes available > at the time the first echo returns [I believe it already works like that], Yes it does, what's the problem here? > and (2) either enhance sysfs to create nodes with the desired ownership and > permissions >From the kernel, you can also do this today, if you know it's "safe" for users to read/write them. greg k-h ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: sysfs permissions on dynamic attributes (led delay_on and delay_off) 2012-07-21 16:15 ` Greg KH @ 2012-07-21 16:23 ` Colin Cross 0 siblings, 0 replies; 14+ messages in thread From: Colin Cross @ 2012-07-21 16:23 UTC (permalink / raw) To: Greg KH; +Cc: Henrique de Moraes Holschuh, Richard Purdie, lkml, Bryan Wu On Sat, Jul 21, 2012 at 9:15 AM, Greg KH <gregkh@linuxfoundation.org> wrote: > On Sat, Jul 21, 2012 at 01:08:55PM -0300, Henrique de Moraes Holschuh wrote: >> On Sat, 21 Jul 2012, Colin Cross wrote: >> > The delay_on and delay_off files could easily override the values from >> > the trigger. >> > >> > Sending a KOBJ_CHANGE uevent is not a great solution, it's still >> > horribly racy in userspace. This script would never work reliably: >> > echo timer > trigger >> > echo 1000 > delay_on >> > echo 1000 > delay_off >> > echo 255 > brightness >> >> Yes, and the proper fix is to instead use a fully async userspace based on >> uevent callbacks. Nasty as all hell. Or the quick fix, which is to wait >> for the system to settle after every sysfs operation that could create new >> sysfs nodes. >> >> You could make sure that (1) no sysfs operation will return control to >> userspace until it is complete, so you'd have all new sysfs nodes available >> at the time the first echo returns [I believe it already works like that], > > Yes it does, what's the problem here? > >> and (2) either enhance sysfs to create nodes with the desired ownership and >> permissions > > From the kernel, you can also do this today, if you know it's "safe" for > users to read/write them. You can set the permissions, but I don't know of any way to set the owner. For my case, I need them to be writable by the "system" user but not by all users. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: sysfs permissions on dynamic attributes (led delay_on and delay_off) 2012-07-21 15:42 ` Colin Cross 2012-07-21 16:08 ` Henrique de Moraes Holschuh @ 2012-07-21 16:13 ` Greg KH 2012-07-21 16:22 ` Colin Cross 1 sibling, 1 reply; 14+ messages in thread From: Greg KH @ 2012-07-21 16:13 UTC (permalink / raw) To: Colin Cross; +Cc: Richard Purdie, lkml, Bryan Wu On Sat, Jul 21, 2012 at 08:42:12AM -0700, Colin Cross wrote: > On Sat, Jul 21, 2012 at 4:21 AM, Richard Purdie > <richard.purdie@linuxfoundation.org> wrote: > > On Sat, 2012-07-21 at 01:26 -0700, Colin Cross wrote: > >> On Sat, Jul 21, 2012 at 12:33 AM, Richard Purdie > >> <richard.purdie@linuxfoundation.org> wrote: > >> > On Fri, 2012-07-20 at 21:08 -0700, Greg KH wrote: > >> >> On Fri, Jul 20, 2012 at 05:46:14PM -0700, Colin Cross wrote: > >> >> > I'm trying to use the standard ledtrig-timer.c code to handle led > >> >> > blinking for notifications on an Android device, and I'm hitting some > >> >> > issues with setting permissions on the dynamically created delay_on > >> >> > and delay_off attributes. For most sysfs files, we have userspace > >> >> > uevent parser that watches for device add notifications and > >> >> > chowns/chmods attributes. This doesn't work for delay_on and > >> >> > delay_off, because they are created later, when "timer" is written to > >> >> > the trigger attribute. There is no uevent when the new files are > >> >> > created, and sysfs doesn't support inotify, so I don't see any way to > >> >> > receive an event to set the permissions. This issue exists any time > >> >> > that device_create_file is called after device_add. > >> >> > > >> >> > What is the appropriate way to get an event to set the permissions? > >> >> > Add inotify support for sysfs file creation? Send a KOBJ_CHANGE > >> >> > uevent in device_create_file? > >> >> > >> >> No. > >> >> > >> >> > Send a KOBJ_CHANGE uevent from the driver after calling > >> >> > device_create_file? > >> >> > >> >> Yes. > >> >> > >> >> > Dynamically create a timer device under /sys/class/leds/<led> so a new > >> >> > add uevent gets sent? > >> >> > >> >> Ick. > >> >> > >> >> > Promote blinking to be a core led feature instead of a trigger, so the > >> >> > files are always present? > >> >> > >> >> That's the best thing, why not just do that? > >> > > >> > This implies we should make every trigger a core led feature and > >> > effectively do away with triggers. I'm not sure that makes sense. > >> > >> Blinking is already effectively a core feature. It is implemented in > >> led-core.c so it can be used by other triggers besides timer, it's > >> state is stored in the led_classdev structure, not in the trigger > >> data, and the only thing left in ledtrig-timer.c is the sysfs files. > > > > Having the attributes present all the time leads to some nasty questions > > like how the on/off delays interact with things like say a network > > activity trigger. Not all triggers are going to respect these delay > > values and I can imagine a whole new set of nasty bug reports with no > > easy solutions if this change is made... > > The delay_on and delay_off files could easily override the values from > the trigger. > > Sending a KOBJ_CHANGE uevent is not a great solution, it's still > horribly racy in userspace. This script would never work reliably: > echo timer > trigger When this returned, the sysfs files would then be there, right? > echo 1000 > delay_on > echo 1000 > delay_off > echo 255 > brightness So this would work. What is racy here? greg k-h ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: sysfs permissions on dynamic attributes (led delay_on and delay_off) 2012-07-21 16:13 ` Greg KH @ 2012-07-21 16:22 ` Colin Cross 0 siblings, 0 replies; 14+ messages in thread From: Colin Cross @ 2012-07-21 16:22 UTC (permalink / raw) To: Greg KH; +Cc: Richard Purdie, lkml, Bryan Wu On Sat, Jul 21, 2012 at 9:13 AM, Greg KH <gregkh@linuxfoundation.org> wrote: > On Sat, Jul 21, 2012 at 08:42:12AM -0700, Colin Cross wrote: >> On Sat, Jul 21, 2012 at 4:21 AM, Richard Purdie >> <richard.purdie@linuxfoundation.org> wrote: >> > On Sat, 2012-07-21 at 01:26 -0700, Colin Cross wrote: >> >> On Sat, Jul 21, 2012 at 12:33 AM, Richard Purdie >> >> <richard.purdie@linuxfoundation.org> wrote: >> >> > On Fri, 2012-07-20 at 21:08 -0700, Greg KH wrote: >> >> >> On Fri, Jul 20, 2012 at 05:46:14PM -0700, Colin Cross wrote: >> >> >> > I'm trying to use the standard ledtrig-timer.c code to handle led >> >> >> > blinking for notifications on an Android device, and I'm hitting some >> >> >> > issues with setting permissions on the dynamically created delay_on >> >> >> > and delay_off attributes. For most sysfs files, we have userspace >> >> >> > uevent parser that watches for device add notifications and >> >> >> > chowns/chmods attributes. This doesn't work for delay_on and >> >> >> > delay_off, because they are created later, when "timer" is written to >> >> >> > the trigger attribute. There is no uevent when the new files are >> >> >> > created, and sysfs doesn't support inotify, so I don't see any way to >> >> >> > receive an event to set the permissions. This issue exists any time >> >> >> > that device_create_file is called after device_add. >> >> >> > >> >> >> > What is the appropriate way to get an event to set the permissions? >> >> >> > Add inotify support for sysfs file creation? Send a KOBJ_CHANGE >> >> >> > uevent in device_create_file? >> >> >> >> >> >> No. >> >> >> >> >> >> > Send a KOBJ_CHANGE uevent from the driver after calling >> >> >> > device_create_file? >> >> >> >> >> >> Yes. >> >> >> >> >> >> > Dynamically create a timer device under /sys/class/leds/<led> so a new >> >> >> > add uevent gets sent? >> >> >> >> >> >> Ick. >> >> >> >> >> >> > Promote blinking to be a core led feature instead of a trigger, so the >> >> >> > files are always present? >> >> >> >> >> >> That's the best thing, why not just do that? >> >> > >> >> > This implies we should make every trigger a core led feature and >> >> > effectively do away with triggers. I'm not sure that makes sense. >> >> >> >> Blinking is already effectively a core feature. It is implemented in >> >> led-core.c so it can be used by other triggers besides timer, it's >> >> state is stored in the led_classdev structure, not in the trigger >> >> data, and the only thing left in ledtrig-timer.c is the sysfs files. >> > >> > Having the attributes present all the time leads to some nasty questions >> > like how the on/off delays interact with things like say a network >> > activity trigger. Not all triggers are going to respect these delay >> > values and I can imagine a whole new set of nasty bug reports with no >> > easy solutions if this change is made... >> >> The delay_on and delay_off files could easily override the values from >> the trigger. >> >> Sending a KOBJ_CHANGE uevent is not a great solution, it's still >> horribly racy in userspace. This script would never work reliably: >> echo timer > trigger > > When this returned, the sysfs files would then be there, right? Yes, but they would owned by root and not writable. udev would be triggered by the KOBJ_CHANGE event and eventually chown/chmod them, but possibly too late. >> echo 1000 > delay_on >> echo 1000 > delay_off >> echo 255 > brightness > > So this would work. > > What is racy here? It's racy if the script is run as non-root, assuming udev has already chowned/chmoded the trigger and brightness files. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2012-07-21 16:24 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-07-21 0:46 sysfs permissions on dynamic attributes (led delay_on and delay_off) Colin Cross 2012-07-21 4:08 ` Greg KH 2012-07-21 5:14 ` Colin Cross 2012-07-21 15:07 ` Greg KH 2012-07-21 7:33 ` Richard Purdie 2012-07-21 8:26 ` Colin Cross 2012-07-21 11:21 ` Richard Purdie 2012-07-21 14:31 ` Henrique de Moraes Holschuh 2012-07-21 15:42 ` Colin Cross 2012-07-21 16:08 ` Henrique de Moraes Holschuh 2012-07-21 16:15 ` Greg KH 2012-07-21 16:23 ` Colin Cross 2012-07-21 16:13 ` Greg KH 2012-07-21 16:22 ` Colin Cross
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).