[PATCH 0/3] sysfs: Group visibility fixups
Hey Greg,
Marc was able to get me a backtrace for that bootup hang scenario that Pierre noted here [1]. A Tested-by is still pending, but I am certain this is the issue, and it may impact more than just the one platform if that "empty_attrs" pattern has been repeated anywhere else in the kernel.
I also took some time to document how to use the helpers a bit better, and introduce DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE() for cases where all that matters is group visibility and not per attribute visibility.
[1]: http://lore.kernel.org/r/b93ec9c2-23f5-486b-a3dc-ed9b960df359@linux.intel.co...
---
Dan Williams (3): sysfs: Fix crash on empty group attributes array sysfs: Document new "group visible" helpers sysfs: Introduce DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE()
fs/sysfs/group.c | 4 +- include/linux/sysfs.h | 85 +++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 84 insertions(+), 5 deletions(-)
base-commit: 70317fd24b419091aa0a6dc3ea3ec7bb50c37c32
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;
On Thu, Feb 22, 2024 at 9:40 PM Dan Williams dan.j.williams@intel.com wrote:
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/0Check 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")
This is not in the mainline, so linux-next I suppose?
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;
Rafael J. Wysocki wrote:
On Thu, Feb 22, 2024 at 9:40 PM Dan Williams dan.j.williams@intel.com wrote:
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/0Check 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")
This is not in the mainline, so linux-next I suppose?
...or at least it will be shortly. Greg notified that it is currently in driver-core-next:
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/commi...
Dan Williams wrote:
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/0Check 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
Ok, this one is now
Tested-by: Marc Herbert marc.herbert@intel.com
...per:
https://github.com/thesofproject/linux/pull/4799#issuecomment-1960469389
Thanks for all the help Marc!
On Thu, Feb 22, 2024 at 12:40:54PM -0800, Dan Williams wrote:
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/0Check for non-empty attributes array before calling is_visible().
[...]
--- 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?
Thanks,
Lukas
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 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.
Thanks,
Lukas
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
Lukas Wunner wrote: [..]
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.
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? I expect this issue to come up again because dynamically populated attribute arrays of a statically defined group is a useful mechanism. I might need this in the PCI TSM enabling... but having at least one attribute there is likely not a problem.
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.
Thanks,
Lukas
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.
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.
Dan Williams wrote:
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.
Oh, but if an is_visible() callback gets confused by empty_attr, that's detectable:
mode = grp->is_visible(kobj, attr, 0); if ((mode & ~SYSFS_GROUP_INVISIBLE) && attr == empty_attr) return 0;
...i.e. the only acceptable responses to an empty_attr check is 0 or ~SYSFS_GROUP_INVISIBLE.
On Sat, Apr 27, 2024 at 02:33:24PM -0700, Dan Williams wrote:
Lukas Wunner wrote:
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.
Memory is cheap and memory-constrained devices can set CONFIG_SYSFS=n.
There aren't that many struct attribute_groups and this is just 8 additional bytes on a 64-bit machine. (There are way more struct attribute than struct attribute_group.) The contortions necessary to overload individual attribute ->is_visible() callbacks to also govern the group's visibility aren't worth it.
Having an ->is_group_visible() callback has the additional benefit that the mode of directories no longer needs to be hardcoded to 0755 in sysfs_create_dir_ns(), but can be set to, say, 0500 or 0700 or 0511, depending on the use case. So more flexibility there as well.
Thanks,
Lukas
Lukas Wunner wrote:
On Sat, Apr 27, 2024 at 02:33:24PM -0700, Dan Williams wrote:
Lukas Wunner wrote:
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.
Memory is cheap and memory-constrained devices can set CONFIG_SYSFS=n.
That sounds severe, but point taken that someone could config-off the cases that need this extension.
There aren't that many struct attribute_groups and this is just 8 additional bytes on a 64-bit machine. (There are way more struct attribute than struct attribute_group.) The contortions necessary to overload individual attribute ->is_visible() callbacks to also govern the group's visibility aren't worth it.
I agree that most systems would not care about growing this structure, but the same is true for almost all other data structure growth in the kernel. It is a typical kernel pastime to squeeze functionality into existing data structures.
Having an ->is_group_visible() callback has the additional benefit that the mode of directories no longer needs to be hardcoded to 0755 in sysfs_create_dir_ns(), but can be set to, say, 0500 or 0700 or 0511, depending on the use case. So more flexibility there as well.
Unnecessary growth is unnecessary growth. In this case all the known use cases can use the SYSFS_GROUP_INVISIBLE flag returned from is_visible(). The awkwardness around cases that want to have an empty attributes array and invisible group directory is noted and puts the solution on notice for running afoul of the sunk cost fallacy in the future.
Add documentation and examples for how to use DEFINE_SYSFS_GROUP_VISIBLE() and SYSFS_GROUP_VISIBLE(). Recall that the motivation for this work is that it is easier to reason about the lifetime of statically defined sysfs attributes that become visible at device_add() time rather than dynamically adding them later. DEFINE_SYSFS_GROUP_VISIBLE() tackles one of the reasons to opt for dynamically created attributes which did not have a facility for hiding empty directories.
Signed-off-by: Dan Williams dan.j.williams@intel.com --- include/linux/sysfs.h | 42 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 39 insertions(+), 3 deletions(-)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h index a42642b277dd..dabf7f4f3581 100644 --- a/include/linux/sysfs.h +++ b/include/linux/sysfs.h @@ -105,8 +105,42 @@ struct attribute_group { #define SYSFS_GROUP_INVISIBLE 020000
/* - * The first call to is_visible() in the create / update path may - * indicate visibility for the entire group + * DEFINE_SYSFS_GROUP_VISIBLE(name): + * A helper macro to pair with the assignment of ".is_visible = + * SYSFS_GROUP_VISIBLE(name)", that arranges for the directory + * associated with a named attribute_group to optionally be hidden. + * This allows for static declaration of attribute_groups, and the + * simplification of attribute visibility lifetime that implies, + * without polluting sysfs with empty attribute directories. + * Ex. + * + * static umode_t example_attr_visible(struct kobject *kobj, + * struct attribute *attr, int n) + * { + * if (example_attr_condition) + * return 0; + * else if (ro_attr_condition) + * return 0444; + * return a->mode; + * } + * + * static bool example_group_visible(struct kobject *kobj) + * { + * if (example_group_condition) + * return false; + * return true; + * } + * + * DEFINE_SYSFS_GROUP_VISIBLE(example); + * + * static struct attribute_group example_group = { + * .name = "example", + * .is_visible = SYSFS_GROUP_VISIBLE(example), + * .attrs = &example_attrs, + * }; + * + * Note that it expects <name>_attr_visible and <name>_group_visible to + * be defined. */ #define DEFINE_SYSFS_GROUP_VISIBLE(name) \ static inline umode_t sysfs_group_visible_##name( \ @@ -119,7 +153,9 @@ struct attribute_group {
/* * Same as DEFINE_SYSFS_GROUP_VISIBLE, but for groups with only binary - * attributes + * attributes. If an attribute_group defines both text and binary + * attributes, the group visibility is determined by the function + * specified to is_visible() not is_bin_visible() */ #define DEFINE_SYSFS_BIN_GROUP_VISIBLE(name) \ static inline umode_t sysfs_group_visible_##name( \
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
/*
On Thu, Feb 22, 2024 at 12:40:48PM -0800, Dan Williams wrote:
Hey Greg,
Marc was able to get me a backtrace for that bootup hang scenario that Pierre noted here [1]. A Tested-by is still pending, but I am certain this is the issue, and it may impact more than just the one platform if that "empty_attrs" pattern has been repeated anywhere else in the kernel.
I also took some time to document how to use the helpers a bit better, and introduce DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE() for cases where all that matters is group visibility and not per attribute visibility.
I'll just queue these up now on my normal driver-core-next branch, and not worry about a stable tag as I doubt anyone wants that now. But if they do, please let me know and I can provide one.
thanks for the quick fixes!
greg k-h
participants (4)
-
Dan Williams -
Greg KH -
Lukas Wunner -
Rafael J. Wysocki