[PATCH 0/3] sysfs: Group visibility fixups

[PATCH 0/3] sysfs: Group visibility fixups

[Dan Williams]

Hey Greg,

Marc was able to get me a backtrace for that bootup hang scenario that
Pierre noted here [1]. A Tested-by is still pending, but I am certain
this is the issue, and it may impact more than just the one platform if
that "empty_attrs" pattern has been repeated anywhere else in the
kernel.

I also took some time to document how to use the helpers a bit better,
and introduce DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE() for cases where all
that matters is group visibility and not per attribute visibility.

[1]: http://lore.kernel.org/r/b93ec9c2-23f5-486b-a3dc-ed9b960df359@linux.intel.com

---

Dan Williams (3):
      sysfs: Fix crash on empty group attributes array
      sysfs: Document new "group visible" helpers
      sysfs: Introduce DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE()


 fs/sysfs/group.c      |    4 +-
 include/linux/sysfs.h |   85 +++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 84 insertions(+), 5 deletions(-)

base-commit: 70317fd24b419091aa0a6dc3ea3ec7bb50c37c32

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

[Dan Williams]

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(-)

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;

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

[Rafael J. Wysocki]

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;
>

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

[Dan Williams]

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

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

[Dan Williams]

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!

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

[Lukas Wunner]

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

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

[Dan Williams]

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.

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

[Lukas Wunner]

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

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

[Greg KH]

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

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

[Dan Williams]

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.

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

[Lukas Wunner]

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

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

[Dan Williams]

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.

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

[Dan Williams]

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.

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

[Dan Williams]

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.

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

[Lukas Wunner]

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

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

[Dan Williams]

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.

[PATCH 2/3] sysfs: Document new "group visible" helpers

[Dan Williams]

Add documentation and examples for how to use
DEFINE_SYSFS_GROUP_VISIBLE() and SYSFS_GROUP_VISIBLE(). Recall that the
motivation for this work is that it is easier to reason about the
lifetime of statically defined sysfs attributes that become visible at
device_add() time rather than dynamically adding them later.
DEFINE_SYSFS_GROUP_VISIBLE() tackles one of the reasons to opt for
dynamically created attributes which did not have a facility for hiding
empty directories.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 include/linux/sysfs.h |   42 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index a42642b277dd..dabf7f4f3581 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -105,8 +105,42 @@ struct attribute_group {
 #define SYSFS_GROUP_INVISIBLE	020000
 
 /*
- * The first call to is_visible() in the create / update path may
- * indicate visibility for the entire group
+ * DEFINE_SYSFS_GROUP_VISIBLE(name):
+ *	A helper macro to pair with the assignment of ".is_visible =
+ *	SYSFS_GROUP_VISIBLE(name)", that arranges for the directory
+ *	associated with a named attribute_group to optionally be hidden.
+ *	This allows for static declaration of attribute_groups, and the
+ *	simplification of attribute visibility lifetime that implies,
+ *	without polluting sysfs with empty attribute directories.
+ * Ex.
+ *
+ * static umode_t example_attr_visible(struct kobject *kobj,
+ *                                   struct attribute *attr, int n)
+ * {
+ *       if (example_attr_condition)
+ *               return 0;
+ *       else if (ro_attr_condition)
+ *               return 0444;
+ *       return a->mode;
+ * }
+ *
+ * static bool example_group_visible(struct kobject *kobj)
+ * {
+ *       if (example_group_condition)
+ *               return false;
+ *       return true;
+ * }
+ *
+ * DEFINE_SYSFS_GROUP_VISIBLE(example);
+ *
+ * static struct attribute_group example_group = {
+ *       .name = "example",
+ *       .is_visible = SYSFS_GROUP_VISIBLE(example),
+ *       .attrs = &example_attrs,
+ * };
+ *
+ * Note that it expects <name>_attr_visible and <name>_group_visible to
+ * be defined.
  */
 #define DEFINE_SYSFS_GROUP_VISIBLE(name)                             \
 	static inline umode_t sysfs_group_visible_##name(            \
@@ -119,7 +153,9 @@ struct attribute_group {
 
 /*
  * Same as DEFINE_SYSFS_GROUP_VISIBLE, but for groups with only binary
- * attributes
+ * attributes. If an attribute_group defines both text and binary
+ * attributes, the group visibility is determined by the function
+ * specified to is_visible() not is_bin_visible()
  */
 #define DEFINE_SYSFS_BIN_GROUP_VISIBLE(name)                             \
 	static inline umode_t sysfs_group_visible_##name(                \

[PATCH 3/3] sysfs: Introduce DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE()

[Dan Williams]

One of the first users of DEFINE_SYSFS_GROUP_VISIBLE() did this:

	static umode_t dp0_attr_visible(struct kobject *kobj,
					struct attribute *attr,
					int n)
	{
		struct sdw_slave *slave = dev_to_sdw_dev(kobj_to_dev(kobj));

		if (slave->prop.dp0_prop)
			return attr->mode;
		return 0;
	}

	static bool dp0_group_visible(struct kobject *kobj)
	{
		struct sdw_slave *slave = dev_to_sdw_dev(kobj_to_dev(kobj));

		if (slave->prop.dp0_prop)
			return true;
		return false;
	}
	DEFINE_SYSFS_GROUP_VISIBLE(dp0);

...i.e. the _group_visible() helper is identical to the _attr_visible()
helper. Use the "simple" helper to reduce that to:

	static bool dp0_group_visible(struct kobject *kobj)
	{
		struct sdw_slave *slave = dev_to_sdw_dev(kobj_to_dev(kobj));

		if (slave->prop.dp0_prop)
			return true;
		return false;
	}
	DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(dp0);

Remove the need to specify per attribute visibility if the goal is to
hide the entire group.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 include/linux/sysfs.h |   45 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index dabf7f4f3581..326341c62385 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -140,7 +140,9 @@ struct attribute_group {
  * };
  *
  * Note that it expects <name>_attr_visible and <name>_group_visible to
- * be defined.
+ * be defined. For cases where individual attributes do not need
+ * separate visibility consideration, only entire group visibility at
+ * once, see DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE().
  */
 #define DEFINE_SYSFS_GROUP_VISIBLE(name)                             \
 	static inline umode_t sysfs_group_visible_##name(            \
@@ -151,6 +153,38 @@ struct attribute_group {
 		return name##_attr_visible(kobj, attr, n);           \
 	}
 
+/*
+ * DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(name):
+ *	A helper macro to pair with SYSFS_GROUP_VISIBLE() that like
+ *	DEFINE_SYSFS_GROUP_VISIBLE() controls group visibility, but does
+ *	not require the implementation of a per-attribute visibility
+ *	callback.
+ * Ex.
+ *
+ * static bool example_group_visible(struct kobject *kobj)
+ * {
+ *       if (example_group_condition)
+ *               return false;
+ *       return true;
+ * }
+ *
+ * DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(example);
+ *
+ * static struct attribute_group example_group = {
+ *       .name = "example",
+ *       .is_visible = SYSFS_GROUP_VISIBLE(example),
+ *       .attrs = &example_attrs,
+ * };
+ */
+#define DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(name)                   \
+	static inline umode_t sysfs_group_visible_##name(         \
+		struct kobject *kobj, struct attribute *a, int n) \
+	{                                                         \
+		if (n == 0 && !name##_group_visible(kobj))        \
+			return SYSFS_GROUP_INVISIBLE;             \
+		return a->mode;                                   \
+	}
+
 /*
  * Same as DEFINE_SYSFS_GROUP_VISIBLE, but for groups with only binary
  * attributes. If an attribute_group defines both text and binary
@@ -166,6 +200,15 @@ struct attribute_group {
 		return name##_attr_visible(kobj, attr, n);               \
 	}
 
+#define DEFINE_SIMPLE_SYSFS_BIN_GROUP_VISIBLE(name)                   \
+	static inline umode_t sysfs_group_visible_##name(             \
+		struct kobject *kobj, struct bin_attribute *a, int n) \
+	{                                                             \
+		if (n == 0 && !name##_group_visible(kobj))            \
+			return SYSFS_GROUP_INVISIBLE;                 \
+		return a->mode;                                       \
+	}
+
 #define SYSFS_GROUP_VISIBLE(fn) sysfs_group_visible_##fn
 
 /*

Re: [PATCH 0/3] sysfs: Group visibility fixups

[Greg KH]

On Thu, Feb 22, 2024 at 12:40:48PM -0800, Dan Williams wrote:
> Hey Greg,
> 
> Marc was able to get me a backtrace for that bootup hang scenario that
> Pierre noted here [1]. A Tested-by is still pending, but I am certain
> this is the issue, and it may impact more than just the one platform if
> that "empty_attrs" pattern has been repeated anywhere else in the
> kernel.
> 
> I also took some time to document how to use the helpers a bit better,
> and introduce DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE() for cases where all
> that matters is group visibility and not per attribute visibility.
> 
> [1]: http://lore.kernel.org/r/b93ec9c2-23f5-486b-a3dc-ed9b960df359@linux.intel.com

I'll just queue these up now on my normal driver-core-next branch, and
not worry about a stable tag as I doubt anyone wants that now.  But if
they do, please let me know and I can provide one.

thanks for the quick fixes!

greg k-h