Date: Wed, 24 Jan 2024 12:31:23 -0800
From: Dan Williams <dan.j.williams@...el.com>
To: Greg KH <gregkh@...uxfoundation.org>, Alistair Francis
	<alistair23@...il.com>
CC: <bhelgaas@...gle.com>, <linux-pci@...r.kernel.org>,
	<Jonathan.Cameron@...wei.com>, <lukas@...ner.de>,
	<alex.williamson@...hat.com>, <christian.koenig@....com>, <kch@...dia.com>,
	<logang@...tatee.com>, <linux-kernel@...r.kernel.org>,
	<chaitanyak@...dia.com>, <rdunlap@...radead.org>, Alistair Francis
	<alistair.francis@....com>
Subject: Re: [PATCH v6 2/3] sysfs: Add a attr_is_visible function to
 attribute_group

Greg KH wrote:
[..]
> > 
> > Hey Greg,
> > 
> > I wanted to follow up on this and see if you are able to provide more
> > details for reproducing or if you are able to look into it?
> 
> Last I tried this, it still crashed and would not boot either on my
> laptop or my workstation.  I don't know how it is working properly for
> you, what systems have you tried it on?
> 
> I'm not going to be able to look at this for many weeks due to
> conference stuff, so if you want to take the series and test it and
> hopefully catch my error, that would be great, I'd love to move forward
> and get this merged someday.

I mentioned to Lukas that I was working on a "sysfs group visibility"
patch and he pointed me to this thread. I will note that I tried to make
the "hide group if all attributes are invisible" approach work, but
reverted to a "new is_group_visible() callback" approach. I did read
through the thread and try to improve the argument in the changelog
accordingly.

I do admit to liking the cleanliness (not touching 'struct
attribute_group') of the "hide if no visible attribute" approch, but see
the criticism of that alternative below, and let me know if it is
convincing. I tested it locally with the following hack to make the
group disappear every other sysfs_update_group() event:

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 3e817a6f94c6..a5c4e8f3e93b 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2022,9 +2022,20 @@ static umode_t cxl_region_target_visible(struct kobject *kobj,
        return 0;
 }
 
+static umode_t cxl_region_target_group_visible(struct kobject *kobj)
+{
+       static u32 visible;
+
+       if (visible++ & 1)
+               return 0755;
+       return 0;
+}
+
 static const struct attribute_group cxl_region_target_group = {
+       .name = "target_group",
        .attrs = target_attrs,
        .is_visible = cxl_region_target_visible,
+       .is_group_visible = cxl_region_target_group_visible,
 };
 
 static const struct attribute_group *get_cxl_region_target_group(void)


-- >8 --
>From 18d6fdf1465f4ce5f561a35797c1313276993af0 Mon Sep 17 00:00:00 2001
From: Dan Williams <dan.j.williams@...el.com>
Date: Tue, 23 Jan 2024 20:20:39 -0800
Subject: [PATCH] sysfs: Introduce is_group_visible() for attribute_groups

Add a method to 'struct attribute_group' to determine the visibility of
an entire named sysfs group relative to the state of its parent kobject.

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.

The downside of the alternatives seem worse than the maintenance burden
of this new capability. Otherwise, this facility also brings support for
changing permissions on sysfs directories from the 0755 default for
potential cases where only root is expected to be able to enumerate.
That may prove useful as PCI sysfs picks up more security sensitive
enumeration.

The size increase of 'struct attribute_group' is a concern. Longer term
this could be reduced by consolidating the 3 is_visible() callbacks into
one that takes a parameter for "attr", "bin_attr", or "group". For now
the support is gated behind CONFIG_SYSFS_GROUP_VISIBLE so it can be
compiled out when not needed.

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@...el.com>
---
 fs/sysfs/Kconfig      |  3 +++
 fs/sysfs/group.c      | 50 +++++++++++++++++++++++++++++++++++--------
 include/linux/sysfs.h | 34 +++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+), 9 deletions(-)

diff --git a/fs/sysfs/Kconfig b/fs/sysfs/Kconfig
index b0fe1cce33a0..d5de8e54a06f 100644
--- a/fs/sysfs/Kconfig
+++ b/fs/sysfs/Kconfig
@@ -23,3 +23,6 @@ config SYSFS
 	example, "root=03:01" for /dev/hda1.
 
 	Designers of embedded systems may wish to say N here to conserve space.
+
+config SYSFS_GROUP_VISIBLE
+	bool
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 138676463336..0a977064e118 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -127,16 +127,36 @@ static int internal_create_group(struct kobject *kobj, int update,
 
 	kobject_get_ownership(kobj, &uid, &gid);
 	if (grp->name) {
+		umode_t mode = S_IRWXU | S_IRUGO | S_IXUGO;
+
+		if (has_group_visible(grp))
+			mode = is_group_visible(grp, kobj);
+
 		if (update) {
 			kn = kernfs_find_and_get(kobj->sd, grp->name);
 			if (!kn) {
-				pr_warn("Can't update unknown attr grp name: %s/%s\n",
-					kobj->name, grp->name);
-				return -EINVAL;
+				if (!has_group_visible(grp)) {
+					pr_warn("Can't update unknown attr grp name: %s/%s\n",
+						kobj->name, grp->name);
+					return -EINVAL;
+				}
+				/* may have been invisible prior to this update */
+				update = 0;
+			} else {
+				/* need to change the visibility of the entire group */
+				sysfs_remove_group(kobj, grp);
+				if (mode == 0) {
+					kernfs_put(kn);
+					return 0;
+				}
+				update = 0;
 			}
-		} else {
-			kn = kernfs_create_dir_ns(kobj->sd, grp->name,
-						  S_IRWXU | S_IRUGO | S_IXUGO,
+		}
+
+		if (!update) {
+			if (mode == 0)
+				return 0;
+			kn = kernfs_create_dir_ns(kobj->sd, grp->name, mode,
 						  uid, gid, kobj, NULL);
 			if (IS_ERR(kn)) {
 				if (PTR_ERR(kn) == -EEXIST)
@@ -262,6 +282,20 @@ int sysfs_update_group(struct kobject *kobj,
 }
 EXPORT_SYMBOL_GPL(sysfs_update_group);
 
+static void warn_group_not_found(const struct attribute_group *grp,
+				 struct kobject *kobj)
+{
+	if (has_group_visible(grp)) {
+		/* may have never been created */
+		pr_debug("sysfs group '%s' not found for kobject '%s'\n",
+			 grp->name, kobject_name(kobj));
+		return;
+	}
+
+	WARN(1, KERN_WARNING "sysfs group '%s' not found for kobject '%s'\n",
+	     grp->name, kobject_name(kobj));
+}
+
 /**
  * sysfs_remove_group: remove a group from a kobject
  * @kobj:	kobject to remove the group from
@@ -279,9 +313,7 @@ void sysfs_remove_group(struct kobject *kobj,
 	if (grp->name) {
 		kn = kernfs_find_and_get(parent, grp->name);
 		if (!kn) {
-			WARN(!kn, KERN_WARNING
-			     "sysfs group '%s' not found for kobject '%s'\n",
-			     grp->name, kobject_name(kobj));
+			warn_group_not_found(grp, kobj);
 			return;
 		}
 	} else {
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index b717a70219f6..31f3dd6fc946 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -77,6 +77,12 @@ do {							\
  *		return 0 if a binary attribute is not visible. The returned
  *		value will replace static permissions defined in
  *		struct bin_attribute.
+ * @is_group_visible:
+ * 		Optional: Function to return permissions associated with
+ * 		directory created for named groups. When this returns 0
+ * 		all attributes are removed on update, or otherwise the
+ * 		directory is not created in the first instance. When not
+ * 		specified the default permissions of 0755 are applied.
  * @attrs:	Pointer to NULL terminated list of attributes.
  * @bin_attrs:	Pointer to NULL terminated list of binary attributes.
  *		Either attrs or bin_attrs or both must be provided.
@@ -87,10 +93,38 @@ struct attribute_group {
 					      struct attribute *, int);
 	umode_t			(*is_bin_visible)(struct kobject *,
 						  struct bin_attribute *, int);
+#ifdef CONFIG_SYSFS_GROUP_VISIBLE
+	umode_t			(*is_group_visible)(struct kobject *);
+#endif
+
 	struct attribute	**attrs;
 	struct bin_attribute	**bin_attrs;
 };
 
+#ifdef CONFIG_SYSFS_GROUP_VISIBLE
+static inline bool has_group_visible(const struct attribute_group *grp)
+{
+	return grp->is_group_visible != NULL;
+}
+
+static inline umode_t is_group_visible(const struct attribute_group *grp,
+				       struct kobject *kobj)
+{
+	return grp->is_group_visible(kobj);
+}
+#else
+static inline bool has_group_visible(const struct attribute_group *grp)
+{
+	return false;
+}
+
+static inline umode_t is_group_visible(const struct attribute_group *grp,
+				       struct kobject *kobj)
+{
+	return 0755;
+}
+#endif
+
 /*
  * Use these macros to make defining attributes easier.
  * See include/linux/device.h for examples..
-- 
2.43.0

Powered by blists - more mailing lists