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