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@xxxxxxxxx/
> >
> > ...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
