Re: [PATCH 1/6] sysfs: Introduce a mechanism to hide static attribute_groups

[Dan Williams]

Dan Williams wrote:
> Pierre-Louis Bossart wrote:
> > 
> > 
> > On 1/30/24 19:46, Greg Kroah-Hartman wrote:
> > > From: Dan Williams <dan.j.williams@xxxxxxxxx>
> > > 
> > > Add a mechanism for named attribute_groups to hide their directory at
> > > sysfs_update_group() time, or otherwise skip emitting the group
> > > directory when the group is first registered. It piggybacks on
> > > is_visible() in a similar manner as SYSFS_PREALLOC, i.e. special flags
> > > in the upper bits of the returned mode. To use it, specify a symbol
> > > prefix to DEFINE_SYSFS_GROUP_VISIBLE(), and then pass that same prefix
> > > to SYSFS_GROUP_VISIBLE() when assigning the @is_visible() callback:
> > > 
> > > 	DEFINE_SYSFS_GROUP_VISIBLE($prefix)
> > > 
> > > 	struct attribute_group $prefix_group = {
> > > 		.name = $name,
> > > 		.is_visible = SYSFS_GROUP_VISIBLE($prefix),
> > > 	};
> > > 
> > > SYSFS_GROUP_VISIBLE() expects a definition of $prefix_group_visible()
> > > and $prefix_attr_visible(), where $prefix_group_visible() just returns
> > > true / false and $prefix_attr_visible() behaves as normal.
> > > 
> > > The motivation for this capability is to centralize PCI device
> > > authentication in the PCI core with a named sysfs group while keeping
> > > that group hidden for devices and platforms that do not meet the
> > > requirements. In a PCI topology, most devices will not support
> > > authentication, a small subset will support just PCI CMA (Component
> > > Measurement and Authentication), a smaller subset will support PCI CMA +
> > > PCIe IDE (Link Integrity and Encryption), and only next generation
> > > server hosts will start to include a platform TSM (TEE Security
> > > Manager).
> > > 
> > > Without this capability the alternatives are:
> > > 
> > > * Check if all attributes are invisible and if so, hide the directory.
> > >   Beyond trouble getting this to work [1], this is an ABI change for
> > >   scenarios if userspace happens to depend on group visibility absent any
> > >   attributes. I.e. this new capability avoids regression since it does
> > >   not retroactively apply to existing cases.
> > > 
> > > * Publish an empty /sys/bus/pci/devices/$pdev/tsm/ directory for all PCI
> > >   devices (i.e. for the case when TSM platform support is present, but
> > >   device support is absent). Unfortunate that this will be a vestigial
> > >   empty directory in the vast majority of cases.
> > > 
> > > * Reintroduce usage of runtime calls to sysfs_{create,remove}_group()
> > >   in the PCI core. Bjorn has already indicated that he does not want to
> > >   see any growth of pci_sysfs_init() [2].
> > > 
> > > * Drop the named group and simulate a directory by prefixing all
> > >   TSM-related attributes with "tsm_". Unfortunate to not use the naming
> > >   capability of a sysfs group as intended.
> > > 
> > > In comparison, there is a small potential for regression if for some
> > > reason an @is_visible() callback had dependencies on how many times it
> > > was called. Additionally, it is no longer an error to update a group
> > > that does not have its directory already present, and it is no longer a
> > > WARN() to remove a group that was never visible.
> > > 
> > > Link: https://lore.kernel.org/all/2024012321-envious-procedure-4a58@gregkh/ [1]
> > > Link: https://lore.kernel.org/linux-pci/20231019200110.GA1410324@bhelgaas/ [2]
> > > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > 
> > This patch seems to introduce a regression on our Lunar Lake test
> > devices, where we can't boot to an ssh shell. No issues on older devices
> > [1]. Bard Liao and I reproduced the same results on different boards.
> > 
> > We'll need to find someone with direct device access to provide more
> > information on the problem, remote testing without ssh is a
> > self-negating proposition.
> > 
> > Is there a dependency on other patches? Our tests are still based on
> > 6.7.0-rc3 due to other upstream issues we're currently working through.
> 
> The only behavior change I can imagine with this patch is that
> ->is_visble() callbacks get called extra times for named attribute
> groups.
> 
> ...or if an is_visible() callback was inadvertantly already using the
> SYSFS_GROUP_INVISIBLE flag in umode_t result.

Are you able to get kernel logs? A before and after with this patch
applied might highlight which attribute does not appreciate the extra
callback...

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index ccb275cdabcb..683c0b10990b 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -33,11 +33,17 @@ 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->is_visible) {
+               pr_info("kobj: %s is_visible: %pS\n", kobj->name,
+                       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->is_bin_visible) {
+               pr_info("kobj: %s is_bin_visible: %pS\n", kobj->name,
+                       grp->is_bin_visible);
                return grp->is_bin_visible(kobj, grp->bin_attrs[0], 0);
+       }
 
        return 0;
 }
@@ -62,6 +68,8 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
                        if (update)
                                kernfs_remove_by_name(parent, (*attr)->name);
                        if (grp->is_visible) {
+                               pr_info("kobj: %s is_visible: %pS\n",
+                                       kobj->name, grp->is_visible);
                                mode = grp->is_visible(kobj, *attr, i);
                                mode &= ~SYSFS_GROUP_INVISIBLE;
                                if (!mode)
@@ -92,6 +100,8 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
                                kernfs_remove_by_name(parent,
                                                (*bin_attr)->attr.name);
                        if (grp->is_bin_visible) {
+                               pr_info("kobj: %s is_bin_visible: %pS\n",
+                                       kobj->name, grp->is_bin_visible);
                                mode = grp->is_bin_visible(kobj, *bin_attr, i);
                                mode &= ~SYSFS_GROUP_INVISIBLE;
                                if (!mode)