From: Greg KH <gregkh@linuxfoundation.org>
To: Lukas Wunner <lukas@wunner.de>
Cc: Dan Williams <dan.j.williams@intel.com>,
Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
Marc Herbert <marc.herbert@intel.com>,
"Rafael J. Wysocki" <rafael@kernel.org>,
linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,
linux-coco@lists.linux.dev, alsa-devel@alsa-project.org
Subject: Re: [PATCH 1/3] sysfs: Fix crash on empty group attributes array
Date: Sat, 27 Apr 2024 13:05:50 +0200 [thread overview]
Message-ID: <2024042748-campus-okay-ffff@gregkh> (raw)
In-Reply-To: <Ziv9984CJeQ4muZy@wunner.de>
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
next prev parent reply other threads:[~2024-04-27 11:06 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-22 20:40 [PATCH 0/3] sysfs: Group visibility fixups Dan Williams
2024-02-22 20:40 ` [PATCH 1/3] sysfs: Fix crash on empty group attributes array Dan Williams
2024-02-22 21:14 ` Rafael J. Wysocki
2024-02-22 22:03 ` Dan Williams
2024-02-22 23:15 ` Dan Williams
2024-04-22 9:20 ` Lukas Wunner
2024-04-26 17:59 ` Dan Williams
2024-04-26 19:18 ` Lukas Wunner
2024-04-27 11:05 ` Greg KH [this message]
2024-04-27 16:49 ` Dan Williams
2024-04-27 21:14 ` Lukas Wunner
2024-04-27 21:33 ` Dan Williams
2024-04-27 22:39 ` Dan Williams
2024-04-27 23:09 ` Dan Williams
2024-04-28 10:08 ` Lukas Wunner
2024-04-29 17:47 ` Dan Williams
2024-02-22 20:41 ` [PATCH 2/3] sysfs: Document new "group visible" helpers Dan Williams
2024-02-22 20:41 ` [PATCH 3/3] sysfs: Introduce DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE() Dan Williams
2024-02-23 6:33 ` [PATCH 0/3] sysfs: Group visibility fixups Greg KH
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2024042748-campus-okay-ffff@gregkh \
--to=gregkh@linuxfoundation.org \
--cc=alsa-devel@alsa-project.org \
--cc=dan.j.williams@intel.com \
--cc=linux-coco@lists.linux.dev \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=lukas@wunner.de \
--cc=marc.herbert@intel.com \
--cc=pierre-louis.bossart@linux.intel.com \
--cc=rafael@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).