[PATCH 0/2] sysfs: attribute_group: allow registration of const bin_attribute

[PATCH 0/2] sysfs: attribute_group: allow registration of const bin_attribute

[Thomas Weißschuh]

A small addition to the sysfs bin_attribute constification preparation.
While it's very late, it would be great if this could make it into 6.13
at it would speed up the tree-wide conversion process.

Based on driver-core/driver-core-next .

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Thomas Weißschuh (2):
      sysfs: attribute_group: allow registration of const bin_attribute
      driver core: Constify bin_attribute definitions

 drivers/base/node.c     |  8 ++++----
 drivers/base/topology.c | 36 ++++++++++++++++++------------------
 include/linux/sysfs.h   |  5 ++++-
 3 files changed, 26 insertions(+), 23 deletions(-)
---
base-commit: 369a9c046c2fdfe037f05b43b84c386bdbccc103
change-id: 20241114-b4-sysfs-const-bin_attr-group-0b784e775144
prerequisite-change-id: 20241028-sysfs-const-bin_attr-a00896481d0b:v2
prerequisite-patch-id: 3173126c01d3db719968b1dc25fbe44ec47215e5
prerequisite-patch-id: c3333053bd7fe677c526715dc6f88d5a7d863e04
prerequisite-patch-id: ae580d9ecb858d8c236a33f272708cac22a7bd9c
prerequisite-patch-id: 1f4297cf4707d54a1da28cd7dbe469590997bd53
prerequisite-patch-id: f5733e7d3f85bb2814cb803fbab68ad8cba316fd
prerequisite-patch-id: bfb9e4123f5e6e5aef82981552839b02cc2edc4a
prerequisite-patch-id: b0f61ee928efc513c42a157e4903e874d52bc57c
prerequisite-patch-id: 5e636e5921603f3c9f4855a4041442396893562e
prerequisite-patch-id: 81e7e41f68a963066675a5a5f669ded9d6485aa8
prerequisite-patch-id: 884429fbd146708c15464a007546dffafe51d060

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

[PATCH 1/2] sysfs: attribute_group: allow registration of const bin_attribute

[Thomas Weißschuh]

To be able to constify instances of struct bin_attribute it has to be
possible to add them to string attribute_group.
The current type of the bin_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
bin_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_bin_file().
Also the struct definition and callback implementation are always
closely linked and are meant to be moved to const in lockstep.

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 d713a6445a6267145a7014f308df3bb25b8c3287..0f2fcd244523f050c5286f19d4fe1846506f9214 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -106,7 +106,10 @@ struct attribute_group {
 					    const struct bin_attribute *,
 					    int);
 	struct attribute	**attrs;
-	struct bin_attribute	**bin_attrs;
+	union {
+		struct bin_attribute		**bin_attrs;
+		const struct bin_attribute	*const *bin_attrs_new;
+	};
 };
 
 #define SYSFS_PREALLOC		010000

-- 
2.47.0

Re: [PATCH 1/2] sysfs: attribute_group: allow registration of const bin_attribute

[Thomas Weißschuh]

Hi Greg,

On 2024-11-15 17:42:48+0100, Thomas Weißschuh wrote:

> [..]

> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index d713a6445a6267145a7014f308df3bb25b8c3287..0f2fcd244523f050c5286f19d4fe1846506f9214 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -106,7 +106,10 @@ struct attribute_group {
>  					    const struct bin_attribute *,
>  					    int);
>  	struct attribute	**attrs;
> -	struct bin_attribute	**bin_attrs;
> +	union {
> +		struct bin_attribute		**bin_attrs;
> +		const struct bin_attribute	*const *bin_attrs_new;
> +	};

Unfortunately this triggers warnings in two drivers.
These incorrectly have a trailing NULL literal in their struct attribute
definition (full list at the end of the mail):

>> drivers/perf/arm-ni.c:248:63: warning: missing braces around initializer [-Wmissing-braces]
     248 | static const struct attribute_group arm_ni_other_attr_group = {
         |                                                               ^


vim +248 drivers/perf/arm-ni.c

4d5a7680f2b4d0 Robin Murphy 2024-09-04  247
4d5a7680f2b4d0 Robin Murphy 2024-09-04 @248  static const struct attribute_group arm_ni_other_attr_group = {
4d5a7680f2b4d0 Robin Murphy 2024-09-04  249  	.attrs = arm_ni_other_attrs,
4d5a7680f2b4d0 Robin Murphy 2024-09-04  250  	NULL
4d5a7680f2b4d0 Robin Murphy 2024-09-04  251  };
4d5a7680f2b4d0 Robin Murphy 2024-09-04  252

These trailing NULLs should first be removed.
How do you want to proceed?

Cocci script and results, only the first two results are relevant at
this moment.

	virtual patch

	@@
	identifier ag, pattrs;
	@@

	  struct attribute_group ag = {
	    .attrs = pattrs,
	-   NULL
	  };

diff -u -p a/drivers/s390/char/con3215.c b/drivers/s390/char/con3215.c
--- a/drivers/s390/char/con3215.c
+++ b/drivers/s390/char/con3215.c
@@ -803,7 +803,6 @@ static struct attribute *con3215_drv_att

 static struct attribute_group con3215_drv_attr_group = {
 	.attrs = con3215_drv_attrs,
-	NULL,
 };

 static const struct attribute_group *con3215_drv_attr_groups[] = {
diff -u -p a/drivers/perf/arm-ni.c b/drivers/perf/arm-ni.c
--- a/drivers/perf/arm-ni.c
+++ b/drivers/perf/arm-ni.c
@@ -247,7 +247,6 @@ static struct attribute *arm_ni_other_at

 static const struct attribute_group arm_ni_other_attr_group = {
 	.attrs = arm_ni_other_attrs,
-	NULL
 };

 static const struct attribute_group *arm_ni_attr_groups[] = {
diff -u -p a/kernel/cpu.c b/kernel/cpu.c
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -2866,7 +2866,6 @@ static struct attribute *cpuhp_cpu_attrs
 static const struct attribute_group cpuhp_cpu_attr_group = {
 	.attrs = cpuhp_cpu_attrs,
 	.name = "hotplug",
-	NULL
 };

 static ssize_t states_show(struct device *dev,
@@ -2898,7 +2897,6 @@ static struct attribute *cpuhp_cpu_root_
 static const struct attribute_group cpuhp_cpu_root_attr_group = {
 	.attrs = cpuhp_cpu_root_attrs,
 	.name = "hotplug",
-	NULL
 };

 #ifdef CONFIG_HOTPLUG_SMT
@@ -3020,7 +3018,6 @@ static struct attribute *cpuhp_smt_attrs
 static const struct attribute_group cpuhp_smt_attr_group = {
 	.attrs = cpuhp_smt_attrs,
 	.name = "smt",
-	NULL
 };

 static int __init cpu_smt_sysfs_init(void)

Re: [PATCH 1/2] sysfs: attribute_group: allow registration of const bin_attribute

[Greg Kroah-Hartman]

On Mon, Nov 18, 2024 at 12:36:06PM +0100, Thomas Weißschuh wrote:
> Hi Greg,
> 
> On 2024-11-15 17:42:48+0100, Thomas Weißschuh wrote:
> 
> > [..]
> 
> > diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> > index d713a6445a6267145a7014f308df3bb25b8c3287..0f2fcd244523f050c5286f19d4fe1846506f9214 100644
> > --- a/include/linux/sysfs.h
> > +++ b/include/linux/sysfs.h
> > @@ -106,7 +106,10 @@ struct attribute_group {
> >  					    const struct bin_attribute *,
> >  					    int);
> >  	struct attribute	**attrs;
> > -	struct bin_attribute	**bin_attrs;
> > +	union {
> > +		struct bin_attribute		**bin_attrs;
> > +		const struct bin_attribute	*const *bin_attrs_new;
> > +	};
> 
> Unfortunately this triggers warnings in two drivers.
> These incorrectly have a trailing NULL literal in their struct attribute
> definition (full list at the end of the mail):
> 
> >> drivers/perf/arm-ni.c:248:63: warning: missing braces around initializer [-Wmissing-braces]
>      248 | static const struct attribute_group arm_ni_other_attr_group = {
>          |                                                               ^
> 
> 
> vim +248 drivers/perf/arm-ni.c
> 
> 4d5a7680f2b4d0 Robin Murphy 2024-09-04  247
> 4d5a7680f2b4d0 Robin Murphy 2024-09-04 @248  static const struct attribute_group arm_ni_other_attr_group = {
> 4d5a7680f2b4d0 Robin Murphy 2024-09-04  249  	.attrs = arm_ni_other_attrs,
> 4d5a7680f2b4d0 Robin Murphy 2024-09-04  250  	NULL
> 4d5a7680f2b4d0 Robin Murphy 2024-09-04  251  };
> 4d5a7680f2b4d0 Robin Murphy 2024-09-04  252
> 
> These trailing NULLs should first be removed.
> How do you want to proceed?

Odd, it passed 0-day testing.

Just send me a patch to fix up these obvious problems, strange it built
in the first place (it's a mix of named and not named identifiers, I
thought the compiler would complain about that...)

> Cocci script and results, only the first two results are relevant at
> this moment.
> 
> 	virtual patch
> 
> 	@@
> 	identifier ag, pattrs;
> 	@@
> 
> 	  struct attribute_group ag = {
> 	    .attrs = pattrs,
> 	-   NULL
> 	  };
> 
> diff -u -p a/drivers/s390/char/con3215.c b/drivers/s390/char/con3215.c
> --- a/drivers/s390/char/con3215.c
> +++ b/drivers/s390/char/con3215.c
> @@ -803,7 +803,6 @@ static struct attribute *con3215_drv_att
> 
>  static struct attribute_group con3215_drv_attr_group = {
>  	.attrs = con3215_drv_attrs,
> -	NULL,
>  };
> 
>  static const struct attribute_group *con3215_drv_attr_groups[] = {
> diff -u -p a/drivers/perf/arm-ni.c b/drivers/perf/arm-ni.c
> --- a/drivers/perf/arm-ni.c
> +++ b/drivers/perf/arm-ni.c
> @@ -247,7 +247,6 @@ static struct attribute *arm_ni_other_at
> 
>  static const struct attribute_group arm_ni_other_attr_group = {
>  	.attrs = arm_ni_other_attrs,
> -	NULL
>  };
> 
>  static const struct attribute_group *arm_ni_attr_groups[] = {
> diff -u -p a/kernel/cpu.c b/kernel/cpu.c
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -2866,7 +2866,6 @@ static struct attribute *cpuhp_cpu_attrs
>  static const struct attribute_group cpuhp_cpu_attr_group = {
>  	.attrs = cpuhp_cpu_attrs,
>  	.name = "hotplug",
> -	NULL
>  };
> 
>  static ssize_t states_show(struct device *dev,
> @@ -2898,7 +2897,6 @@ static struct attribute *cpuhp_cpu_root_
>  static const struct attribute_group cpuhp_cpu_root_attr_group = {
>  	.attrs = cpuhp_cpu_root_attrs,
>  	.name = "hotplug",
> -	NULL
>  };
> 
>  #ifdef CONFIG_HOTPLUG_SMT
> @@ -3020,7 +3018,6 @@ static struct attribute *cpuhp_smt_attrs
>  static const struct attribute_group cpuhp_smt_attr_group = {
>  	.attrs = cpuhp_smt_attrs,
>  	.name = "smt",
> -	NULL
>  };
> 
>  static int __init cpu_smt_sysfs_init(void)

Looks sane, send me a patch?

thanks,

greg k-h

[PATCH 2/2] driver core: Constify bin_attribute definitions

[Thomas Weißschuh]

Mark all 'struct bin_attribute' instances as const to protect against
accidental and malicious modifications.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/base/node.c     |  8 ++++----
 drivers/base/topology.c | 36 ++++++++++++++++++------------------
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/base/node.c b/drivers/base/node.c
index 3e761633ac75826bedb5dd30b879f7cc1af95ec3..0ea653fa34330eb2d89daae455b4b9f101b79f4e 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -45,7 +45,7 @@ static inline ssize_t cpumap_read(struct file *file, struct kobject *kobj,
 	return n;
 }
 
-static BIN_ATTR_RO(cpumap, CPUMAP_FILE_MAX_BYTES);
+static const BIN_ATTR_RO(cpumap, CPUMAP_FILE_MAX_BYTES);
 
 static inline ssize_t cpulist_read(struct file *file, struct kobject *kobj,
 				   const struct bin_attribute *attr, char *buf,
@@ -66,7 +66,7 @@ static inline ssize_t cpulist_read(struct file *file, struct kobject *kobj,
 	return n;
 }
 
-static BIN_ATTR_RO(cpulist, CPULIST_FILE_MAX_BYTES);
+static const BIN_ATTR_RO(cpulist, CPULIST_FILE_MAX_BYTES);
 
 /**
  * struct node_access_nodes - Access class device to hold user visible
@@ -578,7 +578,7 @@ static struct attribute *node_dev_attrs[] = {
 	NULL
 };
 
-static struct bin_attribute *node_dev_bin_attrs[] = {
+static const struct bin_attribute *node_dev_bin_attrs[] = {
 	&bin_attr_cpumap,
 	&bin_attr_cpulist,
 	NULL
@@ -586,7 +586,7 @@ static struct bin_attribute *node_dev_bin_attrs[] = {
 
 static const struct attribute_group node_dev_group = {
 	.attrs = node_dev_attrs,
-	.bin_attrs = node_dev_bin_attrs
+	.bin_attrs_new = node_dev_bin_attrs,
 };
 
 static const struct attribute_group *node_dev_groups[] = {
diff --git a/drivers/base/topology.c b/drivers/base/topology.c
index 1090751d7f458ce8d2a50e82d65b8ce31e938f15..cf160dd2c27bd7302bb8d181ca54ec9aa840fe1c 100644
--- a/drivers/base/topology.c
+++ b/drivers/base/topology.c
@@ -62,50 +62,50 @@ define_id_show_func(ppin, "0x%llx");
 static DEVICE_ATTR_ADMIN_RO(ppin);
 
 define_siblings_read_func(thread_siblings, sibling_cpumask);
-static BIN_ATTR_RO(thread_siblings, CPUMAP_FILE_MAX_BYTES);
-static BIN_ATTR_RO(thread_siblings_list, CPULIST_FILE_MAX_BYTES);
+static const BIN_ATTR_RO(thread_siblings, CPUMAP_FILE_MAX_BYTES);
+static const BIN_ATTR_RO(thread_siblings_list, CPULIST_FILE_MAX_BYTES);
 
 define_siblings_read_func(core_cpus, sibling_cpumask);
-static BIN_ATTR_RO(core_cpus, CPUMAP_FILE_MAX_BYTES);
-static BIN_ATTR_RO(core_cpus_list, CPULIST_FILE_MAX_BYTES);
+static const BIN_ATTR_RO(core_cpus, CPUMAP_FILE_MAX_BYTES);
+static const BIN_ATTR_RO(core_cpus_list, CPULIST_FILE_MAX_BYTES);
 
 define_siblings_read_func(core_siblings, core_cpumask);
-static BIN_ATTR_RO(core_siblings, CPUMAP_FILE_MAX_BYTES);
-static BIN_ATTR_RO(core_siblings_list, CPULIST_FILE_MAX_BYTES);
+static const BIN_ATTR_RO(core_siblings, CPUMAP_FILE_MAX_BYTES);
+static const BIN_ATTR_RO(core_siblings_list, CPULIST_FILE_MAX_BYTES);
 
 #ifdef TOPOLOGY_CLUSTER_SYSFS
 define_siblings_read_func(cluster_cpus, cluster_cpumask);
-static BIN_ATTR_RO(cluster_cpus, CPUMAP_FILE_MAX_BYTES);
-static BIN_ATTR_RO(cluster_cpus_list, CPULIST_FILE_MAX_BYTES);
+static const BIN_ATTR_RO(cluster_cpus, CPUMAP_FILE_MAX_BYTES);
+static const BIN_ATTR_RO(cluster_cpus_list, CPULIST_FILE_MAX_BYTES);
 #endif
 
 #ifdef TOPOLOGY_DIE_SYSFS
 define_siblings_read_func(die_cpus, die_cpumask);
-static BIN_ATTR_RO(die_cpus, CPUMAP_FILE_MAX_BYTES);
-static BIN_ATTR_RO(die_cpus_list, CPULIST_FILE_MAX_BYTES);
+static const BIN_ATTR_RO(die_cpus, CPUMAP_FILE_MAX_BYTES);
+static const BIN_ATTR_RO(die_cpus_list, CPULIST_FILE_MAX_BYTES);
 #endif
 
 define_siblings_read_func(package_cpus, core_cpumask);
-static BIN_ATTR_RO(package_cpus, CPUMAP_FILE_MAX_BYTES);
-static BIN_ATTR_RO(package_cpus_list, CPULIST_FILE_MAX_BYTES);
+static const BIN_ATTR_RO(package_cpus, CPUMAP_FILE_MAX_BYTES);
+static const BIN_ATTR_RO(package_cpus_list, CPULIST_FILE_MAX_BYTES);
 
 #ifdef TOPOLOGY_BOOK_SYSFS
 define_id_show_func(book_id, "%d");
 static DEVICE_ATTR_RO(book_id);
 define_siblings_read_func(book_siblings, book_cpumask);
-static BIN_ATTR_RO(book_siblings, CPUMAP_FILE_MAX_BYTES);
-static BIN_ATTR_RO(book_siblings_list, CPULIST_FILE_MAX_BYTES);
+static const BIN_ATTR_RO(book_siblings, CPUMAP_FILE_MAX_BYTES);
+static const BIN_ATTR_RO(book_siblings_list, CPULIST_FILE_MAX_BYTES);
 #endif
 
 #ifdef TOPOLOGY_DRAWER_SYSFS
 define_id_show_func(drawer_id, "%d");
 static DEVICE_ATTR_RO(drawer_id);
 define_siblings_read_func(drawer_siblings, drawer_cpumask);
-static BIN_ATTR_RO(drawer_siblings, CPUMAP_FILE_MAX_BYTES);
-static BIN_ATTR_RO(drawer_siblings_list, CPULIST_FILE_MAX_BYTES);
+static const BIN_ATTR_RO(drawer_siblings, CPUMAP_FILE_MAX_BYTES);
+static const BIN_ATTR_RO(drawer_siblings_list, CPULIST_FILE_MAX_BYTES);
 #endif
 
-static struct bin_attribute *bin_attrs[] = {
+static const struct bin_attribute *const bin_attrs[] = {
 	&bin_attr_core_cpus,
 	&bin_attr_core_cpus_list,
 	&bin_attr_thread_siblings,
@@ -163,7 +163,7 @@ static umode_t topology_is_visible(struct kobject *kobj,
 
 static const struct attribute_group topology_attr_group = {
 	.attrs = default_attrs,
-	.bin_attrs = bin_attrs,
+	.bin_attrs_new = bin_attrs,
 	.is_visible = topology_is_visible,
 	.name = "topology"
 };

-- 
2.47.0

Re: [PATCH 0/2] sysfs: attribute_group: allow registration of const bin_attribute

[Greg Kroah-Hartman]

On Fri, Nov 15, 2024 at 05:42:47PM +0100, Thomas Weißschuh wrote:
> A small addition to the sysfs bin_attribute constification preparation.
> While it's very late, it would be great if this could make it into 6.13
> at it would speed up the tree-wide conversion process.

Looks good, now queued up for 0-day testing, thanks!

greg k-h