[Linuxarm] Re: [PATCH v7] topology: use bin_attribute to avoid buff overflow

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"

};