diff mbox series

[1/3] sysfs: Fix crash on empty group attributes array

Message ID 170863445442.1479840.1818801787239831650.stgit@dwillia2-xfh.jf.intel.com (mailing list archive)
State New, archived
Headers show
Series sysfs: Group visibility fixups | expand

Commit Message

Dan Williams Feb. 22, 2024, 8:40 p.m. UTC
It turns out that arch/x86/events/intel/core.c makes use of "empty"
attributes.

	static struct attribute *empty_attrs;

	__init int intel_pmu_init(void)
	{
	        struct attribute **extra_skl_attr = &empty_attrs;
	        struct attribute **extra_attr = &empty_attrs;
	        struct attribute **td_attr    = &empty_attrs;
	        struct attribute **mem_attr   = &empty_attrs;
	        struct attribute **tsx_attr   = &empty_attrs;
		...

That breaks the assumption __first_visible() that expects that if
grp->attrs is set then grp->attrs[0] must also be set and results in
backtraces like:

    BUG: kernel NULL pointer dereference, address: 00rnel mode
    #PF: error_code(0x0000) - not-present ] PREEMPT SMP NOPTI
    CPU: 1 PID: 1 Comm: swapper/IP: 0010:exra_is_visible+0x14/0x20
     ? exc_page_fault+0x68/0x190
     internal_create_groups+0x42/0xa0
     pmu_dev_alloc+0xc0/0xe0
     perf_event_sysfs_init+0x580000000000 ]---
    RIP: 0010:exra_is_visible+0x14/0

Check for non-empty attributes array before calling is_visible().

Reported-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Closes: https://github.com/thesofproject/linux/pull/4799#issuecomment-1958537212
Fixes: 70317fd24b41 ("sysfs: Introduce a mechanism to hide static attribute_groups")
Cc: Marc Herbert <marc.herbert@intel.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 fs/sysfs/group.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Rafael J. Wysocki Feb. 22, 2024, 9:14 p.m. UTC | #1
On Thu, Feb 22, 2024 at 9:40 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> It turns out that arch/x86/events/intel/core.c makes use of "empty"
> attributes.
>
>         static struct attribute *empty_attrs;
>
>         __init int intel_pmu_init(void)
>         {
>                 struct attribute **extra_skl_attr = &empty_attrs;
>                 struct attribute **extra_attr = &empty_attrs;
>                 struct attribute **td_attr    = &empty_attrs;
>                 struct attribute **mem_attr   = &empty_attrs;
>                 struct attribute **tsx_attr   = &empty_attrs;
>                 ...
>
> That breaks the assumption __first_visible() that expects that if
> grp->attrs is set then grp->attrs[0] must also be set and results in
> backtraces like:
>
>     BUG: kernel NULL pointer dereference, address: 00rnel mode
>     #PF: error_code(0x0000) - not-present ] PREEMPT SMP NOPTI
>     CPU: 1 PID: 1 Comm: swapper/IP: 0010:exra_is_visible+0x14/0x20
>      ? exc_page_fault+0x68/0x190
>      internal_create_groups+0x42/0xa0
>      pmu_dev_alloc+0xc0/0xe0
>      perf_event_sysfs_init+0x580000000000 ]---
>     RIP: 0010:exra_is_visible+0x14/0
>
> Check for non-empty attributes array before calling is_visible().
>
> Reported-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Closes: https://github.com/thesofproject/linux/pull/4799#issuecomment-1958537212
> Fixes: 70317fd24b41 ("sysfs: Introduce a mechanism to hide static attribute_groups")

This is not in the mainline, so linux-next I suppose?

> Cc: Marc Herbert <marc.herbert@intel.com>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  fs/sysfs/group.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index ccb275cdabcb..8c63ba3cfc47 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -33,10 +33,10 @@ static void remove_files(struct kernfs_node *parent,
>
>  static umode_t __first_visible(const struct attribute_group *grp, struct kobject *kobj)
>  {
> -       if (grp->attrs && grp->is_visible)
> +       if (grp->attrs && grp->attrs[0] && grp->is_visible)
>                 return grp->is_visible(kobj, grp->attrs[0], 0);
>
> -       if (grp->bin_attrs && grp->is_bin_visible)
> +       if (grp->bin_attrs && grp->bin_attrs[0] && grp->is_bin_visible)
>                 return grp->is_bin_visible(kobj, grp->bin_attrs[0], 0);
>
>         return 0;
>
Dan Williams Feb. 22, 2024, 10:03 p.m. UTC | #2
Rafael J. Wysocki wrote:
> On Thu, Feb 22, 2024 at 9:40 PM Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > It turns out that arch/x86/events/intel/core.c makes use of "empty"
> > attributes.
> >
> >         static struct attribute *empty_attrs;
> >
> >         __init int intel_pmu_init(void)
> >         {
> >                 struct attribute **extra_skl_attr = &empty_attrs;
> >                 struct attribute **extra_attr = &empty_attrs;
> >                 struct attribute **td_attr    = &empty_attrs;
> >                 struct attribute **mem_attr   = &empty_attrs;
> >                 struct attribute **tsx_attr   = &empty_attrs;
> >                 ...
> >
> > That breaks the assumption __first_visible() that expects that if
> > grp->attrs is set then grp->attrs[0] must also be set and results in
> > backtraces like:
> >
> >     BUG: kernel NULL pointer dereference, address: 00rnel mode
> >     #PF: error_code(0x0000) - not-present ] PREEMPT SMP NOPTI
> >     CPU: 1 PID: 1 Comm: swapper/IP: 0010:exra_is_visible+0x14/0x20
> >      ? exc_page_fault+0x68/0x190
> >      internal_create_groups+0x42/0xa0
> >      pmu_dev_alloc+0xc0/0xe0
> >      perf_event_sysfs_init+0x580000000000 ]---
> >     RIP: 0010:exra_is_visible+0x14/0
> >
> > Check for non-empty attributes array before calling is_visible().
> >
> > Reported-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > Closes: https://github.com/thesofproject/linux/pull/4799#issuecomment-1958537212
> > Fixes: 70317fd24b41 ("sysfs: Introduce a mechanism to hide static attribute_groups")
> 
> This is not in the mainline, so linux-next I suppose?

...or at least it will be shortly. Greg notified that it is currently in
driver-core-next:

https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/commit/?h=driver-core-next&id=70317fd24b41
Dan Williams Feb. 22, 2024, 11:15 p.m. UTC | #3
Dan Williams wrote:
> It turns out that arch/x86/events/intel/core.c makes use of "empty"
> attributes.
> 
> 	static struct attribute *empty_attrs;
> 
> 	__init int intel_pmu_init(void)
> 	{
> 	        struct attribute **extra_skl_attr = &empty_attrs;
> 	        struct attribute **extra_attr = &empty_attrs;
> 	        struct attribute **td_attr    = &empty_attrs;
> 	        struct attribute **mem_attr   = &empty_attrs;
> 	        struct attribute **tsx_attr   = &empty_attrs;
> 		...
> 
> That breaks the assumption __first_visible() that expects that if
> grp->attrs is set then grp->attrs[0] must also be set and results in
> backtraces like:
> 
>     BUG: kernel NULL pointer dereference, address: 00rnel mode
>     #PF: error_code(0x0000) - not-present ] PREEMPT SMP NOPTI
>     CPU: 1 PID: 1 Comm: swapper/IP: 0010:exra_is_visible+0x14/0x20
>      ? exc_page_fault+0x68/0x190
>      internal_create_groups+0x42/0xa0
>      pmu_dev_alloc+0xc0/0xe0
>      perf_event_sysfs_init+0x580000000000 ]---
>     RIP: 0010:exra_is_visible+0x14/0
> 
> Check for non-empty attributes array before calling is_visible().
> 
> Reported-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Closes: https://github.com/thesofproject/linux/pull/4799#issuecomment-1958537212
> Fixes: 70317fd24b41 ("sysfs: Introduce a mechanism to hide static attribute_groups")
> Cc: Marc Herbert <marc.herbert@intel.com>

Ok, this one is now

Tested-by: Marc Herbert <marc.herbert@intel.com>

...per:

https://github.com/thesofproject/linux/pull/4799#issuecomment-1960469389

Thanks for all the help Marc!
Lukas Wunner April 22, 2024, 9:20 a.m. UTC | #4
On Thu, Feb 22, 2024 at 12:40:54PM -0800, Dan Williams wrote:
> It turns out that arch/x86/events/intel/core.c makes use of "empty"
> attributes.
> 
> 	static struct attribute *empty_attrs;
> 
> 	__init int intel_pmu_init(void)
> 	{
> 	        struct attribute **extra_skl_attr = &empty_attrs;
> 	        struct attribute **extra_attr = &empty_attrs;
> 	        struct attribute **td_attr    = &empty_attrs;
> 	        struct attribute **mem_attr   = &empty_attrs;
> 	        struct attribute **tsx_attr   = &empty_attrs;
> 		...
> 
> That breaks the assumption __first_visible() that expects that if
> grp->attrs is set then grp->attrs[0] must also be set and results in
> backtraces like:
> 
>     BUG: kernel NULL pointer dereference, address: 00rnel mode
>     #PF: error_code(0x0000) - not-present ] PREEMPT SMP NOPTI
>     CPU: 1 PID: 1 Comm: swapper/IP: 0010:exra_is_visible+0x14/0x20
>      ? exc_page_fault+0x68/0x190
>      internal_create_groups+0x42/0xa0
>      pmu_dev_alloc+0xc0/0xe0
>      perf_event_sysfs_init+0x580000000000 ]---
>     RIP: 0010:exra_is_visible+0x14/0
> 
> Check for non-empty attributes array before calling is_visible().
[...]
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -33,10 +33,10 @@ static void remove_files(struct kernfs_node *parent,
>  
>  static umode_t __first_visible(const struct attribute_group *grp, struct kobject *kobj)
>  {
> -	if (grp->attrs && grp->is_visible)
> +	if (grp->attrs && grp->attrs[0] && grp->is_visible)
>  		return grp->is_visible(kobj, grp->attrs[0], 0);
>  
> -	if (grp->bin_attrs && grp->is_bin_visible)
> +	if (grp->bin_attrs && grp->bin_attrs[0] && grp->is_bin_visible)
>  		return grp->is_bin_visible(kobj, grp->bin_attrs[0], 0);
>  
>  	return 0;

I'm wondering why 0 is returned by default and not SYSFS_GROUP_INVISIBLE.

An empty attribute list (containing just the NULL sentinel) will now
result in the attribute group being visible as an empty directory.

I thought the whole point was to hide such empty directories.

Was it a conscious decision to return 0?
Did you expect breakage if SYSFS_GROUP_INVISIBLE is returned?

Thanks,

Lukas
Dan Williams April 26, 2024, 5:59 p.m. UTC | #5
Lukas Wunner wrote:
[..]
> > --- a/fs/sysfs/group.c
> > +++ b/fs/sysfs/group.c
> > @@ -33,10 +33,10 @@ static void remove_files(struct kernfs_node *parent,
> >  
> >  static umode_t __first_visible(const struct attribute_group *grp, struct kobject *kobj)
> >  {
> > -	if (grp->attrs && grp->is_visible)
> > +	if (grp->attrs && grp->attrs[0] && grp->is_visible)
> >  		return grp->is_visible(kobj, grp->attrs[0], 0);
> >  
> > -	if (grp->bin_attrs && grp->is_bin_visible)
> > +	if (grp->bin_attrs && grp->bin_attrs[0] && grp->is_bin_visible)
> >  		return grp->is_bin_visible(kobj, grp->bin_attrs[0], 0);
> >  
> >  	return 0;
> 
> I'm wondering why 0 is returned by default and not SYSFS_GROUP_INVISIBLE.
> 
> An empty attribute list (containing just the NULL sentinel) will now
> result in the attribute group being visible as an empty directory.
> 
> I thought the whole point was to hide such empty directories.
> 
> Was it a conscious decision to return 0?

Perhaps there should be a comment here because yes, this is on purpose.

> Did you expect breakage if SYSFS_GROUP_INVISIBLE is returned?

Yes, the history is here:

    https://lore.kernel.org/all/YwZCPdPl2T+ndzjU@kroah.com/

...where an initial attempt to hide empty group directories resulted in
boot failures. The concern is that there might be user tooling that
depends on that empty directory. So the SYSFS_GROUP_INVISIBLE behavior
can only be enabled by explicit result from an is_visible() handler.

That way there is no regression potential for legacy cases where the
empty directory might matter.
Lukas Wunner April 26, 2024, 7:18 p.m. UTC | #6
On Fri, Apr 26, 2024 at 10:59:06AM -0700, Dan Williams wrote:
> Lukas Wunner wrote:
> > > --- a/fs/sysfs/group.c
> > > +++ b/fs/sysfs/group.c
> > > @@ -33,10 +33,10 @@ static void remove_files(struct kernfs_node *parent,
> > >  
> > >  static umode_t __first_visible(const struct attribute_group *grp, struct kobject *kobj)
> > >  {
> > > -	if (grp->attrs && grp->is_visible)
> > > +	if (grp->attrs && grp->attrs[0] && grp->is_visible)
> > >  		return grp->is_visible(kobj, grp->attrs[0], 0);
> > >  
> > > -	if (grp->bin_attrs && grp->is_bin_visible)
> > > +	if (grp->bin_attrs && grp->bin_attrs[0] && grp->is_bin_visible)
> > >  		return grp->is_bin_visible(kobj, grp->bin_attrs[0], 0);
> > >  
> > >  	return 0;
> > 
> > I'm wondering why 0 is returned by default and not SYSFS_GROUP_INVISIBLE.
> > 
> > An empty attribute list (containing just the NULL sentinel) will now
> > result in the attribute group being visible as an empty directory.
> > 
> > I thought the whole point was to hide such empty directories.
> > 
> > Was it a conscious decision to return 0?
> > Did you expect breakage if SYSFS_GROUP_INVISIBLE is returned?
> 
> Yes, the history is here:
> 
>     https://lore.kernel.org/all/YwZCPdPl2T+ndzjU@kroah.com/
> 
> ...where an initial attempt to hide empty group directories resulted in
> boot failures. The concern is that there might be user tooling that
> depends on that empty directory. So the SYSFS_GROUP_INVISIBLE behavior
> can only be enabled by explicit result from an is_visible() handler.
> 
> That way there is no regression potential for legacy cases where the
> empty directory might matter.

The problem is that no ->is_visible() or ->is_bin_visible() callback
is ever invoked for an empty attribute group.  So there is nothing
that could return SYSFS_GROUP_INVISIBLE.

It is thus impossible to hide them.

Even though an attribute group may be declared empty, attributes may
dynamically be added it to it using sysfs_add_file_to_group().

Case in point:  I'm declaring an empty attribute group named
"spdm_signatures_group" in this patch, to which attributes are
dynamically added:

https://github.com/l1k/linux/commit/ca420b22af05

Because it is impossible to hide the group, every PCI device exposes
it as an empty directory in sysfs, even if it doesn't support CMA
(PCI device authentication).

Fortunately the next patch in the series adds a single bin_attribute
"next_requester_nonce" to the attribute group.  Now I can suddenly
hide the group on devices incapable of CMA, because an
->is_bin_visible() callback is executed:

https://github.com/l1k/linux/commit/8248bc34630e

So in this case I'm able to dodge the bullet because the empty
signatures/ directory for CMA-incapable devices is only briefly
visible in the series.  Nobody will notice unless they apply
only a subset of the series.

But I want to raise awareness that the inability to hide
empty attribute groups feels awkward.

Thanks,

Lukas
Greg KH April 27, 2024, 11:05 a.m. UTC | #7
On Fri, Apr 26, 2024 at 09:18:15PM +0200, Lukas Wunner wrote:
> On Fri, Apr 26, 2024 at 10:59:06AM -0700, Dan Williams wrote:
> > Lukas Wunner wrote:
> > > > --- a/fs/sysfs/group.c
> > > > +++ b/fs/sysfs/group.c
> > > > @@ -33,10 +33,10 @@ static void remove_files(struct kernfs_node *parent,
> > > >  
> > > >  static umode_t __first_visible(const struct attribute_group *grp, struct kobject *kobj)
> > > >  {
> > > > -	if (grp->attrs && grp->is_visible)
> > > > +	if (grp->attrs && grp->attrs[0] && grp->is_visible)
> > > >  		return grp->is_visible(kobj, grp->attrs[0], 0);
> > > >  
> > > > -	if (grp->bin_attrs && grp->is_bin_visible)
> > > > +	if (grp->bin_attrs && grp->bin_attrs[0] && grp->is_bin_visible)
> > > >  		return grp->is_bin_visible(kobj, grp->bin_attrs[0], 0);
> > > >  
> > > >  	return 0;
> > > 
> > > I'm wondering why 0 is returned by default and not SYSFS_GROUP_INVISIBLE.
> > > 
> > > An empty attribute list (containing just the NULL sentinel) will now
> > > result in the attribute group being visible as an empty directory.
> > > 
> > > I thought the whole point was to hide such empty directories.
> > > 
> > > Was it a conscious decision to return 0?
> > > Did you expect breakage if SYSFS_GROUP_INVISIBLE is returned?
> > 
> > Yes, the history is here:
> > 
> >     https://lore.kernel.org/all/YwZCPdPl2T+ndzjU@kroah.com/
> > 
> > ...where an initial attempt to hide empty group directories resulted in
> > boot failures. The concern is that there might be user tooling that
> > depends on that empty directory. So the SYSFS_GROUP_INVISIBLE behavior
> > can only be enabled by explicit result from an is_visible() handler.
> > 
> > That way there is no regression potential for legacy cases where the
> > empty directory might matter.
> 
> The problem is that no ->is_visible() or ->is_bin_visible() callback
> is ever invoked for an empty attribute group.  So there is nothing
> that could return SYSFS_GROUP_INVISIBLE.
> 
> It is thus impossible to hide them.
> 
> Even though an attribute group may be declared empty, attributes may
> dynamically be added it to it using sysfs_add_file_to_group().
> 
> Case in point:  I'm declaring an empty attribute group named
> "spdm_signatures_group" in this patch, to which attributes are
> dynamically added:
> 
> https://github.com/l1k/linux/commit/ca420b22af05
> 
> Because it is impossible to hide the group, every PCI device exposes
> it as an empty directory in sysfs, even if it doesn't support CMA
> (PCI device authentication).
> 
> Fortunately the next patch in the series adds a single bin_attribute
> "next_requester_nonce" to the attribute group.  Now I can suddenly
> hide the group on devices incapable of CMA, because an
> ->is_bin_visible() callback is executed:
> 
> https://github.com/l1k/linux/commit/8248bc34630e
> 
> So in this case I'm able to dodge the bullet because the empty
> signatures/ directory for CMA-incapable devices is only briefly
> visible in the series.  Nobody will notice unless they apply
> only a subset of the series.
> 
> But I want to raise awareness that the inability to hide
> empty attribute groups feels awkward.

It does, but that's because we can't break existing systems :)

Documenting this to be more obvious would be great, I'll glady take
changes for that as I agree, the implementation is "tricky" and took me
a long time to review/understand it as well, as it is complex to deal
with (and I thank Dan for getting it all working properly, I had tried
and failed...)

thanks,

greg k-h
Dan Williams April 27, 2024, 4:49 p.m. UTC | #8
Lukas Wunner wrote:
[..]
> So in this case I'm able to dodge the bullet because the empty
> signatures/ directory for CMA-incapable devices is only briefly
> visible in the series.  Nobody will notice unless they apply
> only a subset of the series.
> 
> But I want to raise awareness that the inability to hide
> empty attribute groups feels awkward.

That is fair, it was definitely some gymnastics to only change user
visible behavior for new "invisible aware" attribute groups that opt-in
while leaving all the legacy cases alone.

The concern is knowing when it is ok to call an is_visible() callback
with a NULL @attr argument, or knowing when an empty array actually
means "hide the group directory".

We could add a sentinel value to indicate "I am an empty attribute list
*AND* I want my directory hidden by default". However, that's almost
identical to requiring a placeholder attribute in the list just to make
__first_visible() happy.

Other ideas? I expect this issue to come up again because dynamically
populated attribute arrays of a statically defined group is a useful
mechanism. I might need this in the PCI TSM enabling... but having at
least one attribute there is likely not a problem.
Lukas Wunner April 27, 2024, 9:14 p.m. UTC | #9
On Sat, Apr 27, 2024 at 09:49:41AM -0700, Dan Williams wrote:
> Lukas Wunner wrote:
> > But I want to raise awareness that the inability to hide
> > empty attribute groups feels awkward.
> 
> That is fair, it was definitely some gymnastics to only change user
> visible behavior for new "invisible aware" attribute groups that opt-in
> while leaving all the legacy cases alone.
> 
> The concern is knowing when it is ok to call an is_visible() callback
> with a NULL @attr argument, or knowing when an empty array actually
> means "hide the group directory".
> 
> We could add a sentinel value to indicate "I am an empty attribute list
> *AND* I want my directory hidden by default". However, that's almost
> identical to requiring a placeholder attribute in the list just to make
> __first_visible() happy.
> 
> Other ideas?

Perhaps an optional ->is_group_visible() callback in struct attribute_group
which gets passed only the struct kobject pointer?

At least for PCI device authentication, that would be sufficient.
I could get from the kobject to the corresponding struct device,
then determine whether the device supports authentication or not.

Because it's a new, optional callback, there should be no compatibility
issues.  The SYSFS_GROUP_INVISIBLE return code from the ->is_visible()
call for individual attributes would not be needed then, at least in my
use case.

Thanks,

Lukas
Dan Williams April 27, 2024, 9:33 p.m. UTC | #10
Lukas Wunner wrote:
> On Sat, Apr 27, 2024 at 09:49:41AM -0700, Dan Williams wrote:
> > Lukas Wunner wrote:
> > > But I want to raise awareness that the inability to hide
> > > empty attribute groups feels awkward.
> > 
> > That is fair, it was definitely some gymnastics to only change user
> > visible behavior for new "invisible aware" attribute groups that opt-in
> > while leaving all the legacy cases alone.
> > 
> > The concern is knowing when it is ok to call an is_visible() callback
> > with a NULL @attr argument, or knowing when an empty array actually
> > means "hide the group directory".
> > 
> > We could add a sentinel value to indicate "I am an empty attribute list
> > *AND* I want my directory hidden by default". However, that's almost
> > identical to requiring a placeholder attribute in the list just to make
> > __first_visible() happy.
> > 
> > Other ideas?
> 
> Perhaps an optional ->is_group_visible() callback in struct attribute_group
> which gets passed only the struct kobject pointer?
> 
> At least for PCI device authentication, that would be sufficient.
> I could get from the kobject to the corresponding struct device,
> then determine whether the device supports authentication or not.
> 
> Because it's a new, optional callback, there should be no compatibility
> issues.  The SYSFS_GROUP_INVISIBLE return code from the ->is_visible()
> call for individual attributes would not be needed then, at least in my
> use case.

That's where I started with this, but decided it was overkill to
increase the size of that data structure globally for a small number of
use cases.
Dan Williams April 27, 2024, 10:39 p.m. UTC | #11
Dan Williams wrote:
> Lukas Wunner wrote:
> > On Sat, Apr 27, 2024 at 09:49:41AM -0700, Dan Williams wrote:
> > > Lukas Wunner wrote:
> > > > But I want to raise awareness that the inability to hide
> > > > empty attribute groups feels awkward.
> > > 
> > > That is fair, it was definitely some gymnastics to only change user
> > > visible behavior for new "invisible aware" attribute groups that opt-in
> > > while leaving all the legacy cases alone.
> > > 
> > > The concern is knowing when it is ok to call an is_visible() callback
> > > with a NULL @attr argument, or knowing when an empty array actually
> > > means "hide the group directory".
> > > 
> > > We could add a sentinel value to indicate "I am an empty attribute list
> > > *AND* I want my directory hidden by default". However, that's almost
> > > identical to requiring a placeholder attribute in the list just to make
> > > __first_visible() happy.
> > > 
> > > Other ideas?
> > 
> > Perhaps an optional ->is_group_visible() callback in struct attribute_group
> > which gets passed only the struct kobject pointer?
> > 
> > At least for PCI device authentication, that would be sufficient.
> > I could get from the kobject to the corresponding struct device,
> > then determine whether the device supports authentication or not.
> > 
> > Because it's a new, optional callback, there should be no compatibility
> > issues.  The SYSFS_GROUP_INVISIBLE return code from the ->is_visible()
> > call for individual attributes would not be needed then, at least in my
> > use case.
> 
> That's where I started with this, but decided it was overkill to
> increase the size of that data structure globally for a small number of
> use cases.

Perhaps could try something like this:

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index d22ad67a0f32..f4054cf08e58 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -33,11 +33,23 @@ static void remove_files(struct kernfs_node *parent,
 
 static umode_t __first_visible(const struct attribute_group *grp, struct kobject *kobj)
 {
-       if (grp->attrs && grp->attrs[0] && grp->is_visible)
-               return grp->is_visible(kobj, grp->attrs[0], 0);
+       if (grp->attrs && grp->is_visible) {
+               struct attribute *attr = grp->attrs[0];
+               struct attribute empty_attr = { 0 };
 
-       if (grp->bin_attrs && grp->bin_attrs[0] && grp->is_bin_visible)
-               return grp->is_bin_visible(kobj, grp->bin_attrs[0], 0);
+               if (!attr)
+                       attr = &empty_attr;
+               return grp->is_visible(kobj, attr, 0);
+       }
+
+       if (grp->bin_attrs && grp->is_bin_visible) {
+               struct bin_attribute *bin_attr = grp->bin_attrs[0];
+               struct bin_attribute empty_bin_attr = { 0 };
+
+               if (!bin_attr)
+                       bin_attr = &empty_bin_attr;
+               return grp->is_bin_visible(kobj, bin_attr, 0);
+       }
 
        return 0;
 }

...because it is highly likely that existing is_visible() callers will
return @attr->mode when they do not recognize the attribute. But this
could lead to some subtle bugs if something only checks the attribute
index value. For example:

lbr_is_visible(struct kobject *kobj, struct attribute *attr, int i)
{
        /* branches */
        if (i == 0)
                return x86_pmu.lbr_nr ? attr->mode : 0;

        return (x86_pmu.flags & PMU_FL_BR_CNTR) ? attr->mode : 0;
}

...but in this case we get lucky because it would return attr->mode
which is 0.
Dan Williams April 27, 2024, 11:09 p.m. UTC | #12
Dan Williams wrote:
> Dan Williams wrote:
> > Lukas Wunner wrote:
> > > On Sat, Apr 27, 2024 at 09:49:41AM -0700, Dan Williams wrote:
> > > > Lukas Wunner wrote:
> > > > > But I want to raise awareness that the inability to hide
> > > > > empty attribute groups feels awkward.
> > > > 
> > > > That is fair, it was definitely some gymnastics to only change user
> > > > visible behavior for new "invisible aware" attribute groups that opt-in
> > > > while leaving all the legacy cases alone.
> > > > 
> > > > The concern is knowing when it is ok to call an is_visible() callback
> > > > with a NULL @attr argument, or knowing when an empty array actually
> > > > means "hide the group directory".
> > > > 
> > > > We could add a sentinel value to indicate "I am an empty attribute list
> > > > *AND* I want my directory hidden by default". However, that's almost
> > > > identical to requiring a placeholder attribute in the list just to make
> > > > __first_visible() happy.
> > > > 
> > > > Other ideas?
> > > 
> > > Perhaps an optional ->is_group_visible() callback in struct attribute_group
> > > which gets passed only the struct kobject pointer?
> > > 
> > > At least for PCI device authentication, that would be sufficient.
> > > I could get from the kobject to the corresponding struct device,
> > > then determine whether the device supports authentication or not.
> > > 
> > > Because it's a new, optional callback, there should be no compatibility
> > > issues.  The SYSFS_GROUP_INVISIBLE return code from the ->is_visible()
> > > call for individual attributes would not be needed then, at least in my
> > > use case.
> > 
> > That's where I started with this, but decided it was overkill to
> > increase the size of that data structure globally for a small number of
> > use cases.
> 
> Perhaps could try something like this:
> 
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index d22ad67a0f32..f4054cf08e58 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -33,11 +33,23 @@ static void remove_files(struct kernfs_node *parent,
>  
>  static umode_t __first_visible(const struct attribute_group *grp, struct kobject *kobj)
>  {
> -       if (grp->attrs && grp->attrs[0] && grp->is_visible)
> -               return grp->is_visible(kobj, grp->attrs[0], 0);
> +       if (grp->attrs && grp->is_visible) {
> +               struct attribute *attr = grp->attrs[0];
> +               struct attribute empty_attr = { 0 };
>  
> -       if (grp->bin_attrs && grp->bin_attrs[0] && grp->is_bin_visible)
> -               return grp->is_bin_visible(kobj, grp->bin_attrs[0], 0);
> +               if (!attr)
> +                       attr = &empty_attr;
> +               return grp->is_visible(kobj, attr, 0);
> +       }
> +
> +       if (grp->bin_attrs && grp->is_bin_visible) {
> +               struct bin_attribute *bin_attr = grp->bin_attrs[0];
> +               struct bin_attribute empty_bin_attr = { 0 };
> +
> +               if (!bin_attr)
> +                       bin_attr = &empty_bin_attr;
> +               return grp->is_bin_visible(kobj, bin_attr, 0);
> +       }
>  
>         return 0;
>  }
> 
> ...because it is highly likely that existing is_visible() callers will
> return @attr->mode when they do not recognize the attribute. But this
> could lead to some subtle bugs if something only checks the attribute
> index value. For example:
> 
> lbr_is_visible(struct kobject *kobj, struct attribute *attr, int i)
> {
>         /* branches */
>         if (i == 0)
>                 return x86_pmu.lbr_nr ? attr->mode : 0;
> 
>         return (x86_pmu.flags & PMU_FL_BR_CNTR) ? attr->mode : 0;
> }
> 
> ...but in this case we get lucky because it would return attr->mode
> which is 0.

Oh, but if an is_visible() callback gets confused by empty_attr, that's
detectable:

    mode = grp->is_visible(kobj, attr, 0);
    if ((mode & ~SYSFS_GROUP_INVISIBLE) && attr == empty_attr)
            return 0;
           
...i.e. the only acceptable responses to an empty_attr check is 0 or
~SYSFS_GROUP_INVISIBLE.
Lukas Wunner April 28, 2024, 10:08 a.m. UTC | #13
On Sat, Apr 27, 2024 at 02:33:24PM -0700, Dan Williams wrote:
> Lukas Wunner wrote:
> > Perhaps an optional ->is_group_visible() callback in struct attribute_group
> > which gets passed only the struct kobject pointer?
> > 
> > At least for PCI device authentication, that would be sufficient.
> > I could get from the kobject to the corresponding struct device,
> > then determine whether the device supports authentication or not.
> > 
> > Because it's a new, optional callback, there should be no compatibility
> > issues.  The SYSFS_GROUP_INVISIBLE return code from the ->is_visible()
> > call for individual attributes would not be needed then, at least in my
> > use case.
> 
> That's where I started with this, but decided it was overkill to
> increase the size of that data structure globally for a small number of
> use cases.

Memory is cheap and memory-constrained devices can set CONFIG_SYSFS=n.

There aren't that many struct attribute_groups and this is just
8 additional bytes on a 64-bit machine.  (There are way more
struct attribute than struct attribute_group.)  The contortions
necessary to overload individual attribute ->is_visible() callbacks
to also govern the group's visibility aren't worth it.

Having an ->is_group_visible() callback has the additional benefit that
the mode of directories no longer needs to be hardcoded to 0755 in
sysfs_create_dir_ns(), but can be set to, say, 0500 or 0700 or 0511,
depending on the use case.  So more flexibility there as well.

Thanks,

Lukas
Dan Williams April 29, 2024, 5:47 p.m. UTC | #14
Lukas Wunner wrote:
> On Sat, Apr 27, 2024 at 02:33:24PM -0700, Dan Williams wrote:
> > Lukas Wunner wrote:
> > > Perhaps an optional ->is_group_visible() callback in struct attribute_group
> > > which gets passed only the struct kobject pointer?
> > > 
> > > At least for PCI device authentication, that would be sufficient.
> > > I could get from the kobject to the corresponding struct device,
> > > then determine whether the device supports authentication or not.
> > > 
> > > Because it's a new, optional callback, there should be no compatibility
> > > issues.  The SYSFS_GROUP_INVISIBLE return code from the ->is_visible()
> > > call for individual attributes would not be needed then, at least in my
> > > use case.
> > 
> > That's where I started with this, but decided it was overkill to
> > increase the size of that data structure globally for a small number of
> > use cases.
> 
> Memory is cheap and memory-constrained devices can set CONFIG_SYSFS=n.

That sounds severe, but point taken that someone could config-off the
cases that need this extension.

> There aren't that many struct attribute_groups and this is just
> 8 additional bytes on a 64-bit machine.  (There are way more
> struct attribute than struct attribute_group.)  The contortions
> necessary to overload individual attribute ->is_visible() callbacks
> to also govern the group's visibility aren't worth it.

I agree that most systems would not care about growing this structure,
but the same is true for almost all other data structure growth in the
kernel. It is a typical kernel pastime to squeeze functionality into
existing data structures.

> Having an ->is_group_visible() callback has the additional benefit that
> the mode of directories no longer needs to be hardcoded to 0755 in
> sysfs_create_dir_ns(), but can be set to, say, 0500 or 0700 or 0511,
> depending on the use case.  So more flexibility there as well.

Unnecessary growth is unnecessary growth. In this case all the known use
cases can use the SYSFS_GROUP_INVISIBLE flag returned from is_visible().
The awkwardness around cases that want to have an empty attributes array
and invisible group directory is noted and puts the solution on notice
for running afoul of the sunk cost fallacy in the future.
diff mbox series

Patch

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index ccb275cdabcb..8c63ba3cfc47 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -33,10 +33,10 @@  static void remove_files(struct kernfs_node *parent,
 
 static umode_t __first_visible(const struct attribute_group *grp, struct kobject *kobj)
 {
-	if (grp->attrs && grp->is_visible)
+	if (grp->attrs && grp->attrs[0] && grp->is_visible)
 		return grp->is_visible(kobj, grp->attrs[0], 0);
 
-	if (grp->bin_attrs && grp->is_bin_visible)
+	if (grp->bin_attrs && grp->bin_attrs[0] && grp->is_bin_visible)
 		return grp->is_bin_visible(kobj, grp->bin_attrs[0], 0);
 
 	return 0;