[PATCH v3 0/7] sysfs: prepare the constification of struct attribute

[PATCH v3 0/7] sysfs: prepare the constification of struct attribute

[Thomas Weißschuh]

The migration of 'struct attribute' and its related structures and
macros are more complicated than those for 'struct bin_attribute'.
Mostly because they are all shared by various custom attribute types.
Introduce some initial utilities to support the migration.
These are enough to migrate some specialized attributes atomically or
those which don't use 'struct attribute' in their callbacks stepwise.

The big outstanding problems are 'struct device_attribute' and
'struct kobj_attribute'. These are used everywhere and I'm not yet sure
about a migration plan.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Changes in v3:
- Rebase von v6.17-rc1
- Link to v2: https://lore.kernel.org/r/20250629-sysfs-const-attr-prep-v2-0-9ec5fe39083f@weissschuh.net

Changes in v2:
- Rebase onto driver-core-next
- Simplify attribute macro definitions
- +Cc Danilo
- Link to v1: https://lore.kernel.org/r/20250116-sysfs-const-attr-prep-v1-0-15e72dba4205@weissschuh.net

---
Thomas Weißschuh (7):
      sysfs: attribute_group: allow registration of const attribute
      sysfs: transparently handle const pointers in ATTRIBUTE_GROUPS()
      sysfs: introduce __SYSFS_FUNCTION_ALTERNATIVE()
      sysfs: attribute_group: enable const variants of is_visible()
      samples/kobject: add is_visible() callback to attribute group
      samples/kobject: constify 'struct foo_attribute'
      sysfs: simplify attribute definition macros

 fs/sysfs/group.c               | 10 +++++++--
 include/linux/sysfs.h          | 48 +++++++++++++++++++++++++-----------------
 samples/kobject/kset-example.c | 44 ++++++++++++++++++++++++++------------
 3 files changed, 67 insertions(+), 35 deletions(-)
---
base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
change-id: 20250114-sysfs-const-attr-prep-e9414982dc4f

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH v3 1/7] sysfs: attribute_group: allow registration of const attribute

[Thomas Weißschuh]

To be able to constify instances of struct attribute it has to be
possible to add them to struct attribute_group.
The current type of the attrs member however is not compatible with that.
Introduce a union that allows registration of both const and non-const
attributes to enable a piecewise transition.
As both union member types are compatible no logic needs to be adapted.

Technically it is now possible register a const struct
attribute and receive it as mutable pointer in the callbacks.
This is a soundness issue.
But this same soundness issue already exists today in
sysfs_create_file().
Also the struct definition and callback implementation are always
closely linked and are meant to be moved to const in lockstep.

Similar to commit 906c508afdca ("sysfs: attribute_group: allow registration of const bin_attribute")

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 include/linux/sysfs.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index f418aae4f1134f8126783d9e8eb575ba4278e927..a47092e837d9eb014894d1f7e49f0fd0f9a2e350 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -105,7 +105,10 @@ struct attribute_group {
 	size_t			(*bin_size)(struct kobject *,
 					    const struct bin_attribute *,
 					    int);
-	struct attribute	**attrs;
+	union {
+		struct attribute	**attrs;
+		const struct attribute	*const *attrs_new;
+	};
 	union {
 		const struct bin_attribute	*const *bin_attrs;
 		const struct bin_attribute	*const *bin_attrs_new;

-- 
2.50.1

[PATCH v3 2/7] sysfs: transparently handle const pointers in ATTRIBUTE_GROUPS()

[Thomas Weißschuh]

To ease the constification process of 'struct attribute', transparently
handle the const pointers in ATTRIBUTE_GROUPS().
A cast is used instead of assigning to .attrs_new as it keeps the macro
smaller. As both members are aliased to each other the result is
identical.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 include/linux/sysfs.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index a47092e837d9eb014894d1f7e49f0fd0f9a2e350..118a9b1d3b3e7528fb213d83f85d31bbac0dc309 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -290,7 +290,12 @@ static const struct attribute_group *_name##_groups[] = {	\
 
 #define ATTRIBUTE_GROUPS(_name)					\
 static const struct attribute_group _name##_group = {		\
-	.attrs = _name##_attrs,					\
+	.attrs = _Generic(_name##_attrs,			\
+			  struct attribute **:			\
+				_name##_attrs,			\
+			  const struct attribute *const *:	\
+				(void *)_name##_attrs		\
+	),							\
 };								\
 __ATTRIBUTE_GROUPS(_name)
 

-- 
2.50.1

[PATCH v3 3/7] sysfs: introduce __SYSFS_FUNCTION_ALTERNATIVE()

[Thomas Weißschuh]

For the constification phase of 'struct attribute' various callback
struct members will need to exist in both const and non-const variants.
Keeping both members in a union avoids memory and CPU overhead but will
be detected and trapped by Control Flow Integrity (CFI).
By deciding between a struct and a union depending whether CFI is
enabled, most configurations can avoid this overhead.
Code using these callbacks will still need to be updated to handle both
members explicitly.
In the union case the compiler will recognize that testing for one union
member is enough and optimize away the code for the other one.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 include/linux/sysfs.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 118a9b1d3b3e7528fb213d83f85d31bbac0dc309..3966947e78b8c030971daf5ee66bf5ab40bc2a17 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -58,6 +58,12 @@ do {							\
 #define sysfs_attr_init(attr) do {} while (0)
 #endif
 
+#ifdef CONFIG_CFI_CLANG
+#define __SYSFS_FUNCTION_ALTERNATIVE(MEMBERS...) struct { MEMBERS }
+#else
+#define __SYSFS_FUNCTION_ALTERNATIVE(MEMBERS...) union { MEMBERS }
+#endif
+
 /**
  * struct attribute_group - data structure used to declare an attribute group.
  * @name:	Optional: Attribute group name

-- 
2.50.1

[PATCH v3 4/7] sysfs: attribute_group: enable const variants of is_visible()

[Thomas Weißschuh]

When constifying instances of struct attribute, for consistency the
corresponding .is_visible() callback should be adapted, too.
Introduce a temporary transition mechanism until all callbacks are
converted.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 fs/sysfs/group.c      | 10 ++++++++--
 include/linux/sysfs.h |  8 ++++++--
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 2d78e94072a0d4ed957560c60cc3c5dd49ab6fb4..d514ce8ac4d964676006dc45a31bb9f5fe5fd5dd 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -36,6 +36,9 @@ static umode_t __first_visible(const struct attribute_group *grp, struct kobject
 	if (grp->attrs && grp->attrs[0] && grp->is_visible)
 		return grp->is_visible(kobj, grp->attrs[0], 0);
 
+	if (grp->attrs && grp->attrs[0] && grp->is_visible_new)
+		return grp->is_visible_new(kobj, grp->attrs[0], 0);
+
 	if (grp->bin_attrs && grp->bin_attrs[0] && grp->is_bin_visible)
 		return grp->is_bin_visible(kobj, grp->bin_attrs[0], 0);
 
@@ -61,8 +64,11 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
 			 */
 			if (update)
 				kernfs_remove_by_name(parent, (*attr)->name);
-			if (grp->is_visible) {
-				mode = grp->is_visible(kobj, *attr, i);
+			if (grp->is_visible || grp->is_visible_new) {
+				if (grp->is_visible)
+					mode = grp->is_visible(kobj, *attr, i);
+				else
+					mode = grp->is_visible_new(kobj, *attr, i);
 				mode &= ~SYSFS_GROUP_INVISIBLE;
 				if (!mode)
 					continue;
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 3966947e78b8c030971daf5ee66bf5ab40bc2a17..1807b0369bd4d993deab81c4497903468b751a19 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -104,8 +104,12 @@ do {							\
  */
 struct attribute_group {
 	const char		*name;
-	umode_t			(*is_visible)(struct kobject *,
-					      struct attribute *, int);
+	__SYSFS_FUNCTION_ALTERNATIVE(
+		umode_t			(*is_visible)(struct kobject *,
+						      struct attribute *, int);
+		umode_t			(*is_visible_new)(struct kobject *,
+							  const struct attribute *, int);
+	);
 	umode_t			(*is_bin_visible)(struct kobject *,
 						  const struct bin_attribute *, int);
 	size_t			(*bin_size)(struct kobject *,

-- 
2.50.1

[PATCH v3 5/7] samples/kobject: add is_visible() callback to attribute group

[Thomas Weißschuh]

There was no example for the is_visible() callback so far.

It will also become an example and test for the constification of
'struct attribute' later.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 samples/kobject/kset-example.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/samples/kobject/kset-example.c b/samples/kobject/kset-example.c
index 579ce150217c6e613887e32a08206573543b3091..1aac595ed9498b30448485a60d9376cb5b5ea1d3 100644
--- a/samples/kobject/kset-example.c
+++ b/samples/kobject/kset-example.c
@@ -178,7 +178,22 @@ static struct attribute *foo_default_attrs[] = {
 	&bar_attribute.attr,
 	NULL,	/* need to NULL terminate the list of attributes */
 };
-ATTRIBUTE_GROUPS(foo_default);
+
+static umode_t foo_default_attrs_is_visible(struct kobject *kobj,
+					    struct attribute *attr,
+					    int n)
+{
+	/* Hide attributes with the same name as the kobject. */
+	if (strcmp(kobject_name(kobj), attr->name) == 0)
+		return 0;
+	return attr->mode;
+}
+
+static const struct attribute_group foo_default_group = {
+	.attrs		= foo_default_attrs,
+	.is_visible	= foo_default_attrs_is_visible,
+};
+__ATTRIBUTE_GROUPS(foo_default);
 
 /*
  * Our own ktype for our kobjects.  Here we specify our sysfs ops, the

-- 
2.50.1

[PATCH v3 6/7] samples/kobject: constify 'struct foo_attribute'

[Thomas Weißschuh]

Showcase and test the new 'struct attribute' constification facilities.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 samples/kobject/kset-example.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/samples/kobject/kset-example.c b/samples/kobject/kset-example.c
index 1aac595ed9498b30448485a60d9376cb5b5ea1d3..98aac6940f51f3312b44083a9d4ffe7afed1dba7 100644
--- a/samples/kobject/kset-example.c
+++ b/samples/kobject/kset-example.c
@@ -37,10 +37,11 @@ struct foo_obj {
 /* a custom attribute that works just for a struct foo_obj. */
 struct foo_attribute {
 	struct attribute attr;
-	ssize_t (*show)(struct foo_obj *foo, struct foo_attribute *attr, char *buf);
-	ssize_t (*store)(struct foo_obj *foo, struct foo_attribute *attr, const char *buf, size_t count);
+	ssize_t (*show)(struct foo_obj *foo, const struct foo_attribute *attr, char *buf);
+	ssize_t (*store)(struct foo_obj *foo, const struct foo_attribute *attr,
+			 const char *buf, size_t count);
 };
-#define to_foo_attr(x) container_of(x, struct foo_attribute, attr)
+#define to_foo_attr(x) container_of_const(x, struct foo_attribute, attr)
 
 /*
  * The default show function that must be passed to sysfs.  This will be
@@ -53,7 +54,7 @@ static ssize_t foo_attr_show(struct kobject *kobj,
 			     struct attribute *attr,
 			     char *buf)
 {
-	struct foo_attribute *attribute;
+	const struct foo_attribute *attribute;
 	struct foo_obj *foo;
 
 	attribute = to_foo_attr(attr);
@@ -73,7 +74,7 @@ static ssize_t foo_attr_store(struct kobject *kobj,
 			      struct attribute *attr,
 			      const char *buf, size_t len)
 {
-	struct foo_attribute *attribute;
+	const struct foo_attribute *attribute;
 	struct foo_obj *foo;
 
 	attribute = to_foo_attr(attr);
@@ -109,13 +110,13 @@ static void foo_release(struct kobject *kobj)
 /*
  * The "foo" file where the .foo variable is read from and written to.
  */
-static ssize_t foo_show(struct foo_obj *foo_obj, struct foo_attribute *attr,
+static ssize_t foo_show(struct foo_obj *foo_obj, const struct foo_attribute *attr,
 			char *buf)
 {
 	return sysfs_emit(buf, "%d\n", foo_obj->foo);
 }
 
-static ssize_t foo_store(struct foo_obj *foo_obj, struct foo_attribute *attr,
+static ssize_t foo_store(struct foo_obj *foo_obj, const struct foo_attribute *attr,
 			 const char *buf, size_t count)
 {
 	int ret;
@@ -128,14 +129,14 @@ static ssize_t foo_store(struct foo_obj *foo_obj, struct foo_attribute *attr,
 }
 
 /* Sysfs attributes cannot be world-writable. */
-static struct foo_attribute foo_attribute =
+static const struct foo_attribute foo_attribute =
 	__ATTR(foo, 0664, foo_show, foo_store);
 
 /*
  * More complex function where we determine which variable is being accessed by
  * looking at the attribute for the "baz" and "bar" files.
  */
-static ssize_t b_show(struct foo_obj *foo_obj, struct foo_attribute *attr,
+static ssize_t b_show(struct foo_obj *foo_obj, const struct foo_attribute *attr,
 		      char *buf)
 {
 	int var;
@@ -147,7 +148,7 @@ static ssize_t b_show(struct foo_obj *foo_obj, struct foo_attribute *attr,
 	return sysfs_emit(buf, "%d\n", var);
 }
 
-static ssize_t b_store(struct foo_obj *foo_obj, struct foo_attribute *attr,
+static ssize_t b_store(struct foo_obj *foo_obj, const struct foo_attribute *attr,
 		       const char *buf, size_t count)
 {
 	int var, ret;
@@ -163,16 +164,16 @@ static ssize_t b_store(struct foo_obj *foo_obj, struct foo_attribute *attr,
 	return count;
 }
 
-static struct foo_attribute baz_attribute =
+static const struct foo_attribute baz_attribute =
 	__ATTR(baz, 0664, b_show, b_store);
-static struct foo_attribute bar_attribute =
+static const struct foo_attribute bar_attribute =
 	__ATTR(bar, 0664, b_show, b_store);
 
 /*
  * Create a group of attributes so that we can create and destroy them all
  * at once.
  */
-static struct attribute *foo_default_attrs[] = {
+static const struct attribute *const foo_default_attrs[] = {
 	&foo_attribute.attr,
 	&baz_attribute.attr,
 	&bar_attribute.attr,
@@ -180,7 +181,7 @@ static struct attribute *foo_default_attrs[] = {
 };
 
 static umode_t foo_default_attrs_is_visible(struct kobject *kobj,
-					    struct attribute *attr,
+					    const struct attribute *attr,
 					    int n)
 {
 	/* Hide attributes with the same name as the kobject. */
@@ -190,8 +191,8 @@ static umode_t foo_default_attrs_is_visible(struct kobject *kobj,
 }
 
 static const struct attribute_group foo_default_group = {
-	.attrs		= foo_default_attrs,
-	.is_visible	= foo_default_attrs_is_visible,
+	.attrs_new	= foo_default_attrs,
+	.is_visible_new	= foo_default_attrs_is_visible,
 };
 __ATTRIBUTE_GROUPS(foo_default);
 

-- 
2.50.1

[PATCH v3 7/7] sysfs: simplify attribute definition macros

[Thomas Weißschuh]

Define the macros in terms of each other.
This makes them easier to understand and also will make it easier to
implement the transition machinery for 'const struct attribute'.

__ATTR_RO_MODE() can't be implemented in terms of __ATTR() as not all
attributes have a .store callback.
The same issue theoretically exists for __ATTR_WO(), but practically it
does not occur inside the current tree.

Reorder __ATTR_RO() below __ATTR_RO_MODE() to keep the order of the
macro definition consistent with respect to each other.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 include/linux/sysfs.h | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 1807b0369bd4d993deab81c4497903468b751a19..2d6f984e10a96ab9916024ae7b72458edf0c5bd6 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -254,28 +254,20 @@ struct attribute_group {
 	.store	= _store,						\
 }
 
-#define __ATTR_RO(_name) {						\
-	.attr	= { .name = __stringify(_name), .mode = 0444 },		\
-	.show	= _name##_show,						\
-}
-
 #define __ATTR_RO_MODE(_name, _mode) {					\
 	.attr	= { .name = __stringify(_name),				\
 		    .mode = VERIFY_OCTAL_PERMISSIONS(_mode) },		\
 	.show	= _name##_show,						\
 }
 
-#define __ATTR_RW_MODE(_name, _mode) {					\
-	.attr	= { .name = __stringify(_name),				\
-		    .mode = VERIFY_OCTAL_PERMISSIONS(_mode) },		\
-	.show	= _name##_show,						\
-	.store	= _name##_store,					\
-}
+#define __ATTR_RO(_name)						\
+	__ATTR_RO_MODE(_name, 0444)
 
-#define __ATTR_WO(_name) {						\
-	.attr	= { .name = __stringify(_name), .mode = 0200 },		\
-	.store	= _name##_store,					\
-}
+#define __ATTR_RW_MODE(_name, _mode)					\
+	__ATTR(_name, _mode, _name##_show, _name##_store)
+
+#define __ATTR_WO(_name)						\
+	__ATTR(_name, 0200, NULL, _name##_store)
 
 #define __ATTR_RW(_name) __ATTR(_name, 0644, _name##_show, _name##_store)
 

-- 
2.50.1

Re: [PATCH v3 0/7] sysfs: prepare the constification of struct attribute

[Greg Kroah-Hartman]

On Mon, Aug 11, 2025 at 11:14:26AM +0200, Thomas Weißschuh wrote:
> The migration of 'struct attribute' and its related structures and
> macros are more complicated than those for 'struct bin_attribute'.
> Mostly because they are all shared by various custom attribute types.
> Introduce some initial utilities to support the migration.
> These are enough to migrate some specialized attributes atomically or
> those which don't use 'struct attribute' in their callbacks stepwise.
> 
> The big outstanding problems are 'struct device_attribute' and
> 'struct kobj_attribute'. These are used everywhere and I'm not yet sure
> about a migration plan.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
> Changes in v3:
> - Rebase von v6.17-rc1
> - Link to v2: https://lore.kernel.org/r/20250629-sysfs-const-attr-prep-v2-0-9ec5fe39083f@weissschuh.net

Sorry about the delay on my side for these, been swamped and then took a
vacation and now am trying to catch up.  Please give me some time to get
to them, they aren't lost and I really want to apply them...

thanks,

greg k-h

Re: [PATCH v3 1/7] sysfs: attribute_group: allow registration of const attribute

[Greg Kroah-Hartman]

On Mon, Aug 11, 2025 at 11:14:27AM +0200, Thomas Weißschuh wrote:
> To be able to constify instances of struct attribute it has to be
> possible to add them to struct attribute_group.
> The current type of the attrs member however is not compatible with that.
> Introduce a union that allows registration of both const and non-const
> attributes to enable a piecewise transition.
> As both union member types are compatible no logic needs to be adapted.
> 
> Technically it is now possible register a const struct
> attribute and receive it as mutable pointer in the callbacks.
> This is a soundness issue.
> But this same soundness issue already exists today in
> sysfs_create_file().
> Also the struct definition and callback implementation are always
> closely linked and are meant to be moved to const in lockstep.
> 
> Similar to commit 906c508afdca ("sysfs: attribute_group: allow registration of const bin_attribute")
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  include/linux/sysfs.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index f418aae4f1134f8126783d9e8eb575ba4278e927..a47092e837d9eb014894d1f7e49f0fd0f9a2e350 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -105,7 +105,10 @@ struct attribute_group {
>  	size_t			(*bin_size)(struct kobject *,
>  					    const struct bin_attribute *,
>  					    int);
> -	struct attribute	**attrs;
> +	union {
> +		struct attribute	**attrs;
> +		const struct attribute	*const *attrs_new;

I know you will drop the "_new" prefix after a while, but "new" is
relative, and not very descriptive.  How about "_const"?

> +	};
>  	union {
>  		const struct bin_attribute	*const *bin_attrs;
>  		const struct bin_attribute	*const *bin_attrs_new;

There is no bin_attrs_new anymore.  Finally.  sorry about that...

greg k-h

Re: [PATCH v3 5/7] samples/kobject: add is_visible() callback to attribute group

[Greg Kroah-Hartman]

On Mon, Aug 11, 2025 at 11:14:31AM +0200, Thomas Weißschuh wrote:
> There was no example for the is_visible() callback so far.
> 
> It will also become an example and test for the constification of
> 'struct attribute' later.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  samples/kobject/kset-example.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/samples/kobject/kset-example.c b/samples/kobject/kset-example.c
> index 579ce150217c6e613887e32a08206573543b3091..1aac595ed9498b30448485a60d9376cb5b5ea1d3 100644
> --- a/samples/kobject/kset-example.c
> +++ b/samples/kobject/kset-example.c
> @@ -178,7 +178,22 @@ static struct attribute *foo_default_attrs[] = {
>  	&bar_attribute.attr,
>  	NULL,	/* need to NULL terminate the list of attributes */
>  };
> -ATTRIBUTE_GROUPS(foo_default);
> +
> +static umode_t foo_default_attrs_is_visible(struct kobject *kobj,
> +					    struct attribute *attr,
> +					    int n)
> +{
> +	/* Hide attributes with the same name as the kobject. */
> +	if (strcmp(kobject_name(kobj), attr->name) == 0)
> +		return 0;
> +	return attr->mode;
> +}
> +
> +static const struct attribute_group foo_default_group = {
> +	.attrs		= foo_default_attrs,
> +	.is_visible	= foo_default_attrs_is_visible,
> +};
> +__ATTRIBUTE_GROUPS(foo_default);

Wait, why?  Shouldn't ATTRIBUTE_GROUPS() still work here?  No one should
have to call __ATTRIBUTE_GROUPS() in their code, that's just going to be
too messy over time.

thanks

greg k-h

Re: [PATCH v3 6/7] samples/kobject: constify 'struct foo_attribute'

[Greg Kroah-Hartman]

On Mon, Aug 11, 2025 at 11:14:32AM +0200, Thomas Weißschuh wrote:
> Showcase and test the new 'struct attribute' constification facilities.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  samples/kobject/kset-example.c | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/samples/kobject/kset-example.c b/samples/kobject/kset-example.c
> index 1aac595ed9498b30448485a60d9376cb5b5ea1d3..98aac6940f51f3312b44083a9d4ffe7afed1dba7 100644
> --- a/samples/kobject/kset-example.c
> +++ b/samples/kobject/kset-example.c
> @@ -37,10 +37,11 @@ struct foo_obj {
>  /* a custom attribute that works just for a struct foo_obj. */
>  struct foo_attribute {
>  	struct attribute attr;
> -	ssize_t (*show)(struct foo_obj *foo, struct foo_attribute *attr, char *buf);
> -	ssize_t (*store)(struct foo_obj *foo, struct foo_attribute *attr, const char *buf, size_t count);
> +	ssize_t (*show)(struct foo_obj *foo, const struct foo_attribute *attr, char *buf);
> +	ssize_t (*store)(struct foo_obj *foo, const struct foo_attribute *attr,
> +			 const char *buf, size_t count);
>  };
> -#define to_foo_attr(x) container_of(x, struct foo_attribute, attr)
> +#define to_foo_attr(x) container_of_const(x, struct foo_attribute, attr)
>  
>  /*
>   * The default show function that must be passed to sysfs.  This will be
> @@ -53,7 +54,7 @@ static ssize_t foo_attr_show(struct kobject *kobj,
>  			     struct attribute *attr,
>  			     char *buf)
>  {
> -	struct foo_attribute *attribute;
> +	const struct foo_attribute *attribute;
>  	struct foo_obj *foo;
>  
>  	attribute = to_foo_attr(attr);
> @@ -73,7 +74,7 @@ static ssize_t foo_attr_store(struct kobject *kobj,
>  			      struct attribute *attr,
>  			      const char *buf, size_t len)
>  {
> -	struct foo_attribute *attribute;
> +	const struct foo_attribute *attribute;
>  	struct foo_obj *foo;
>  
>  	attribute = to_foo_attr(attr);
> @@ -109,13 +110,13 @@ static void foo_release(struct kobject *kobj)
>  /*
>   * The "foo" file where the .foo variable is read from and written to.
>   */
> -static ssize_t foo_show(struct foo_obj *foo_obj, struct foo_attribute *attr,
> +static ssize_t foo_show(struct foo_obj *foo_obj, const struct foo_attribute *attr,
>  			char *buf)
>  {
>  	return sysfs_emit(buf, "%d\n", foo_obj->foo);
>  }
>  
> -static ssize_t foo_store(struct foo_obj *foo_obj, struct foo_attribute *attr,
> +static ssize_t foo_store(struct foo_obj *foo_obj, const struct foo_attribute *attr,
>  			 const char *buf, size_t count)
>  {
>  	int ret;
> @@ -128,14 +129,14 @@ static ssize_t foo_store(struct foo_obj *foo_obj, struct foo_attribute *attr,
>  }
>  
>  /* Sysfs attributes cannot be world-writable. */
> -static struct foo_attribute foo_attribute =
> +static const struct foo_attribute foo_attribute =
>  	__ATTR(foo, 0664, foo_show, foo_store);
>  
>  /*
>   * More complex function where we determine which variable is being accessed by
>   * looking at the attribute for the "baz" and "bar" files.
>   */
> -static ssize_t b_show(struct foo_obj *foo_obj, struct foo_attribute *attr,
> +static ssize_t b_show(struct foo_obj *foo_obj, const struct foo_attribute *attr,
>  		      char *buf)
>  {
>  	int var;
> @@ -147,7 +148,7 @@ static ssize_t b_show(struct foo_obj *foo_obj, struct foo_attribute *attr,
>  	return sysfs_emit(buf, "%d\n", var);
>  }
>  
> -static ssize_t b_store(struct foo_obj *foo_obj, struct foo_attribute *attr,
> +static ssize_t b_store(struct foo_obj *foo_obj, const struct foo_attribute *attr,
>  		       const char *buf, size_t count)
>  {
>  	int var, ret;
> @@ -163,16 +164,16 @@ static ssize_t b_store(struct foo_obj *foo_obj, struct foo_attribute *attr,
>  	return count;
>  }
>  
> -static struct foo_attribute baz_attribute =
> +static const struct foo_attribute baz_attribute =
>  	__ATTR(baz, 0664, b_show, b_store);

We really should make this ATTR_RW() one of these days.

> -static struct foo_attribute bar_attribute =
> +static const struct foo_attribute bar_attribute =
>  	__ATTR(bar, 0664, b_show, b_store);

This one too.

Actually almost all existing users of __ATTR() should be fixed up so we
can just remove that (special modes can use the __ATTR_*_MODE() macro)

But that's separate from this series, sorry for the noise.

greg k-h

Re: [PATCH v3 4/7] sysfs: attribute_group: enable const variants of is_visible()

[Greg Kroah-Hartman]

On Mon, Aug 11, 2025 at 11:14:30AM +0200, Thomas Weißschuh wrote:
> When constifying instances of struct attribute, for consistency the
> corresponding .is_visible() callback should be adapted, too.
> Introduce a temporary transition mechanism until all callbacks are
> converted.

I count about 600+ users of is_visible() now, how is that going to be
converted?  It feels like a lot of churn/work, what's the plan here?

These changes seem a lot more intrusive than the bin_attr ones :(

thanks,

greg k-h

Re: [PATCH v3 1/7] sysfs: attribute_group: allow registration of const attribute

[Thomas Weißschuh]

On 2025-08-19 13:22:55+0200, Greg Kroah-Hartman wrote:
> On Mon, Aug 11, 2025 at 11:14:27AM +0200, Thomas Weißschuh wrote:
> > To be able to constify instances of struct attribute it has to be
> > possible to add them to struct attribute_group.
> > The current type of the attrs member however is not compatible with that.
> > Introduce a union that allows registration of both const and non-const
> > attributes to enable a piecewise transition.
> > As both union member types are compatible no logic needs to be adapted.
> > 
> > Technically it is now possible register a const struct
> > attribute and receive it as mutable pointer in the callbacks.
> > This is a soundness issue.
> > But this same soundness issue already exists today in
> > sysfs_create_file().
> > Also the struct definition and callback implementation are always
> > closely linked and are meant to be moved to const in lockstep.
> > 
> > Similar to commit 906c508afdca ("sysfs: attribute_group: allow registration of const bin_attribute")
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> >  include/linux/sysfs.h | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> > index f418aae4f1134f8126783d9e8eb575ba4278e927..a47092e837d9eb014894d1f7e49f0fd0f9a2e350 100644
> > --- a/include/linux/sysfs.h
> > +++ b/include/linux/sysfs.h
> > @@ -105,7 +105,10 @@ struct attribute_group {
> >  	size_t			(*bin_size)(struct kobject *,
> >  					    const struct bin_attribute *,
> >  					    int);
> > -	struct attribute	**attrs;
> > +	union {
> > +		struct attribute	**attrs;
> > +		const struct attribute	*const *attrs_new;
> 
> I know you will drop the "_new" prefix after a while, but "new" is
> relative, and not very descriptive.

That is somewhat intentional to express that it is a transitional thing.

> How about "_const"?

At some point the regular variant will be const too, so "_const" would
be a bit weird.

> > +	};
> >  	union {
> >  		const struct bin_attribute	*const *bin_attrs;
> >  		const struct bin_attribute	*const *bin_attrs_new;
> 
> There is no bin_attrs_new anymore.  Finally.  sorry about that...

Thanks! No worries.


Thomas

Re: [PATCH v3 5/7] samples/kobject: add is_visible() callback to attribute group

[Thomas Weißschuh]

On 2025-08-19 13:24:04+0200, Greg Kroah-Hartman wrote:
> On Mon, Aug 11, 2025 at 11:14:31AM +0200, Thomas Weißschuh wrote:
> > There was no example for the is_visible() callback so far.
> > 
> > It will also become an example and test for the constification of
> > 'struct attribute' later.
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> >  samples/kobject/kset-example.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/samples/kobject/kset-example.c b/samples/kobject/kset-example.c
> > index 579ce150217c6e613887e32a08206573543b3091..1aac595ed9498b30448485a60d9376cb5b5ea1d3 100644
> > --- a/samples/kobject/kset-example.c
> > +++ b/samples/kobject/kset-example.c
> > @@ -178,7 +178,22 @@ static struct attribute *foo_default_attrs[] = {
> >  	&bar_attribute.attr,
> >  	NULL,	/* need to NULL terminate the list of attributes */
> >  };
> > -ATTRIBUTE_GROUPS(foo_default);
> > +
> > +static umode_t foo_default_attrs_is_visible(struct kobject *kobj,
> > +					    struct attribute *attr,
> > +					    int n)
> > +{
> > +	/* Hide attributes with the same name as the kobject. */
> > +	if (strcmp(kobject_name(kobj), attr->name) == 0)
> > +		return 0;
> > +	return attr->mode;
> > +}
> > +
> > +static const struct attribute_group foo_default_group = {
> > +	.attrs		= foo_default_attrs,
> > +	.is_visible	= foo_default_attrs_is_visible,
> > +};
> > +__ATTRIBUTE_GROUPS(foo_default);
> 
> Wait, why?  Shouldn't ATTRIBUTE_GROUPS() still work here?  No one should
> have to call __ATTRIBUTE_GROUPS() in their code, that's just going to be
> too messy over time.

ATTRIBUTE_GROUPS() can not handle .is_visible().
There are already a few users throughout the tree.

Thomas

Re: [PATCH v3 4/7] sysfs: attribute_group: enable const variants of is_visible()

[Thomas Weißschuh]

On 2025-08-19 13:28:40+0200, Greg Kroah-Hartman wrote:
> On Mon, Aug 11, 2025 at 11:14:30AM +0200, Thomas Weißschuh wrote:
> > When constifying instances of struct attribute, for consistency the
> > corresponding .is_visible() callback should be adapted, too.
> > Introduce a temporary transition mechanism until all callbacks are
> > converted.
> 
> I count about 600+ users of is_visible() now, how is that going to be
> converted?  It feels like a lot of churn/work, what's the plan here?

The idea was to convert one driver at a time to the const variants.
Adapting is_visible(), read(), write() and the group structs
together. And then submit these batched per subsystem.
It's not a purely mechanical change as the users may modify their
attributes.

It will be a lot of churn. is_visible() is not even the big one.

> These changes seem a lot more intrusive than the bin_attr ones :(

Indeed :-/


Thomas

Re: [PATCH v3 1/7] sysfs: attribute_group: allow registration of const attribute

[Greg Kroah-Hartman]

On Tue, Aug 19, 2025 at 03:59:04PM +0200, Thomas Weißschuh wrote:
> On 2025-08-19 13:22:55+0200, Greg Kroah-Hartman wrote:
> > On Mon, Aug 11, 2025 at 11:14:27AM +0200, Thomas Weißschuh wrote:
> > > To be able to constify instances of struct attribute it has to be
> > > possible to add them to struct attribute_group.
> > > The current type of the attrs member however is not compatible with that.
> > > Introduce a union that allows registration of both const and non-const
> > > attributes to enable a piecewise transition.
> > > As both union member types are compatible no logic needs to be adapted.
> > > 
> > > Technically it is now possible register a const struct
> > > attribute and receive it as mutable pointer in the callbacks.
> > > This is a soundness issue.
> > > But this same soundness issue already exists today in
> > > sysfs_create_file().
> > > Also the struct definition and callback implementation are always
> > > closely linked and are meant to be moved to const in lockstep.
> > > 
> > > Similar to commit 906c508afdca ("sysfs: attribute_group: allow registration of const bin_attribute")
> > > 
> > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > > ---
> > >  include/linux/sysfs.h | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> > > index f418aae4f1134f8126783d9e8eb575ba4278e927..a47092e837d9eb014894d1f7e49f0fd0f9a2e350 100644
> > > --- a/include/linux/sysfs.h
> > > +++ b/include/linux/sysfs.h
> > > @@ -105,7 +105,10 @@ struct attribute_group {
> > >  	size_t			(*bin_size)(struct kobject *,
> > >  					    const struct bin_attribute *,
> > >  					    int);
> > > -	struct attribute	**attrs;
> > > +	union {
> > > +		struct attribute	**attrs;
> > > +		const struct attribute	*const *attrs_new;
> > 
> > I know you will drop the "_new" prefix after a while, but "new" is
> > relative, and not very descriptive.
> 
> That is somewhat intentional to express that it is a transitional thing.

Fair, but given the huge quantity here, it's going to take a long time,
so "new" is going to be rough to push through for 6+ months.

> > How about "_const"?
> 
> At some point the regular variant will be const too, so "_const" would
> be a bit weird.

Yes, that's when you "switch it back", right?  You would have to do that
for _new as well.

thanks,

greg k-h

Re: [PATCH v3 1/7] sysfs: attribute_group: allow registration of const attribute

[Thomas Weißschuh]

On 2025-08-19 16:10:42+0200, Greg Kroah-Hartman wrote:
> On Tue, Aug 19, 2025 at 03:59:04PM +0200, Thomas Weißschuh wrote:
> > On 2025-08-19 13:22:55+0200, Greg Kroah-Hartman wrote:
> > > On Mon, Aug 11, 2025 at 11:14:27AM +0200, Thomas Weißschuh wrote:
> > > > To be able to constify instances of struct attribute it has to be
> > > > possible to add them to struct attribute_group.
> > > > The current type of the attrs member however is not compatible with that.
> > > > Introduce a union that allows registration of both const and non-const
> > > > attributes to enable a piecewise transition.
> > > > As both union member types are compatible no logic needs to be adapted.
> > > > 
> > > > Technically it is now possible register a const struct
> > > > attribute and receive it as mutable pointer in the callbacks.
> > > > This is a soundness issue.
> > > > But this same soundness issue already exists today in
> > > > sysfs_create_file().
> > > > Also the struct definition and callback implementation are always
> > > > closely linked and are meant to be moved to const in lockstep.
> > > > 
> > > > Similar to commit 906c508afdca ("sysfs: attribute_group: allow registration of const bin_attribute")
> > > > 
> > > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > > > ---
> > > >  include/linux/sysfs.h | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> > > > index f418aae4f1134f8126783d9e8eb575ba4278e927..a47092e837d9eb014894d1f7e49f0fd0f9a2e350 100644
> > > > --- a/include/linux/sysfs.h
> > > > +++ b/include/linux/sysfs.h
> > > > @@ -105,7 +105,10 @@ struct attribute_group {
> > > >  	size_t			(*bin_size)(struct kobject *,
> > > >  					    const struct bin_attribute *,
> > > >  					    int);
> > > > -	struct attribute	**attrs;
> > > > +	union {
> > > > +		struct attribute	**attrs;
> > > > +		const struct attribute	*const *attrs_new;
> > > 
> > > I know you will drop the "_new" prefix after a while, but "new" is
> > > relative, and not very descriptive.
> > 
> > That is somewhat intentional to express that it is a transitional thing.
> 
> Fair, but given the huge quantity here, it's going to take a long time,
> so "new" is going to be rough to push through for 6+ months.

Looking at how 'struct bin_attribute' went probably quite a bit longer.

> > > How about "_const"?
> > 
> > At some point the regular variant will be const too, so "_const" would
> > be a bit weird.
> 
> Yes, that's when you "switch it back", right?  You would have to do that
> for _new as well.

There will probably be some overlap. But in the end it probably
doesn't matter. Let's go with "_const".

Thomas