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

[Greg KH <gregkh@linuxfoundation.org>]

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