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

[Lukas Wunner <lukas@wunner.de>]

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