sysfs permissions on dynamic attributes (led delay_on and delay_off)

sysfs permissions on dynamic attributes (led delay_on and delay_off)

[Colin Cross]

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?

Re: sysfs permissions on dynamic attributes (led delay_on and delay_off)

[Greg KH]

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

Re: sysfs permissions on dynamic attributes (led delay_on and delay_off)

[Colin Cross]

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.

Re: sysfs permissions on dynamic attributes (led delay_on and delay_off)

[Richard Purdie]

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

Re: sysfs permissions on dynamic attributes (led delay_on and delay_off)

[Colin Cross]

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.

Re: sysfs permissions on dynamic attributes (led delay_on and delay_off)

[Richard Purdie]

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

Re: sysfs permissions on dynamic attributes (led delay_on and delay_off)

[Henrique de Moraes Holschuh]

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

Re: sysfs permissions on dynamic attributes (led delay_on and delay_off)

[Greg KH]

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

Re: sysfs permissions on dynamic attributes (led delay_on and delay_off)

[Colin Cross]

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

Re: sysfs permissions on dynamic attributes (led delay_on and delay_off)

[Henrique de Moraes Holschuh]

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

Re: sysfs permissions on dynamic attributes (led delay_on and delay_off)

[Greg KH]

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

Re: sysfs permissions on dynamic attributes (led delay_on and delay_off)

[Greg KH]

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

Re: sysfs permissions on dynamic attributes (led delay_on and delay_off)

[Colin Cross]

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.

Re: sysfs permissions on dynamic attributes (led delay_on and delay_off)

[Colin Cross]

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.