| Message ID | 170863445442.1479840.1818801787239831650.stgit@dwillia2-xfh.jf.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | sysfs: Group visibility fixups | expand |
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/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") 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/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") > > 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/commit/?h=driver-core-next&id=70317fd24b41
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/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> 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/0 > > Check 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.
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;
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(-)