Reading sys/devices/system/cpu/cpuX/topology/ returns cpu topology. However, the size of this file is limited to PAGE_SIZE because of the limitation for sysfs attribute. so we use bin_attribute instead of attribute to avoid NR_CPUS too big to cause buff overflow.
This patch is based on the following discussion. https://www.spinics.net/lists/linux-doc/msg95921.html
Signed-off-by: Tian Tao tiantao6@hisilicon.com --- v2: rewrite the function name##_read v3: remove the temp buffer. v4: rewrite the function name##_read v5: rewrite the patch v6: update the commit message v7: remove BUFF_SIZE, dynamic calculation of size --- drivers/base/topology.c | 144 +++++++++++++++++++++++++++++++----------------- 1 file changed, 94 insertions(+), 50 deletions(-)
diff --git a/drivers/base/topology.c b/drivers/base/topology.c index 4d254fc..883fa39 100644 --- a/drivers/base/topology.c +++ b/drivers/base/topology.c @@ -21,24 +21,59 @@ static ssize_t name##_show(struct device *dev, \ return sysfs_emit(buf, "%d\n", topology_##name(dev->id)); \ }
-#define define_siblings_show_map(name, mask) \ -static ssize_t name##_show(struct device *dev, \ - struct device_attribute *attr, char *buf) \ -{ \ - return cpumap_print_to_pagebuf(false, buf, topology_##mask(dev->id));\ -} +static int topology_snprintf(char *buf, ssize_t size, const char *fmt, ...) +{ + va_list args; + int i;
-#define define_siblings_show_list(name, mask) \ -static ssize_t name##_list_show(struct device *dev, \ - struct device_attribute *attr, \ - char *buf) \ -{ \ - return cpumap_print_to_pagebuf(true, buf, topology_##mask(dev->id));\ + va_start(args, fmt); + if (size <= 0) + i = vsnprintf(NULL, 0, fmt, args); + else + i = vscnprintf(buf, size, fmt, args); + va_end(args); + + return i; }
-#define define_siblings_show_func(name, mask) \ - define_siblings_show_map(name, mask); \ - define_siblings_show_list(name, mask) +#define define_siblings_read_func(name, mask) \ +static ssize_t name##_read(struct file *file, struct kobject *kobj, \ + struct bin_attribute *attr, char *buf, \ + loff_t off, size_t count) \ +{ \ + struct device *dev = kobj_to_dev(kobj); \ + int len; \ + void *data; \ + len = topology_snprintf(NULL, 0, "%*pb\n", nr_cpu_ids, \ + cpumask_bits(topology_##mask(dev->id))); \ + data = kvmalloc(len+1, GFP_KERNEL); \ + if (!data) \ + return -ENOMEM; \ + len = topology_snprintf(data, len+1, "%*pb\n", nr_cpu_ids, \ + cpumask_bits(topology_##mask(dev->id))); \ + len = memory_read_from_buffer(buf, count, &off, data, len); \ + kvfree(data); \ + return len; \ +} \ + \ +static ssize_t name##_list_read(struct file *file, struct kobject *kobj, \ + struct bin_attribute *attr, char *buf, \ + loff_t off, size_t count) \ +{ \ + struct device *dev = kobj_to_dev(kobj); \ + int len; \ + void *data; \ + len = topology_snprintf(NULL, 0, "%*pbl\n", nr_cpu_ids, \ + cpumask_bits(topology_##mask(dev->id))); \ + data = kvmalloc(len+1, GFP_KERNEL); \ + if (!data) \ + return -ENOMEM; \ + len = topology_snprintf(data, len+1, "%*pbl\n", nr_cpu_ids, \ + cpumask_bits(topology_##mask(dev->id))); \ + len = memory_read_from_buffer(buf, count, &off, data, len); \ + kvfree(data); \ + return len; \ +}
define_id_show_func(physical_package_id); static DEVICE_ATTR_RO(physical_package_id); @@ -49,71 +84,80 @@ static DEVICE_ATTR_RO(die_id); define_id_show_func(core_id); static DEVICE_ATTR_RO(core_id);
-define_siblings_show_func(thread_siblings, sibling_cpumask); -static DEVICE_ATTR_RO(thread_siblings); -static DEVICE_ATTR_RO(thread_siblings_list); +define_siblings_read_func(thread_siblings, sibling_cpumask); +static BIN_ATTR_RO(thread_siblings, 0); +static BIN_ATTR_RO(thread_siblings_list, 0);
-define_siblings_show_func(core_cpus, sibling_cpumask); -static DEVICE_ATTR_RO(core_cpus); -static DEVICE_ATTR_RO(core_cpus_list); +define_siblings_read_func(core_cpus, sibling_cpumask); +static BIN_ATTR_RO(core_cpus, 0); +static BIN_ATTR_RO(core_cpus_list, 0);
-define_siblings_show_func(core_siblings, core_cpumask); -static DEVICE_ATTR_RO(core_siblings); -static DEVICE_ATTR_RO(core_siblings_list); +define_siblings_read_func(core_siblings, core_cpumask); +static BIN_ATTR_RO(core_siblings, 0); +static BIN_ATTR_RO(core_siblings_list, 0);
-define_siblings_show_func(die_cpus, die_cpumask); -static DEVICE_ATTR_RO(die_cpus); -static DEVICE_ATTR_RO(die_cpus_list); +define_siblings_read_func(die_cpus, die_cpumask); +static BIN_ATTR_RO(die_cpus, 0); +static BIN_ATTR_RO(die_cpus_list, 0);
-define_siblings_show_func(package_cpus, core_cpumask); -static DEVICE_ATTR_RO(package_cpus); -static DEVICE_ATTR_RO(package_cpus_list); +define_siblings_read_func(package_cpus, core_cpumask); +static BIN_ATTR_RO(package_cpus, 0); +static BIN_ATTR_RO(package_cpus_list, 0);
#ifdef CONFIG_SCHED_BOOK define_id_show_func(book_id); static DEVICE_ATTR_RO(book_id); -define_siblings_show_func(book_siblings, book_cpumask); -static DEVICE_ATTR_RO(book_siblings); -static DEVICE_ATTR_RO(book_siblings_list); +define_siblings_read_func(book_siblings, book_cpumask); +static BIN_ATTR_RO(book_siblings, 0); +static BIN_ATTR_RO(book_siblings_list, 0); #endif
#ifdef CONFIG_SCHED_DRAWER define_id_show_func(drawer_id); static DEVICE_ATTR_RO(drawer_id); -define_siblings_show_func(drawer_siblings, drawer_cpumask); -static DEVICE_ATTR_RO(drawer_siblings); -static DEVICE_ATTR_RO(drawer_siblings_list); +define_siblings_read_func(drawer_siblings, drawer_cpumask); +static BIN_ATTR_RO(drawer_siblings, 0); +static BIN_ATTR_RO(drawer_siblings_list, 0); +#endif + +static struct bin_attribute *bin_attrs[] = { + &bin_attr_core_cpus, + &bin_attr_core_cpus_list, + &bin_attr_thread_siblings, + &bin_attr_thread_siblings_list, + &bin_attr_core_siblings, + &bin_attr_core_siblings_list, + &bin_attr_die_cpus, + &bin_attr_die_cpus_list, + &bin_attr_package_cpus, + &bin_attr_package_cpus_list, +#ifdef CONFIG_SCHED_BOOK + &bin_attr_book_siblings, + &bin_attr_book_siblings_list, #endif +#ifdef CONFIG_SCHED_DRAWER + &bin_attr_drawer_siblings, + &bin_attr_drawer_siblings_list, +#endif + NULL, +};
static struct attribute *default_attrs[] = { &dev_attr_physical_package_id.attr, &dev_attr_die_id.attr, &dev_attr_core_id.attr, - &dev_attr_thread_siblings.attr, - &dev_attr_thread_siblings_list.attr, - &dev_attr_core_cpus.attr, - &dev_attr_core_cpus_list.attr, - &dev_attr_core_siblings.attr, - &dev_attr_core_siblings_list.attr, - &dev_attr_die_cpus.attr, - &dev_attr_die_cpus_list.attr, - &dev_attr_package_cpus.attr, - &dev_attr_package_cpus_list.attr, #ifdef CONFIG_SCHED_BOOK &dev_attr_book_id.attr, - &dev_attr_book_siblings.attr, - &dev_attr_book_siblings_list.attr, #endif #ifdef CONFIG_SCHED_DRAWER &dev_attr_drawer_id.attr, - &dev_attr_drawer_siblings.attr, - &dev_attr_drawer_siblings_list.attr, #endif NULL };
static const struct attribute_group topology_attr_group = { .attrs = default_attrs, + .bin_attrs = bin_attrs, .name = "topology" };
-----Original Message----- From: tiantao (H) Sent: Thursday, May 27, 2021 2:57 PM To: Song Bao Hua (Barry Song) song.bao.hua@hisilicon.com; Jonathan Cameron jonathan.cameron@huawei.com Cc: linuxarm@openeuler.org; tiantao (H) tiantao6@hisilicon.com Subject: [PATCH v7] topology: use bin_attribute to avoid buff overflow
Reading sys/devices/system/cpu/cpuX/topology/ returns cpu topology. However, the size of this file is limited to PAGE_SIZE because of the limitation for sysfs attribute. so we use bin_attribute instead of attribute to avoid NR_CPUS too big to cause buff overflow.
This patch is based on the following discussion. https://www.spinics.net/lists/linux-doc/msg95921.html
Signed-off-by: Tian Tao tiantao6@hisilicon.com
v2: rewrite the function name##_read v3: remove the temp buffer. v4: rewrite the function name##_read v5: rewrite the patch v6: update the commit message v7: remove BUFF_SIZE, dynamic calculation of size
drivers/base/topology.c | 144
Thanks very much. We are almost there.
+++++++++++++++++++++++++++++++----------------- 1 file changed, 94 insertions(+), 50 deletions(-)
diff --git a/drivers/base/topology.c b/drivers/base/topology.c index 4d254fc..883fa39 100644 --- a/drivers/base/topology.c +++ b/drivers/base/topology.c @@ -21,24 +21,59 @@ static ssize_t name##_show(struct device *dev, \ return sysfs_emit(buf, "%d\n", topology_##name(dev->id)); \ }
-#define define_siblings_show_map(name, mask) \ -static ssize_t name##_show(struct device *dev, \
struct device_attribute *attr, char *buf) \-{ \
- return cpumap_print_to_pagebuf(false, buf, topology_##mask(dev->id));\
-} +static int topology_snprintf(char *buf, ssize_t size, const char *fmt, ...) +{
- va_list args;
- int i;
As discussed, better to make a common wrapper so that other modules calling cpumap_print_to_pagebuf, such as drivers/base/node.c, can reuse the wrapper.
-#define define_siblings_show_list(name, mask) \ -static ssize_t name##_list_show(struct device *dev, \
struct device_attribute *attr, \char *buf) \-{ \
- return cpumap_print_to_pagebuf(true, buf, topology_##mask(dev->id));\
- va_start(args, fmt);
- if (size <= 0)
i = vsnprintf(NULL, 0, fmt, args);- else
i = vscnprintf(buf, size, fmt, args);- va_end(args);
- return i;
}
-#define define_siblings_show_func(name, mask) \
- define_siblings_show_map(name, mask); \
- define_siblings_show_list(name, mask)
+#define define_siblings_read_func(name, mask) \ +static ssize_t name##_read(struct file *file, struct kobject *kobj, \
struct bin_attribute *attr, char *buf, \loff_t off, size_t count) \+{ \
- struct device *dev = kobj_to_dev(kobj); \
- int len; \
- void *data; \
- len = topology_snprintf(NULL, 0, "%*pb\n", nr_cpu_ids, \
cpumask_bits(topology_##mask(dev->id))); \- data = kvmalloc(len+1, GFP_KERNEL); \
- if (!data) \
return -ENOMEM; \- len = topology_snprintf(data, len+1, "%*pb\n", nr_cpu_ids, \
cpumask_bits(topology_##mask(dev->id))); \- len = memory_read_from_buffer(buf, count, &off, data, len); \
- kvfree(data); \
- return len; \
+} \
\+static ssize_t name##_list_read(struct file *file, struct kobject *kobj, \
struct bin_attribute *attr, char *buf, \loff_t off, size_t count) \+{ \
- struct device *dev = kobj_to_dev(kobj); \
- int len; \
- void *data; \
- len = topology_snprintf(NULL, 0, "%*pbl\n", nr_cpu_ids, \
cpumask_bits(topology_##mask(dev->id))); \- data = kvmalloc(len+1, GFP_KERNEL); \
- if (!data) \
return -ENOMEM; \- len = topology_snprintf(data, len+1, "%*pbl\n", nr_cpu_ids, \
cpumask_bits(topology_##mask(dev->id))); \- len = memory_read_from_buffer(buf, count, &off, data, len); \
- kvfree(data); \
- return len; \
+}
As discussed, here we have some bad smells in code. The only difference between name##_list_read and name##_read is pb and pbl.
Thanks Barry
On Thu, 27 May 2021 10:56:40 +0800 Tian Tao tiantao6@hisilicon.com wrote:
Reading sys/devices/system/cpu/cpuX/topology/ returns cpu topology. However, the size of this file is limited to PAGE_SIZE because of the limitation for sysfs attribute. so we use bin_attribute instead of attribute to avoid NR_CPUS too big to cause buff overflow.
This patch is based on the following discussion. https://www.spinics.net/lists/linux-doc/msg95921.html
Signed-off-by: Tian Tao tiantao6@hisilicon.com
Basic approach now looks good. A few small suggestions inline.
v2: rewrite the function name##_read v3: remove the temp buffer. v4: rewrite the function name##_read v5: rewrite the patch v6: update the commit message v7: remove BUFF_SIZE, dynamic calculation of size
drivers/base/topology.c | 144 +++++++++++++++++++++++++++++++----------------- 1 file changed, 94 insertions(+), 50 deletions(-)
diff --git a/drivers/base/topology.c b/drivers/base/topology.c index 4d254fc..883fa39 100644 --- a/drivers/base/topology.c +++ b/drivers/base/topology.c @@ -21,24 +21,59 @@ static ssize_t name##_show(struct device *dev, \ return sysfs_emit(buf, "%d\n", topology_##name(dev->id)); \ }
-#define define_siblings_show_map(name, mask) \ -static ssize_t name##_show(struct device *dev, \
struct device_attribute *attr, char *buf) \-{ \
- return cpumap_print_to_pagebuf(false, buf, topology_##mask(dev->id));\
-} +static int topology_snprintf(char *buf, ssize_t size, const char *fmt, ...) +{
- va_list args;
- int i;
-#define define_siblings_show_list(name, mask) \ -static ssize_t name##_list_show(struct device *dev, \
struct device_attribute *attr, \char *buf) \-{ \
- return cpumap_print_to_pagebuf(true, buf, topology_##mask(dev->id));\
- va_start(args, fmt);
- if (size <= 0)
How would size be less than 0?
i = vsnprintf(NULL, 0, fmt, args);- else
i = vscnprintf(buf, size, fmt, args);
What benefit from this wrapper?
Just call, snprintf() directly below for the first case and if you want to be paranoid scnprintf() for the second.
- va_end(args);
- return i;
}
-#define define_siblings_show_func(name, mask) \
- define_siblings_show_map(name, mask); \
- define_siblings_show_list(name, mask)
+#define define_siblings_read_func(name, mask) \ +static ssize_t name##_read(struct file *file, struct kobject *kobj, \
struct bin_attribute *attr, char *buf, \loff_t off, size_t count) \+{ \
- struct device *dev = kobj_to_dev(kobj); \
- int len; \
- void *data; \
- len = topology_snprintf(NULL, 0, "%*pb\n", nr_cpu_ids, \
cpumask_bits(topology_##mask(dev->id))); \- data = kvmalloc(len+1, GFP_KERNEL); \
- if (!data) \
return -ENOMEM; \- len = topology_snprintf(data, len+1, "%*pb\n", nr_cpu_ids, \
cpumask_bits(topology_##mask(dev->id))); \- len = memory_read_from_buffer(buf, count, &off, data, len); \
- kvfree(data); \
- return len; \
+} \
\+static ssize_t name##_list_read(struct file *file, struct kobject *kobj, \
struct bin_attribute *attr, char *buf, \loff_t off, size_t count) \+{ \
- struct device *dev = kobj_to_dev(kobj); \
- int len; \
- void *data; \
blank line here. Even through it is a macro, it should obey kernel style Same issue needs cleaning up elsewhere.
- len = topology_snprintf(NULL, 0, "%*pbl\n", nr_cpu_ids, \
cpumask_bits(topology_##mask(dev->id))); \- data = kvmalloc(len+1, GFP_KERNEL); \
Spaces around +
- if (!data) \
return -ENOMEM; \- len = topology_snprintf(data, len+1, "%*pbl\n", nr_cpu_ids, \
cpumask_bits(topology_##mask(dev->id))); \
Introduce a second local variable, as the meaning of len changing during the function is not good for readability.
- len = memory_read_from_buffer(buf, count, &off, data, len); \
- kvfree(data); \
blank line here
- return len; \
+}
define_id_show_func(physical_package_id); static DEVICE_ATTR_RO(physical_package_id); @@ -49,71 +84,80 @@ static DEVICE_ATTR_RO(die_id); define_id_show_func(core_id); static DEVICE_ATTR_RO(core_id);
-define_siblings_show_func(thread_siblings, sibling_cpumask); -static DEVICE_ATTR_RO(thread_siblings); -static DEVICE_ATTR_RO(thread_siblings_list); +define_siblings_read_func(thread_siblings, sibling_cpumask); +static BIN_ATTR_RO(thread_siblings, 0); +static BIN_ATTR_RO(thread_siblings_list, 0);
-define_siblings_show_func(core_cpus, sibling_cpumask); -static DEVICE_ATTR_RO(core_cpus); -static DEVICE_ATTR_RO(core_cpus_list); +define_siblings_read_func(core_cpus, sibling_cpumask); +static BIN_ATTR_RO(core_cpus, 0); +static BIN_ATTR_RO(core_cpus_list, 0);
-define_siblings_show_func(core_siblings, core_cpumask); -static DEVICE_ATTR_RO(core_siblings); -static DEVICE_ATTR_RO(core_siblings_list); +define_siblings_read_func(core_siblings, core_cpumask); +static BIN_ATTR_RO(core_siblings, 0); +static BIN_ATTR_RO(core_siblings_list, 0);
-define_siblings_show_func(die_cpus, die_cpumask); -static DEVICE_ATTR_RO(die_cpus); -static DEVICE_ATTR_RO(die_cpus_list); +define_siblings_read_func(die_cpus, die_cpumask); +static BIN_ATTR_RO(die_cpus, 0); +static BIN_ATTR_RO(die_cpus_list, 0);
-define_siblings_show_func(package_cpus, core_cpumask); -static DEVICE_ATTR_RO(package_cpus); -static DEVICE_ATTR_RO(package_cpus_list); +define_siblings_read_func(package_cpus, core_cpumask); +static BIN_ATTR_RO(package_cpus, 0); +static BIN_ATTR_RO(package_cpus_list, 0);
#ifdef CONFIG_SCHED_BOOK define_id_show_func(book_id); static DEVICE_ATTR_RO(book_id); -define_siblings_show_func(book_siblings, book_cpumask); -static DEVICE_ATTR_RO(book_siblings); -static DEVICE_ATTR_RO(book_siblings_list); +define_siblings_read_func(book_siblings, book_cpumask); +static BIN_ATTR_RO(book_siblings, 0); +static BIN_ATTR_RO(book_siblings_list, 0); #endif
#ifdef CONFIG_SCHED_DRAWER define_id_show_func(drawer_id); static DEVICE_ATTR_RO(drawer_id); -define_siblings_show_func(drawer_siblings, drawer_cpumask); -static DEVICE_ATTR_RO(drawer_siblings); -static DEVICE_ATTR_RO(drawer_siblings_list); +define_siblings_read_func(drawer_siblings, drawer_cpumask); +static BIN_ATTR_RO(drawer_siblings, 0); +static BIN_ATTR_RO(drawer_siblings_list, 0); +#endif
+static struct bin_attribute *bin_attrs[] = {
- &bin_attr_core_cpus,
- &bin_attr_core_cpus_list,
- &bin_attr_thread_siblings,
- &bin_attr_thread_siblings_list,
- &bin_attr_core_siblings,
- &bin_attr_core_siblings_list,
- &bin_attr_die_cpus,
- &bin_attr_die_cpus_list,
- &bin_attr_package_cpus,
- &bin_attr_package_cpus_list,
+#ifdef CONFIG_SCHED_BOOK
- &bin_attr_book_siblings,
- &bin_attr_book_siblings_list,
#endif +#ifdef CONFIG_SCHED_DRAWER
- &bin_attr_drawer_siblings,
- &bin_attr_drawer_siblings_list,
+#endif
- NULL,
+};
static struct attribute *default_attrs[] = { &dev_attr_physical_package_id.attr, &dev_attr_die_id.attr, &dev_attr_core_id.attr,
- &dev_attr_thread_siblings.attr,
- &dev_attr_thread_siblings_list.attr,
- &dev_attr_core_cpus.attr,
- &dev_attr_core_cpus_list.attr,
- &dev_attr_core_siblings.attr,
- &dev_attr_core_siblings_list.attr,
- &dev_attr_die_cpus.attr,
- &dev_attr_die_cpus_list.attr,
- &dev_attr_package_cpus.attr,
- &dev_attr_package_cpus_list.attr,
#ifdef CONFIG_SCHED_BOOK &dev_attr_book_id.attr,
- &dev_attr_book_siblings.attr,
- &dev_attr_book_siblings_list.attr,
#endif #ifdef CONFIG_SCHED_DRAWER &dev_attr_drawer_id.attr,
- &dev_attr_drawer_siblings.attr,
- &dev_attr_drawer_siblings_list.attr,
#endif NULL };
static const struct attribute_group topology_attr_group = { .attrs = default_attrs,
- .bin_attrs = bin_attrs, .name = "topology"
};
-
Jonathan Cameron -
Song Bao Hua (Barry Song) -
Tian Tao