It turns out that arch/x86/events/intel/core.c makes use of "empty" attributes.

static struct attribute *empty_attrs;

__init int intel_pmu_init(void) { struct attribute **extra_skl_attr = &empty_attrs; struct attribute **extra_attr = &empty_attrs; struct attribute **td_attr = &empty_attrs; struct attribute **mem_attr = &empty_attrs; struct attribute **tsx_attr = &empty_attrs; ...

That breaks the assumption __first_visible() that expects that if grp->attrs is set then grp->attrs[0] must also be set and results in backtraces like:

BUG: kernel NULL pointer dereference, address: 00rnel mode #PF: error_code(0x0000) - not-present ] PREEMPT SMP NOPTI CPU: 1 PID: 1 Comm: swapper/IP: 0010:exra_is_visible+0x14/0x20 ? exc_page_fault+0x68/0x190 internal_create_groups+0x42/0xa0 pmu_dev_alloc+0xc0/0xe0 perf_event_sysfs_init+0x580000000000 ]--- RIP: 0010:exra_is_visible+0x14/0

Check for non-empty attributes array before calling is_visible().

Reported-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Closes: https://github.com/thesofproject/linux/pull/4799#issuecomment-1958537212 Fixes: 70317fd24b41 ("sysfs: Introduce a mechanism to hide static attribute_groups") Cc: Marc Herbert marc.herbert@intel.com Cc: "Rafael J. Wysocki" rafael@kernel.org Signed-off-by: Dan Williams dan.j.williams@intel.com --- fs/sysfs/group.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c index ccb275cdabcb..8c63ba3cfc47 100644 --- 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;

One of the first users of DEFINE_SYSFS_GROUP_VISIBLE() did this:

static umode_t dp0_attr_visible(struct kobject *kobj, struct attribute *attr, int n) { struct sdw_slave *slave = dev_to_sdw_dev(kobj_to_dev(kobj));

if (slave->prop.dp0_prop) return attr->mode; return 0; }

static bool dp0_group_visible(struct kobject *kobj) { struct sdw_slave *slave = dev_to_sdw_dev(kobj_to_dev(kobj));

if (slave->prop.dp0_prop) return true; return false; } DEFINE_SYSFS_GROUP_VISIBLE(dp0);

...i.e. the _group_visible() helper is identical to the _attr_visible() helper. Use the "simple" helper to reduce that to:

static bool dp0_group_visible(struct kobject *kobj) { struct sdw_slave *slave = dev_to_sdw_dev(kobj_to_dev(kobj));

if (slave->prop.dp0_prop) return true; return false; } DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(dp0);

Remove the need to specify per attribute visibility if the goal is to hide the entire group.

Signed-off-by: Dan Williams dan.j.williams@intel.com --- include/linux/sysfs.h | 45 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h index dabf7f4f3581..326341c62385 100644 --- a/include/linux/sysfs.h +++ b/include/linux/sysfs.h @@ -140,7 +140,9 @@ struct attribute_group { * }; * * Note that it expects <name>_attr_visible and <name>_group_visible to - * be defined. + * be defined. For cases where individual attributes do not need + * separate visibility consideration, only entire group visibility at + * once, see DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(). */ #define DEFINE_SYSFS_GROUP_VISIBLE(name) \ static inline umode_t sysfs_group_visible_##name( \ @@ -151,6 +153,38 @@ struct attribute_group { return name##_attr_visible(kobj, attr, n); \ }

+/* + * DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(name): + * A helper macro to pair with SYSFS_GROUP_VISIBLE() that like + * DEFINE_SYSFS_GROUP_VISIBLE() controls group visibility, but does + * not require the implementation of a per-attribute visibility + * callback. + * Ex. + * + * static bool example_group_visible(struct kobject *kobj) + * { + * if (example_group_condition) + * return false; + * return true; + * } + * + * DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(example); + * + * static struct attribute_group example_group = { + * .name = "example", + * .is_visible = SYSFS_GROUP_VISIBLE(example), + * .attrs = &example_attrs, + * }; + */ +#define DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(name) \ + static inline umode_t sysfs_group_visible_##name( \ + struct kobject *kobj, struct attribute *a, int n) \ + { \ + if (n == 0 && !name##_group_visible(kobj)) \ + return SYSFS_GROUP_INVISIBLE; \ + return a->mode; \ + } + /* * Same as DEFINE_SYSFS_GROUP_VISIBLE, but for groups with only binary * attributes. If an attribute_group defines both text and binary @@ -166,6 +200,15 @@ struct attribute_group { return name##_attr_visible(kobj, attr, n); \ }

+#define DEFINE_SIMPLE_SYSFS_BIN_GROUP_VISIBLE(name) \ + static inline umode_t sysfs_group_visible_##name( \ + struct kobject *kobj, struct bin_attribute *a, int n) \ + { \ + if (n == 0 && !name##_group_visible(kobj)) \ + return SYSFS_GROUP_INVISIBLE; \ + return a->mode; \ + } + #define SYSFS_GROUP_VISIBLE(fn) sysfs_group_visible_##fn

/*

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?

Perhaps there should be a comment here because yes, this is on purpose.

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.

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

Dan Williams wrote:

Lukas Wunner wrote:

...

On Sat, Apr 27, 2024 at 09:49:41AM -0700, Dan Williams wrote:

...

Lukas Wunner wrote:

...

But I want to raise awareness that the inability to hide empty attribute groups feels awkward.

That is fair, it was definitely some gymnastics to only change user visible behavior for new "invisible aware" attribute groups that opt-in while leaving all the legacy cases alone.

The concern is knowing when it is ok to call an is_visible() callback with a NULL @attr argument, or knowing when an empty array actually means "hide the group directory".

We could add a sentinel value to indicate "I am an empty attribute list *AND* I want my directory hidden by default". However, that's almost identical to requiring a placeholder attribute in the list just to make __first_visible() happy.

Other ideas?

Perhaps an optional ->is_group_visible() callback in struct attribute_group which gets passed only the struct kobject pointer?

At least for PCI device authentication, that would be sufficient. I could get from the kobject to the corresponding struct device, then determine whether the device supports authentication or not.

Because it's a new, optional callback, there should be no compatibility issues. The SYSFS_GROUP_INVISIBLE return code from the ->is_visible() call for individual attributes would not be needed then, at least in my use case.

That's where I started with this, but decided it was overkill to increase the size of that data structure globally for a small number of use cases.

Perhaps could try something like this:

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c index d22ad67a0f32..f4054cf08e58 100644 --- a/fs/sysfs/group.c +++ b/fs/sysfs/group.c @@ -33,11 +33,23 @@ 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->attrs[0] && grp->is_visible) - return grp->is_visible(kobj, grp->attrs[0], 0); + if (grp->attrs && grp->is_visible) { + struct attribute *attr = grp->attrs[0]; + struct attribute empty_attr = { 0 };

- if (grp->bin_attrs && grp->bin_attrs[0] && grp->is_bin_visible) - return grp->is_bin_visible(kobj, grp->bin_attrs[0], 0); + if (!attr) + attr = &empty_attr; + return grp->is_visible(kobj, attr, 0); + } + + if (grp->bin_attrs && grp->is_bin_visible) { + struct bin_attribute *bin_attr = grp->bin_attrs[0]; + struct bin_attribute empty_bin_attr = { 0 }; + + if (!bin_attr) + bin_attr = &empty_bin_attr; + return grp->is_bin_visible(kobj, bin_attr, 0); + }

return 0; }

...because it is highly likely that existing is_visible() callers will return @attr->mode when they do not recognize the attribute. But this could lead to some subtle bugs if something only checks the attribute index value. For example:

lbr_is_visible(struct kobject *kobj, struct attribute *attr, int i) { /* branches */ if (i == 0) return x86_pmu.lbr_nr ? attr->mode : 0;

return (x86_pmu.flags & PMU_FL_BR_CNTR) ? attr->mode : 0; }

...but in this case we get lucky because it would return attr->mode which is 0.