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

-----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