[PATCH] sysfs: Allow bin_attributes to be added to groups

[PATCH] sysfs: Allow bin_attributes to be added to groups

[Lukas Wunner]

Commit dfa87c824a9a ("sysfs: allow attributes to be added to groups")
introduced dynamic addition of sysfs attributes to groups.

Allow the same for bin_attributes, in support of a forthcoming commit
which adds various bin_attributes every time a PCI device is
authenticated.

Addition of bin_attributes to groups differs from regular attributes in
that different kernfs_ops are selected by sysfs_add_bin_file_mode_ns()
vis-à-vis sysfs_add_file_mode_ns().

So call either of those two functions from sysfs_add_file_to_group()
based on an additional boolean parameter and add two wrapper functions,
one for bin_attributes and another for regular attributes.

Removal of bin_attributes from groups does not require a differentiation
for bin_attributes and can use the same code path as regular attributes.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Alan Stern <stern@rowland.harvard.edu>
---
Submitting this ahead of my PCI device authentication v2 patches.
Not sure if the patch is acceptable without an accompanying user,
but even if it's not, perhaps someone has early review feedback
or wants to provide an Acked-by?  Thank you!

 fs/sysfs/file.c       | 69 ++++++++++++++++++++++++++++++++++++-------
 include/linux/sysfs.h | 19 ++++++++++++
 2 files changed, 78 insertions(+), 10 deletions(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index d1995e2d6c94..9268232781b5 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -383,14 +383,14 @@ int sysfs_create_files(struct kobject *kobj, const struct attribute * const *ptr
 }
 EXPORT_SYMBOL_GPL(sysfs_create_files);
 
-/**
- * sysfs_add_file_to_group - add an attribute file to a pre-existing group.
- * @kobj: object we're acting for.
- * @attr: attribute descriptor.
- * @group: group name.
- */
-int sysfs_add_file_to_group(struct kobject *kobj,
-		const struct attribute *attr, const char *group)
+static const struct bin_attribute *to_bin_attr(const struct attribute *attr)
+{
+	return container_of(attr, struct bin_attribute, attr);
+}
+
+static int __sysfs_add_file_to_group(struct kobject *kobj,
+				     const struct attribute *attr,
+				     const char *group, bool is_bin_attr)
 {
 	struct kernfs_node *parent;
 	kuid_t uid;
@@ -408,14 +408,49 @@ int sysfs_add_file_to_group(struct kobject *kobj,
 		return -ENOENT;
 
 	kobject_get_ownership(kobj, &uid, &gid);
-	error = sysfs_add_file_mode_ns(parent, attr, attr->mode, uid, gid,
-				       NULL);
+	if (is_bin_attr)
+		error = sysfs_add_bin_file_mode_ns(parent, to_bin_attr(attr),
+						   attr->mode, uid, gid, NULL);
+	else
+		error = sysfs_add_file_mode_ns(parent, attr,
+					       attr->mode, uid, gid, NULL);
 	kernfs_put(parent);
 
 	return error;
 }
+
+/**
+ * sysfs_add_file_to_group - add an attribute file to a pre-existing group.
+ * @kobj: object we're acting for.
+ * @attr: attribute descriptor.
+ * @group: group name.
+ *
+ * Returns 0 on success or error code on failure.
+ */
+int sysfs_add_file_to_group(struct kobject *kobj,
+			    const struct attribute *attr,
+			    const char *group)
+{
+	return __sysfs_add_file_to_group(kobj, attr, group, false);
+}
 EXPORT_SYMBOL_GPL(sysfs_add_file_to_group);
 
+/**
+ * sysfs_add_bin_file_to_group - add bin_attribute file to pre-existing group.
+ * @kobj: object we're acting for.
+ * @attr: attribute descriptor.
+ * @group: group name.
+ *
+ * Returns 0 on success or error code on failure.
+ */
+int sysfs_add_bin_file_to_group(struct kobject *kobj,
+				const struct bin_attribute *attr,
+				const char *group)
+{
+	return __sysfs_add_file_to_group(kobj, &attr->attr, group, true);
+}
+EXPORT_SYMBOL_GPL(sysfs_add_bin_file_to_group);
+
 /**
  * sysfs_chmod_file - update the modified mode value on an object attribute.
  * @kobj: object we're acting for.
@@ -565,6 +600,20 @@ void sysfs_remove_file_from_group(struct kobject *kobj,
 }
 EXPORT_SYMBOL_GPL(sysfs_remove_file_from_group);
 
+/**
+ * sysfs_remove_bin_file_from_group - remove bin_attribute file from group.
+ * @kobj: object we're acting for.
+ * @attr: attribute descriptor.
+ * @group: group name.
+ */
+void sysfs_remove_bin_file_from_group(struct kobject *kobj,
+				      const struct bin_attribute *attr,
+				      const char *group)
+{
+	sysfs_remove_file_from_group(kobj, &attr->attr, group);
+}
+EXPORT_SYMBOL_GPL(sysfs_remove_bin_file_from_group);
+
 /**
  *	sysfs_create_bin_file - create binary file for object.
  *	@kobj:	object.
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index a7d725fbf739..aff1d81e8971 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -451,6 +451,12 @@ int sysfs_add_file_to_group(struct kobject *kobj,
 			const struct attribute *attr, const char *group);
 void sysfs_remove_file_from_group(struct kobject *kobj,
 			const struct attribute *attr, const char *group);
+int sysfs_add_bin_file_to_group(struct kobject *kobj,
+				const struct bin_attribute *attr,
+				const char *group);
+void sysfs_remove_bin_file_from_group(struct kobject *kobj,
+				      const struct bin_attribute *attr,
+				      const char *group);
 int sysfs_merge_group(struct kobject *kobj,
 		       const struct attribute_group *grp);
 void sysfs_unmerge_group(struct kobject *kobj,
@@ -660,6 +666,19 @@ static inline void sysfs_remove_file_from_group(struct kobject *kobj,
 {
 }
 
+static inline int sysfs_add_bin_file_to_group(struct kobject *kobj,
+					      const struct bin_attribute *attr,
+					      const char *group)
+{
+	return 0;
+}
+
+static inline void sysfs_remove_bin_file_from_group(struct kobject *kobj,
+					      const struct bin_attribute *attr,
+					      const char *group)
+{
+}
+
 static inline int sysfs_merge_group(struct kobject *kobj,
 		       const struct attribute_group *grp)
 {
-- 
2.43.0

RE: [PATCH] sysfs: Allow bin_attributes to be added to groups

[Dan Williams]

Lukas Wunner wrote:
> Commit dfa87c824a9a ("sysfs: allow attributes to be added to groups")
> introduced dynamic addition of sysfs attributes to groups.
> 
> Allow the same for bin_attributes, in support of a forthcoming commit
> which adds various bin_attributes every time a PCI device is
> authenticated.
> 
> Addition of bin_attributes to groups differs from regular attributes in
> that different kernfs_ops are selected by sysfs_add_bin_file_mode_ns()
> vis-à-vis sysfs_add_file_mode_ns().
> 
> So call either of those two functions from sysfs_add_file_to_group()
> based on an additional boolean parameter and add two wrapper functions,
> one for bin_attributes and another for regular attributes.
> 
> Removal of bin_attributes from groups does not require a differentiation
> for bin_attributes and can use the same code path as regular attributes.
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> ---
> Submitting this ahead of my PCI device authentication v2 patches.
> Not sure if the patch is acceptable without an accompanying user,
> but even if it's not, perhaps someone has early review feedback
> or wants to provide an Acked-by?  Thank you!

On the one hand it makes sense from a symmetry perspective, on the other
hand the expectation and the infrastructure for dynamic sysfs visibilty
has increased since 2007.

That is why I would like to see the use case to understand why a
dynamically added a bin_attribute is needed compared with a statically
defined attribute with dynamic visibility.

Re: [PATCH] sysfs: Allow bin_attributes to be added to groups

[Lukas Wunner]

On Fri, Apr 26, 2024 at 10:40:53AM -0700, Dan Williams wrote:
> Lukas Wunner wrote:
> > Commit dfa87c824a9a ("sysfs: allow attributes to be added to groups")
> > introduced dynamic addition of sysfs attributes to groups.
> > 
> > Allow the same for bin_attributes, in support of a forthcoming commit
> > which adds various bin_attributes every time a PCI device is
> > authenticated.
> > 
> > Addition of bin_attributes to groups differs from regular attributes in
> > that different kernfs_ops are selected by sysfs_add_bin_file_mode_ns()
> > vis-à-vis sysfs_add_file_mode_ns().
> > 
> > So call either of those two functions from sysfs_add_file_to_group()
> > based on an additional boolean parameter and add two wrapper functions,
> > one for bin_attributes and another for regular attributes.
> > 
> > Removal of bin_attributes from groups does not require a differentiation
> > for bin_attributes and can use the same code path as regular attributes.
> > 
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > Cc: Alan Stern <stern@rowland.harvard.edu>
> > ---
> > Submitting this ahead of my PCI device authentication v2 patches.
> > Not sure if the patch is acceptable without an accompanying user,
> > but even if it's not, perhaps someone has early review feedback
> > or wants to provide an Acked-by?  Thank you!
> 
> On the one hand it makes sense from a symmetry perspective, on the other
> hand the expectation and the infrastructure for dynamic sysfs visibilty
> has increased since 2007.
> 
> That is why I would like to see the use case to understand why a
> dynamically added a bin_attribute is needed compared with a statically
> defined attribute with dynamic visibility.

I assume "would like to see" means "on the mailing list", which would
(or will) be as part of the PCI device authentication v2 patches.

But in case you're curious, the use case is the log of signatures
presented by the device.  Each signature is exposed as a bin_attribute
in a signatures/ directory below a PCI device's sysfs directory,
for verification by a remote attester (or user space in general).

Here's the code:

https://github.com/l1k/linux/commit/ca420b22af05

The signature's bin_attribute is accompanied by several other
bin_attributes containing ancillary data, such as nonces
(to allow user space to ascertain that a fresh nonce has been used).

The signatures/ directory is an empty attribute group to which the
signatures received from the device are dynamically added using
sysfs_add_bin_file_to_group() (introduced by the present patch).

All of that was done to address an objection raised by James Bottomley
at Plumbers that it is "not good enough" if the kernel keeps the
challenge-response received from the device to itself and doesn't
allow a remote attester to verify it.

Here is the recording, in his own words:

https://www.youtube.com/watch?v=ZqMIlZ5lPAw&t=2345s

A static attribute with dynamic visibility doesn't cut it in this case:

Each signature needs a unique filename and the number of signatures
that can be generated is essentially unlimited.  (Though older ones
are likely uninteresting and can be culled.)  If you want to expose,
say, up to 1000 signatures per device, you'd have to allocate an array
of 1000 signatures (for each device!), even though the actual number
of signatures received might be much lower.  It would be a waste of
memory.  It is much more economical to add signature attributes
dynamically on demand.

It has the additional benefit that it allows user space to dynamically
adjust the maximum number of signatures retained in the log.
That's more difficult to implement with static attributes, as you'd
have to reallocate the attributes array and adjust all the pointers
pointing to it.

Thanks,

Lukas

Re: [PATCH] sysfs: Allow bin_attributes to be added to groups

[Greg Kroah-Hartman]

On Fri, Apr 26, 2024 at 08:59:51PM +0200, Lukas Wunner wrote:
> On Fri, Apr 26, 2024 at 10:40:53AM -0700, Dan Williams wrote:
> > Lukas Wunner wrote:
> > > Commit dfa87c824a9a ("sysfs: allow attributes to be added to groups")
> > > introduced dynamic addition of sysfs attributes to groups.
> > > 
> > > Allow the same for bin_attributes, in support of a forthcoming commit
> > > which adds various bin_attributes every time a PCI device is
> > > authenticated.
> > > 
> > > Addition of bin_attributes to groups differs from regular attributes in
> > > that different kernfs_ops are selected by sysfs_add_bin_file_mode_ns()
> > > vis-à-vis sysfs_add_file_mode_ns().
> > > 
> > > So call either of those two functions from sysfs_add_file_to_group()
> > > based on an additional boolean parameter and add two wrapper functions,
> > > one for bin_attributes and another for regular attributes.
> > > 
> > > Removal of bin_attributes from groups does not require a differentiation
> > > for bin_attributes and can use the same code path as regular attributes.
> > > 
> > > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > > Cc: Alan Stern <stern@rowland.harvard.edu>
> > > ---
> > > Submitting this ahead of my PCI device authentication v2 patches.
> > > Not sure if the patch is acceptable without an accompanying user,
> > > but even if it's not, perhaps someone has early review feedback
> > > or wants to provide an Acked-by?  Thank you!
> > 
> > On the one hand it makes sense from a symmetry perspective, on the other
> > hand the expectation and the infrastructure for dynamic sysfs visibilty
> > has increased since 2007.
> > 
> > That is why I would like to see the use case to understand why a
> > dynamically added a bin_attribute is needed compared with a statically
> > defined attribute with dynamic visibility.
> 
> I assume "would like to see" means "on the mailing list", which would
> (or will) be as part of the PCI device authentication v2 patches.
> 
> But in case you're curious, the use case is the log of signatures
> presented by the device.  Each signature is exposed as a bin_attribute
> in a signatures/ directory below a PCI device's sysfs directory,
> for verification by a remote attester (or user space in general).
> 
> Here's the code:
> 
> https://github.com/l1k/linux/commit/ca420b22af05
> 
> The signature's bin_attribute is accompanied by several other
> bin_attributes containing ancillary data, such as nonces
> (to allow user space to ascertain that a fresh nonce has been used).
> 
> The signatures/ directory is an empty attribute group to which the
> signatures received from the device are dynamically added using
> sysfs_add_bin_file_to_group() (introduced by the present patch).
> 
> All of that was done to address an objection raised by James Bottomley
> at Plumbers that it is "not good enough" if the kernel keeps the
> challenge-response received from the device to itself and doesn't
> allow a remote attester to verify it.
> 
> Here is the recording, in his own words:
> 
> https://www.youtube.com/watch?v=ZqMIlZ5lPAw&t=2345s
> 
> A static attribute with dynamic visibility doesn't cut it in this case:
> 
> Each signature needs a unique filename and the number of signatures
> that can be generated is essentially unlimited.  (Though older ones
> are likely uninteresting and can be culled.)  If you want to expose,
> say, up to 1000 signatures per device, you'd have to allocate an array
> of 1000 signatures (for each device!), even though the actual number
> of signatures received might be much lower.  It would be a waste of
> memory.  It is much more economical to add signature attributes
> dynamically on demand.
> 
> It has the additional benefit that it allows user space to dynamically
> adjust the maximum number of signatures retained in the log.
> That's more difficult to implement with static attributes, as you'd
> have to reallocate the attributes array and adjust all the pointers
> pointing to it.

Thanks for the details, that makes sense.

And overall, the patch also looks good, so just include it in the patch
series that uses it and I'll be glad to ack it then.

thanks,

greg k-h

Re: [PATCH] sysfs: Allow bin_attributes to be added to groups

[Dan Williams]

Lukas Wunner wrote:
> On Fri, Apr 26, 2024 at 10:40:53AM -0700, Dan Williams wrote:
> > Lukas Wunner wrote:
> > > Commit dfa87c824a9a ("sysfs: allow attributes to be added to groups")
> > > introduced dynamic addition of sysfs attributes to groups.
> > > 
> > > Allow the same for bin_attributes, in support of a forthcoming commit
> > > which adds various bin_attributes every time a PCI device is
> > > authenticated.
> > > 
> > > Addition of bin_attributes to groups differs from regular attributes in
> > > that different kernfs_ops are selected by sysfs_add_bin_file_mode_ns()
> > > vis-à-vis sysfs_add_file_mode_ns().
> > > 
> > > So call either of those two functions from sysfs_add_file_to_group()
> > > based on an additional boolean parameter and add two wrapper functions,
> > > one for bin_attributes and another for regular attributes.
> > > 
> > > Removal of bin_attributes from groups does not require a differentiation
> > > for bin_attributes and can use the same code path as regular attributes.
> > > 
> > > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > > Cc: Alan Stern <stern@rowland.harvard.edu>
> > > ---
> > > Submitting this ahead of my PCI device authentication v2 patches.
> > > Not sure if the patch is acceptable without an accompanying user,
> > > but even if it's not, perhaps someone has early review feedback
> > > or wants to provide an Acked-by?  Thank you!
> > 
> > On the one hand it makes sense from a symmetry perspective, on the other
> > hand the expectation and the infrastructure for dynamic sysfs visibilty
> > has increased since 2007.
> > 
> > That is why I would like to see the use case to understand why a
> > dynamically added a bin_attribute is needed compared with a statically
> > defined attribute with dynamic visibility.
> 
> I assume "would like to see" means "on the mailing list", which would
> (or will) be as part of the PCI device authentication v2 patches.

That works, yeah.

[..]
> Each signature needs a unique filename and the number of signatures
> that can be generated is essentially unlimited.  (Though older ones
> are likely uninteresting and can be culled.)  If you want to expose,
> say, up to 1000 signatures per device, you'd have to allocate an array
> of 1000 signatures (for each device!), even though the actual number
> of signatures received might be much lower.  It would be a waste of
> memory.  It is much more economical to add signature attributes
> dynamically on demand.

Ah, yeah. If it was a double-digit number of files then a static array
would be ok, but once it gets to 1000 potential files, then dynamic
makes complete sense.

> It has the additional benefit that it allows user space to dynamically
> adjust the maximum number of signatures retained in the log.
> That's more difficult to implement with static attributes, as you'd
> have to reallocate the attributes array and adjust all the pointers
> pointing to it.

To be clear, and for example, dynamic adjusting an array of attributes
based on userspace input is what cxl_region_target_group does, but again
with 1000 potential files the static array would be silly.

Thanks for the details!