[PATCH v4 0/4] use bin_attribute to avoid cpumap buff overflow

[PATCH v4 0/4] use bin_attribute to avoid cpumap buff overflow

[Barry Song]

patch #1 adds a new function cpumap_print_to_buf and patch #2 uses
this function in drivers/base/topology.c, and patch #3 uses this new
function in drivers/base/node.c.
patch #4 adds test cases for the new API.

v4:
add test cases for bitmap_print_to_buf API;
add Reviewed-by of Jonathan Cameron for patches 1-3, thanks!

v3:
fixed the strlen issue and patch #1,#2,#3 minor formatting issues, thanks
to Andy Shevchenko and Jonathan Cameron.

v2:
split the original patch #1 into two patches and use kasprintf() in
patch #1 to simplify the code. do some minor formatting adjustments.

Barry Song (1):
  lib: test_bitmap: add bitmap_print_to_buf test cases

Tian Tao (3):
  lib: bitmap: introduce bitmap_print_to_buf
  topology: use bin_attribute to avoid buff overflow
  drivers/base/node.c: use bin_attribute to avoid buff overflow

 drivers/base/node.c     |  52 +++++++++-----
 drivers/base/topology.c | 115 +++++++++++++++++--------------
 include/linux/bitmap.h  |   2 +
 include/linux/cpumask.h |  21 ++++++
 lib/bitmap.c            |  37 +++++++++-
 lib/test_bitmap.c       | 149 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 304 insertions(+), 72 deletions(-)

-- 
2.25.1

[PATCH v4 1/4] lib: bitmap: introduce bitmap_print_to_buf

[Barry Song]

From: Tian Tao <tiantao6@hisilicon.com>

New API bitmap_print_to_buf() with bin_attribute to avoid maskp
exceeding PAGE_SIZE. bitmap_print_to_pagebuf() is a special case
of bitmap_print_to_buf(), so in bitmap_print_to_pagebuf() call
bitmap_print_to_buf().

Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Stefano Brivio <sbrivio@redhat.com>
Cc: Alexander Gordeev <agordeev@linux.ibm.com>
Cc: "Ma, Jianpeng" <jianpeng.ma@intel.com>
Cc: Yury Norov <yury.norov@gmail.com>
Cc: Valentin Schneider <valentin.schneider@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Daniel Bristot de Oliveira <bristot@redhat.com>
---
 include/linux/bitmap.h  |  2 ++
 include/linux/cpumask.h | 21 +++++++++++++++++++++
 lib/bitmap.c            | 37 +++++++++++++++++++++++++++++++++++--
 3 files changed, 58 insertions(+), 2 deletions(-)

diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index a36cfcec4e77..0de6effa2797 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -226,6 +226,8 @@ void bitmap_copy_le(unsigned long *dst, const unsigned long *src, unsigned int n
 unsigned int bitmap_ord_to_pos(const unsigned long *bitmap, unsigned int ord, unsigned int nbits);
 int bitmap_print_to_pagebuf(bool list, char *buf,
 				   const unsigned long *maskp, int nmaskbits);
+int bitmap_print_to_buf(bool list, char *buf, const unsigned long *maskp,
+			int nmaskbits, loff_t off, size_t count);
 
 #define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) & (BITS_PER_LONG - 1)))
 #define BITMAP_LAST_WORD_MASK(nbits) (~0UL >> (-(nbits) & (BITS_PER_LONG - 1)))
diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index bfc4690de4f4..1bef69e4b950 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -983,6 +983,27 @@ cpumap_print_to_pagebuf(bool list, char *buf, const struct cpumask *mask)
 				      nr_cpu_ids);
 }
 
+/**
+ * cpumap_print_to_buf  - copies the cpumask into the buffer either
+ *      as comma-separated list of cpus or hex values of cpumask
+ * @list: indicates whether the cpumap must be list
+ * @mask: the cpumask to copy
+ * @buf: the buffer to copy into
+ * @off: in the string from which we are copying, We copy to @buf + off
+ * @count: the maximum number of bytes to print
+ *
+ * The role of cpumap_print_to_buf() and cpumap_print_to_pagebuf() is
+ * the same, the difference is that buf of bitmap_print_to_buf()
+ * can be more than one pagesize.
+ */
+static inline ssize_t
+cpumap_print_to_buf(bool list, char *buf, const struct cpumask *mask,
+		    loff_t off, size_t count)
+{
+	return bitmap_print_to_buf(list, buf, cpumask_bits(mask),
+				   nr_cpu_ids, off, count);
+}
+
 #if NR_CPUS <= BITS_PER_LONG
 #define CPU_MASK_ALL							\
 (cpumask_t) { {								\
diff --git a/lib/bitmap.c b/lib/bitmap.c
index 74ceb02f45e3..ed7ef6e2edca 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -461,6 +461,40 @@ int bitmap_parse_user(const char __user *ubuf,
 }
 EXPORT_SYMBOL(bitmap_parse_user);
 
+/**
+ * bitmap_print_to_buf - convert bitmap to list or hex format ASCII string
+ * @list: indicates whether the bitmap must be list
+ * @buf: the kernel space buffer to read to
+ * @maskp: pointer to bitmap to convert
+ * @nmaskbits: size of bitmap, in bits
+ * @off: offset in data buffer below
+ * @count: the maximum number of bytes to print
+ *
+ * The role of bitmap_print_to_buf() and bitmap_print_to_pagebuf() is
+ * the same, the difference is that buf of bitmap_print_to_buf()
+ * can be more than one pagesize.
+ */
+int bitmap_print_to_buf(bool list, char *buf, const unsigned long *maskp,
+			int nmaskbits, loff_t off, size_t count)
+{
+	const char *fmt = list ? "%*pbl\n" : "%*pb\n";
+	ssize_t size;
+	void *data;
+
+	if (off == LLONG_MAX && count == PAGE_SIZE - offset_in_page(buf))
+		return scnprintf(buf, count, fmt, nmaskbits, maskp);
+
+	data = kasprintf(GFP_KERNEL, fmt, nmaskbits, maskp);
+	if (!data)
+		return -ENOMEM;
+
+	size = memory_read_from_buffer(buf, count, &off, data, strlen(data) + 1);
+	kfree(data);
+
+	return size;
+}
+EXPORT_SYMBOL(bitmap_print_to_buf);
+
 /**
  * bitmap_print_to_pagebuf - convert bitmap to list or hex format ASCII string
  * @list: indicates whether the bitmap must be list
@@ -482,8 +516,7 @@ int bitmap_print_to_pagebuf(bool list, char *buf, const unsigned long *maskp,
 {
 	ptrdiff_t len = PAGE_SIZE - offset_in_page(buf);
 
-	return list ? scnprintf(buf, len, "%*pbl\n", nmaskbits, maskp) :
-		      scnprintf(buf, len, "%*pb\n", nmaskbits, maskp);
+	return bitmap_print_to_buf(list, buf, maskp, nmaskbits, LLONG_MAX, len);
 }
 EXPORT_SYMBOL(bitmap_print_to_pagebuf);
 
-- 
2.25.1

[PATCH v4 2/4] topology: use bin_attribute to avoid buff overflow

[Barry Song]

From: Tian Tao <tiantao6@hisilicon.com>

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.

Link: https://lore.kernel.org/lkml/20210319041618.14316-2-song.bao.hua@hisilicon.com/
Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
---
 drivers/base/topology.c | 115 ++++++++++++++++++++++------------------
 1 file changed, 63 insertions(+), 52 deletions(-)

diff --git a/drivers/base/topology.c b/drivers/base/topology.c
index 4d254fcc93d1..dd3980124e33 100644
--- a/drivers/base/topology.c
+++ b/drivers/base/topology.c
@@ -21,25 +21,27 @@ 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));\
+#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);                                 \
+										\
+	return cpumap_print_to_buf(false, buf, topology_##mask(dev->id),	\
+				   off, count);                                 \
+}										\
+										\
+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);					\
+										\
+	return cpumap_print_to_buf(true, buf, topology_##mask(dev->id),		\
+				   off, count);					\
 }
 
-#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));\
-}
-
-#define define_siblings_show_func(name, mask)	\
-	define_siblings_show_map(name, mask);	\
-	define_siblings_show_list(name, mask)
-
 define_id_show_func(physical_package_id);
 static DEVICE_ATTR_RO(physical_package_id);
 
@@ -49,71 +51,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"
 };
 
-- 
2.25.1

[PATCH v4 3/4] drivers/base/node.c: use bin_attribute to avoid buff overflow

[Barry Song]

From: Tian Tao <tiantao6@hisilicon.com>

Reading sys/devices/system/cpu/cpuX/nodeX/ returns cpumap and cpulist.
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.

Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
---
 drivers/base/node.c | 52 +++++++++++++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 18 deletions(-)

diff --git a/drivers/base/node.c b/drivers/base/node.c
index 2c36f61d30bc..7107e4784df1 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -27,42 +27,45 @@ static struct bus_type node_subsys = {
 };
 
 
-static ssize_t node_read_cpumap(struct device *dev, bool list, char *buf)
+static ssize_t node_read_cpumap(struct device *dev, bool list, char *buf,
+				loff_t off, size_t count)
 {
 	ssize_t n;
 	cpumask_var_t mask;
 	struct node *node_dev = to_node(dev);
 
-	/* 2008/04/07: buf currently PAGE_SIZE, need 9 chars per 32 bits. */
-	BUILD_BUG_ON((NR_CPUS/32 * 9) > (PAGE_SIZE-1));
-
 	if (!alloc_cpumask_var(&mask, GFP_KERNEL))
 		return 0;
 
 	cpumask_and(mask, cpumask_of_node(node_dev->dev.id), cpu_online_mask);
-	n = cpumap_print_to_pagebuf(list, buf, mask);
+	n = cpumap_print_to_buf(list, buf, mask, off, count);
 	free_cpumask_var(mask);
 
 	return n;
 }
 
-static inline ssize_t cpumap_show(struct device *dev,
-				  struct device_attribute *attr,
-				  char *buf)
+static inline ssize_t cpumap_read(struct file *file, struct kobject *kobj,
+				  struct bin_attribute *attr, char *buf,
+				  loff_t off, size_t count)
 {
-	return node_read_cpumap(dev, false, buf);
+	struct device *dev = kobj_to_dev(kobj);
+
+	return node_read_cpumap(dev, false, buf, off, count);
 }
 
-static DEVICE_ATTR_RO(cpumap);
 
-static inline ssize_t cpulist_show(struct device *dev,
-				   struct device_attribute *attr,
-				   char *buf)
+static BIN_ATTR_RO(cpumap, 0);
+
+static inline ssize_t cpulist_read(struct file *file, struct kobject *kobj,
+				   struct bin_attribute *attr, char *buf,
+				   loff_t off, size_t count)
 {
-	return node_read_cpumap(dev, true, buf);
+	struct device *dev = kobj_to_dev(kobj);
+
+	return node_read_cpumap(dev, true, buf, off, count);
 }
 
-static DEVICE_ATTR_RO(cpulist);
+static BIN_ATTR_RO(cpulist, 0);
 
 /**
  * struct node_access_nodes - Access class device to hold user visible
@@ -555,15 +558,28 @@ static ssize_t node_read_distance(struct device *dev,
 static DEVICE_ATTR(distance, 0444, node_read_distance, NULL);
 
 static struct attribute *node_dev_attrs[] = {
-	&dev_attr_cpumap.attr,
-	&dev_attr_cpulist.attr,
 	&dev_attr_meminfo.attr,
 	&dev_attr_numastat.attr,
 	&dev_attr_distance.attr,
 	&dev_attr_vmstat.attr,
 	NULL
 };
-ATTRIBUTE_GROUPS(node_dev);
+
+static struct bin_attribute *node_dev_bin_attrs[] = {
+	&bin_attr_cpumap,
+	&bin_attr_cpulist,
+	NULL
+};
+
+static const struct attribute_group node_dev_group = {
+	.attrs = node_dev_attrs,
+	.bin_attrs = node_dev_bin_attrs
+};
+
+static const struct attribute_group *node_dev_groups[] = {
+	&node_dev_group,
+	NULL
+};
 
 #ifdef CONFIG_HUGETLBFS
 /*
-- 
2.25.1

[PATCH v4 4/4] lib: test_bitmap: add bitmap_print_to_buf test cases

[Barry Song]

The added cases cover both bitmap_buf size more than 4KB and bitmap_buf
less than 4KB.
And it also covers the case offset is non-zero.

Signed-off-by: Barry Song <song.bao.hua@hisilicon.com>
---
 lib/test_bitmap.c | 149 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 149 insertions(+)

diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
index 9cd575583180..73259cd3814f 100644
--- a/lib/test_bitmap.c
+++ b/lib/test_bitmap.c
@@ -19,6 +19,7 @@
 KSTM_MODULE_GLOBALS();
 
 static char pbl_buffer[PAGE_SIZE] __initdata;
+static char print_buf[PAGE_SIZE * 2] __initdata;
 
 static const unsigned long exp1[] __initconst = {
 	BITMAP_FROM_U64(1),
@@ -156,6 +157,19 @@ static bool __init __check_eq_clump8(const char *srcfile, unsigned int line,
 	return true;
 }
 
+static bool __init
+__check_eq_str(const char *srcfile, unsigned int line,
+		const char *exp_str, const char *str,
+		unsigned int len)
+{
+	if (strncmp(exp_str, str, len)) {
+		pr_err("[%s:%u] expected %s, got %s\n",
+			srcfile, line, exp_str, str);
+		return false;
+	}
+	return true;
+}
+
 #define __expect_eq(suffix, ...)					\
 	({								\
 		int result = 0;						\
@@ -173,6 +187,7 @@ static bool __init __check_eq_clump8(const char *srcfile, unsigned int line,
 #define expect_eq_pbl(...)		__expect_eq(pbl, ##__VA_ARGS__)
 #define expect_eq_u32_array(...)	__expect_eq(u32_array, ##__VA_ARGS__)
 #define expect_eq_clump8(...)		__expect_eq(clump8, ##__VA_ARGS__)
+#define expect_eq_str(...)		__expect_eq(str, ##__VA_ARGS__)
 
 static void __init test_zero_clear(void)
 {
@@ -653,6 +668,139 @@ static void __init test_bitmap_cut(void)
 	}
 }
 
+struct test_bitmap_print {
+	const unsigned long *bitmap;
+	unsigned long nbits;
+	const char *mask;
+	const char *list;
+};
+
+static const unsigned long small_bitmap[] __initconst = {
+	BITMAP_FROM_U64(0x3333333311111111ULL),
+};
+
+static const char small_mask[] __initconst = "33333333,11111111\n";
+static const char small_list[] __initconst = "0,4,8,12,16,20,24,28,32-33,36-37,40-41,44-45,48-49,52-53,56-57,60-61\n";
+
+static const unsigned long large_bitmap[] __initconst = {
+	BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
+	BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
+	BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
+	BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
+	BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
+	BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
+	BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
+	BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
+	BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
+	BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
+	BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
+	BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
+	BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
+	BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
+	BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
+	BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
+	BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
+	BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
+	BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
+	BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
+};
+
+static const char large_mask[] __initconst = "33333333,11111111,33333333,11111111,"
+					"33333333,11111111,33333333,11111111,"
+					"33333333,11111111,33333333,11111111,"
+					"33333333,11111111,33333333,11111111,"
+					"33333333,11111111,33333333,11111111,"
+					"33333333,11111111,33333333,11111111,"
+					"33333333,11111111,33333333,11111111,"
+					"33333333,11111111,33333333,11111111,"
+					"33333333,11111111,33333333,11111111,"
+					"33333333,11111111,33333333,11111111,"
+					"33333333,11111111,33333333,11111111,"
+					"33333333,11111111,33333333,11111111,"
+					"33333333,11111111,33333333,11111111,"
+					"33333333,11111111,33333333,11111111,"
+					"33333333,11111111,33333333,11111111,"
+					"33333333,11111111,33333333,11111111,"
+					"33333333,11111111,33333333,11111111,"
+					"33333333,11111111,33333333,11111111,"
+					"33333333,11111111,33333333,11111111,"
+					"33333333,11111111,33333333,11111111\n";
+
+static const char large_list[] __initconst = /* more than 4KB */
+	"0,4,8,12,16,20,24,28,32-33,36-37,40-41,44-45,48-49,52-53,56-57,60-61,64,68,72,76,80,84,88,92,96-97,100-101,104-1"
+	"05,108-109,112-113,116-117,120-121,124-125,128,132,136,140,144,148,152,156,160-161,164-165,168-169,172-173,176-1"
+	"77,180-181,184-185,188-189,192,196,200,204,208,212,216,220,224-225,228-229,232-233,236-237,240-241,244-245,248-2"
+	"49,252-253,256,260,264,268,272,276,280,284,288-289,292-293,296-297,300-301,304-305,308-309,312-313,316-317,320,3"
+	"24,328,332,336,340,344,348,352-353,356-357,360-361,364-365,368-369,372-373,376-377,380-381,384,388,392,396,400,4"
+	"04,408,412,416-417,420-421,424-425,428-429,432-433,436-437,440-441,444-445,448,452,456,460,464,468,472,476,480-4"
+	"81,484-485,488-489,492-493,496-497,500-501,504-505,508-509,512,516,520,524,528,532,536,540,544-545,548-549,552-5"
+	"53,556-557,560-561,564-565,568-569,572-573,576,580,584,588,592,596,600,604,608-609,612-613,616-617,620-621,624-6"
+	"25,628-629,632-633,636-637,640,644,648,652,656,660,664,668,672-673,676-677,680-681,684-685,688-689,692-693,696-6"
+	"97,700-701,704,708,712,716,720,724,728,732,736-737,740-741,744-745,748-749,752-753,756-757,760-761,764-765,768,7"
+	"72,776,780,784,788,792,796,800-801,804-805,808-809,812-813,816-817,820-821,824-825,828-829,832,836,840,844,848,8"
+	"52,856,860,864-865,868-869,872-873,876-877,880-881,884-885,888-889,892-893,896,900,904,908,912,916,920,924,928-9"
+	"29,932-933,936-937,940-941,944-945,948-949,952-953,956-957,960,964,968,972,976,980,984,988,992-993,996-997,1000-"
+	"1001,1004-1005,1008-1009,1012-1013,1016-1017,1020-1021,1024,1028,1032,1036,1040,1044,1048,1052,1056-1057,1060-10"
+	"61,1064-1065,1068-1069,1072-1073,1076-1077,1080-1081,1084-1085,1088,1092,1096,1100,1104,1108,1112,1116,1120-1121"
+	",1124-1125,1128-1129,1132-1133,1136-1137,1140-1141,1144-1145,1148-1149,1152,1156,1160,1164,1168,1172,1176,1180,1"
+	"184-1185,1188-1189,1192-1193,1196-1197,1200-1201,1204-1205,1208-1209,1212-1213,1216,1220,1224,1228,1232,1236,124"
+	"0,1244,1248-1249,1252-1253,1256-1257,1260-1261,1264-1265,1268-1269,1272-1273,1276-1277,1280,1284,1288,1292,1296,"
+	"1300,1304,1308,1312-1313,1316-1317,1320-1321,1324-1325,1328-1329,1332-1333,1336-1337,1340-1341,1344,1348,1352,13"
+	"56,1360,1364,1368,1372,1376-1377,1380-1381,1384-1385,1388-1389,1392-1393,1396-1397,1400-1401,1404-1405,1408,1412"
+	",1416,1420,1424,1428,1432,1436,1440-1441,1444-1445,1448-1449,1452-1453,1456-1457,1460-1461,1464-1465,1468-1469,1"
+	"472,1476,1480,1484,1488,1492,1496,1500,1504-1505,1508-1509,1512-1513,1516-1517,1520-1521,1524-1525,1528-1529,153"
+	"2-1533,1536,1540,1544,1548,1552,1556,1560,1564,1568-1569,1572-1573,1576-1577,1580-1581,1584-1585,1588-1589,1592-"
+	"1593,1596-1597,1600,1604,1608,1612,1616,1620,1624,1628,1632-1633,1636-1637,1640-1641,1644-1645,1648-1649,1652-16"
+	"53,1656-1657,1660-1661,1664,1668,1672,1676,1680,1684,1688,1692,1696-1697,1700-1701,1704-1705,1708-1709,1712-1713"
+	",1716-1717,1720-1721,1724-1725,1728,1732,1736,1740,1744,1748,1752,1756,1760-1761,1764-1765,1768-1769,1772-1773,1"
+	"776-1777,1780-1781,1784-1785,1788-1789,1792,1796,1800,1804,1808,1812,1816,1820,1824-1825,1828-1829,1832-1833,183"
+	"6-1837,1840-1841,1844-1845,1848-1849,1852-1853,1856,1860,1864,1868,1872,1876,1880,1884,1888-1889,1892-1893,1896-"
+	"1897,1900-1901,1904-1905,1908-1909,1912-1913,1916-1917,1920,1924,1928,1932,1936,1940,1944,1948,1952-1953,1956-19"
+	"57,1960-1961,1964-1965,1968-1969,1972-1973,1976-1977,1980-1981,1984,1988,1992,1996,2000,2004,2008,2012,2016-2017"
+	",2020-2021,2024-2025,2028-2029,2032-2033,2036-2037,2040-2041,2044-2045,2048,2052,2056,2060,2064,2068,2072,2076,2"
+	"080-2081,2084-2085,2088-2089,2092-2093,2096-2097,2100-2101,2104-2105,2108-2109,2112,2116,2120,2124,2128,2132,213"
+	"6,2140,2144-2145,2148-2149,2152-2153,2156-2157,2160-2161,2164-2165,2168-2169,2172-2173,2176,2180,2184,2188,2192,"
+	"2196,2200,2204,2208-2209,2212-2213,2216-2217,2220-2221,2224-2225,2228-2229,2232-2233,2236-2237,2240,2244,2248,22"
+	"52,2256,2260,2264,2268,2272-2273,2276-2277,2280-2281,2284-2285,2288-2289,2292-2293,2296-2297,2300-2301,2304,2308"
+	",2312,2316,2320,2324,2328,2332,2336-2337,2340-2341,2344-2345,2348-2349,2352-2353,2356-2357,2360-2361,2364-2365,2"
+	"368,2372,2376,2380,2384,2388,2392,2396,2400-2401,2404-2405,2408-2409,2412-2413,2416-2417,2420-2421,2424-2425,242"
+	"8-2429,2432,2436,2440,2444,2448,2452,2456,2460,2464-2465,2468-2469,2472-2473,2476-2477,2480-2481,2484-2485,2488-"
+	"2489,2492-2493,2496,2500,2504,2508,2512,2516,2520,2524,2528-2529,2532-2533,2536-2537,2540-2541,2544-2545,2548-25"
+	"49,2552-2553,2556-2557\n";
+
+static const struct test_bitmap_print test_print[] __initconst = {
+	{ small_bitmap, sizeof(small_bitmap) * BITS_PER_BYTE, small_mask, small_list },
+	{ large_bitmap, sizeof(large_bitmap) * BITS_PER_BYTE, large_mask, large_list },
+};
+
+static void __init test_bitmap_print_buf(void)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(test_print); i++) {
+		const struct test_bitmap_print *t = &test_print[i];
+		int n;
+
+		n = bitmap_print_to_buf(false, print_buf, t->bitmap,
+					t->nbits, 0, 2 * PAGE_SIZE);
+		expect_eq_uint(strlen(t->mask) + 1, n);
+		expect_eq_str(t->mask, print_buf, n);
+
+		n = bitmap_print_to_buf(true, print_buf, t->bitmap,
+					t->nbits, 0, 2 * PAGE_SIZE);
+		expect_eq_uint(strlen(t->list) + 1, n);
+		expect_eq_str(t->list, print_buf, n);
+
+		/* test bitmap_print_to_buf by non-zero offset */
+		if (strlen(t->list) > PAGE_SIZE) {
+			n = bitmap_print_to_buf(true, print_buf, t->bitmap,
+					t->nbits, PAGE_SIZE, PAGE_SIZE);
+			expect_eq_uint(strlen(t->list) + 1 - PAGE_SIZE, n);
+			expect_eq_str(t->list + PAGE_SIZE, print_buf, n);
+		}
+	}
+}
+
 static void __init selftest(void)
 {
 	test_zero_clear();
@@ -665,6 +813,7 @@ static void __init selftest(void)
 	test_mem_optimisations();
 	test_for_each_set_clump8();
 	test_bitmap_cut();
+	test_bitmap_print_buf();
 }
 
 KSTM_MODULE_LOADERS(test_bitmap);
-- 
2.25.1

Re: [PATCH v4 1/4] lib: bitmap: introduce bitmap_print_to_buf

[Greg KH]

On Thu, Jun 17, 2021 at 10:19:07PM +1200, Barry Song wrote:
> From: Tian Tao <tiantao6@hisilicon.com>
> 
> New API bitmap_print_to_buf() with bin_attribute to avoid maskp
> exceeding PAGE_SIZE.

I can not parse this sentance at all, sorry.

What does a "bin_attribute" mean to this change?  What does PAGE_SIZE
have to do with anything at all here?

> bitmap_print_to_pagebuf() is a special case
> of bitmap_print_to_buf(), so in bitmap_print_to_pagebuf() call
> bitmap_print_to_buf().

Again, I can not parse this, sorry.  Can you please try writing this in
a different way to be more descriptive?

> 
> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Stefano Brivio <sbrivio@redhat.com>
> Cc: Alexander Gordeev <agordeev@linux.ibm.com>
> Cc: "Ma, Jianpeng" <jianpeng.ma@intel.com>
> Cc: Yury Norov <yury.norov@gmail.com>
> Cc: Valentin Schneider <valentin.schneider@arm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Daniel Bristot de Oliveira <bristot@redhat.com>
> ---
>  include/linux/bitmap.h  |  2 ++
>  include/linux/cpumask.h | 21 +++++++++++++++++++++
>  lib/bitmap.c            | 37 +++++++++++++++++++++++++++++++++++--
>  3 files changed, 58 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> index a36cfcec4e77..0de6effa2797 100644
> --- a/include/linux/bitmap.h
> +++ b/include/linux/bitmap.h
> @@ -226,6 +226,8 @@ void bitmap_copy_le(unsigned long *dst, const unsigned long *src, unsigned int n
>  unsigned int bitmap_ord_to_pos(const unsigned long *bitmap, unsigned int ord, unsigned int nbits);
>  int bitmap_print_to_pagebuf(bool list, char *buf,
>  				   const unsigned long *maskp, int nmaskbits);
> +int bitmap_print_to_buf(bool list, char *buf, const unsigned long *maskp,
> +			int nmaskbits, loff_t off, size_t count);
>  
>  #define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) & (BITS_PER_LONG - 1)))
>  #define BITMAP_LAST_WORD_MASK(nbits) (~0UL >> (-(nbits) & (BITS_PER_LONG - 1)))
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index bfc4690de4f4..1bef69e4b950 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -983,6 +983,27 @@ cpumap_print_to_pagebuf(bool list, char *buf, const struct cpumask *mask)
>  				      nr_cpu_ids);
>  }
>  
> +/**
> + * cpumap_print_to_buf  - copies the cpumask into the buffer either
> + *      as comma-separated list of cpus or hex values of cpumask

How do you choose between a comma or a hex value?

> + * @list: indicates whether the cpumap must be list

Should this be the thing that determines a comma or not?  If true does
that mean a comma?  Or if false?  Can you please document this?


> + * @mask: the cpumask to copy
> + * @buf: the buffer to copy into
> + * @off: in the string from which we are copying, We copy to @buf + off
> + * @count: the maximum number of bytes to print
> + *
> + * The role of cpumap_print_to_buf() and cpumap_print_to_pagebuf() is
> + * the same, the difference is that buf of bitmap_print_to_buf()
> + * can be more than one pagesize.
> + */
> +static inline ssize_t
> +cpumap_print_to_buf(bool list, char *buf, const struct cpumask *mask,
> +		    loff_t off, size_t count)
> +{
> +	return bitmap_print_to_buf(list, buf, cpumask_bits(mask),
> +				   nr_cpu_ids, off, count);
> +}
> +
>  #if NR_CPUS <= BITS_PER_LONG
>  #define CPU_MASK_ALL							\
>  (cpumask_t) { {								\
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index 74ceb02f45e3..ed7ef6e2edca 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -461,6 +461,40 @@ int bitmap_parse_user(const char __user *ubuf,
>  }
>  EXPORT_SYMBOL(bitmap_parse_user);
>  
> +/**
> + * bitmap_print_to_buf - convert bitmap to list or hex format ASCII string
> + * @list: indicates whether the bitmap must be list
> + * @buf: the kernel space buffer to read to
> + * @maskp: pointer to bitmap to convert
> + * @nmaskbits: size of bitmap, in bits
> + * @off: offset in data buffer below
> + * @count: the maximum number of bytes to print
> + *
> + * The role of bitmap_print_to_buf() and bitmap_print_to_pagebuf() is
> + * the same, the difference is that buf of bitmap_print_to_buf()
> + * can be more than one pagesize.

"pagesize" does not make sense here.

Why does PAGE_SIZE have anything to do with this function at all?

> + */
> +int bitmap_print_to_buf(bool list, char *buf, const unsigned long *maskp,
> +			int nmaskbits, loff_t off, size_t count)
> +{
> +	const char *fmt = list ? "%*pbl\n" : "%*pb\n";
> +	ssize_t size;
> +	void *data;
> +
> +	if (off == LLONG_MAX && count == PAGE_SIZE - offset_in_page(buf))
> +		return scnprintf(buf, count, fmt, nmaskbits, maskp);

I do not understand this check, can you please comment why you are doing
it this way?


thanks,

greg k-h

Re: [PATCH v4 2/4] topology: use bin_attribute to avoid buff overflow

[Greg KH]

On Thu, Jun 17, 2021 at 10:19:08PM +1200, Barry Song wrote:
> From: Tian Tao <tiantao6@hisilicon.com>
> 
> 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.
> 
> Link: https://lore.kernel.org/lkml/20210319041618.14316-2-song.bao.hua@hisilicon.com/
> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>

Is this whole series supposed to go through my tree?

If so I'll wait on it based on my comments so far, otherwise I would
give an ack for this one...

thanks,

greg k-h

Re: [PATCH v4 3/4] drivers/base/node.c: use bin_attribute to avoid buff overflow

[Greg KH]

On Thu, Jun 17, 2021 at 10:19:09PM +1200, Barry Song wrote:
> From: Tian Tao <tiantao6@hisilicon.com>
> 
> Reading sys/devices/system/cpu/cpuX/nodeX/ returns cpumap and cpulist.
> 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.
> 
> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> ---
>  drivers/base/node.c | 52 +++++++++++++++++++++++++++++----------------
>  1 file changed, 34 insertions(+), 18 deletions(-)

You are forwarding on a patch from someone else, you HAVE to also sign
off on it as well.

thanks,

greg k-h

Re: [PATCH v4 2/4] topology: use bin_attribute to avoid buff overflow

[Greg KH]

On Thu, Jun 17, 2021 at 10:19:08PM +1200, Barry Song wrote:
> From: Tian Tao <tiantao6@hisilicon.com>
> 
> 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.
> 
> Link: https://lore.kernel.org/lkml/20210319041618.14316-2-song.bao.hua@hisilicon.com/
> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>

As with the other patch, you have to also sign off on this as well.

The link is pointing to a v5 of a different patch series, why put that
here?

thanks,

greg k-h

RE: [PATCH v4 2/4] topology: use bin_attribute to avoid buff overflow

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Thursday, June 17, 2021 10:56 PM
> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>
> Cc: andriy.shevchenko@linux.intel.com; linux-kernel@vger.kernel.org;
> linux@rasmusvillemoes.dk; rafael@kernel.org; akpm@linux-foundation.org;
> rdunlap@infradead.org; agordeev@linux.ibm.com; sbrivio@redhat.com;
> jianpeng.ma@intel.com; yury.norov@gmail.com; valentin.schneider@arm.com;
> peterz@infradead.org; bristot@redhat.com; guodong.xu@linaro.org;
> tangchengchang <tangchengchang@huawei.com>; Zengtao (B)
> <prime.zeng@hisilicon.com>; yangyicong <yangyicong@huawei.com>;
> tim.c.chen@linux.intel.com; tiantao (H) <tiantao6@hisilicon.com>; Jonathan
> Cameron <jonathan.cameron@huawei.com>; Linuxarm <linuxarm@huawei.com>
> Subject: Re: [PATCH v4 2/4] topology: use bin_attribute to avoid buff overflow
> 
> On Thu, Jun 17, 2021 at 10:19:08PM +1200, Barry Song wrote:
> > From: Tian Tao <tiantao6@hisilicon.com>
> >
> > 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.
> >
> > Link:
> https://lore.kernel.org/lkml/20210319041618.14316-2-song.bao.hua@hisilicon
> .com/
> > Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> > Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> 
> As with the other patch, you have to also sign off on this as well.
> 
Ok. Will add my s-o-b for patch 1-3.

> The link is pointing to a v5 of a different patch series, why put that
> here?

This patchset originated from the discussion of that one in the
link tag.

Jonathan and me were trying to add a cluster topology level.
And that one exposed the problem this patchset is addressing.

> 
> thanks,
> 
> greg k-h

Thanks
Barry

RE: [PATCH v4 2/4] topology: use bin_attribute to avoid buff overflow

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Thursday, June 17, 2021 10:55 PM
> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>
> Cc: andriy.shevchenko@linux.intel.com; linux-kernel@vger.kernel.org;
> linux@rasmusvillemoes.dk; rafael@kernel.org; akpm@linux-foundation.org;
> rdunlap@infradead.org; agordeev@linux.ibm.com; sbrivio@redhat.com;
> jianpeng.ma@intel.com; yury.norov@gmail.com; valentin.schneider@arm.com;
> peterz@infradead.org; bristot@redhat.com; guodong.xu@linaro.org;
> tangchengchang <tangchengchang@huawei.com>; Zengtao (B)
> <prime.zeng@hisilicon.com>; yangyicong <yangyicong@huawei.com>;
> tim.c.chen@linux.intel.com; tiantao (H) <tiantao6@hisilicon.com>; Jonathan
> Cameron <jonathan.cameron@huawei.com>; Linuxarm <linuxarm@huawei.com>
> Subject: Re: [PATCH v4 2/4] topology: use bin_attribute to avoid buff overflow
> 
> On Thu, Jun 17, 2021 at 10:19:08PM +1200, Barry Song wrote:
> > From: Tian Tao <tiantao6@hisilicon.com>
> >
> > 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.
> >
> > Link:
> https://lore.kernel.org/lkml/20210319041618.14316-2-song.bao.hua@hisilicon
> .com/
> > Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> > Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> 
> Is this whole series supposed to go through my tree?

I guess so, and that would be perfect.

> 
> If so I'll wait on it based on my comments so far, otherwise I would
> give an ack for this one...
> 

I will address those issues in your comments.


> thanks,
> 
> greg k-h

Thanks
Barry

Re: [PATCH v4 1/4] lib: bitmap: introduce bitmap_print_to_buf

[Yury Norov]

On Thu, Jun 17, 2021 at 10:19:07PM +1200, Barry Song wrote:
> From: Tian Tao <tiantao6@hisilicon.com>
> 
> New API bitmap_print_to_buf() with bin_attribute to avoid maskp
> exceeding PAGE_SIZE. bitmap_print_to_pagebuf() is a special case
> of bitmap_print_to_buf(), so in bitmap_print_to_pagebuf() call
> bitmap_print_to_buf().
> 
> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Stefano Brivio <sbrivio@redhat.com>
> Cc: Alexander Gordeev <agordeev@linux.ibm.com>
> Cc: "Ma, Jianpeng" <jianpeng.ma@intel.com>
> Cc: Yury Norov <yury.norov@gmail.com>
> Cc: Valentin Schneider <valentin.schneider@arm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Daniel Bristot de Oliveira <bristot@redhat.com>
> ---
>  include/linux/bitmap.h  |  2 ++
>  include/linux/cpumask.h | 21 +++++++++++++++++++++
>  lib/bitmap.c            | 37 +++++++++++++++++++++++++++++++++++--
>  3 files changed, 58 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> index a36cfcec4e77..0de6effa2797 100644
> --- a/include/linux/bitmap.h
> +++ b/include/linux/bitmap.h
> @@ -226,6 +226,8 @@ void bitmap_copy_le(unsigned long *dst, const unsigned long *src, unsigned int n
>  unsigned int bitmap_ord_to_pos(const unsigned long *bitmap, unsigned int ord, unsigned int nbits);
>  int bitmap_print_to_pagebuf(bool list, char *buf,
>  				   const unsigned long *maskp, int nmaskbits);
> +int bitmap_print_to_buf(bool list, char *buf, const unsigned long *maskp,
> +			int nmaskbits, loff_t off, size_t count);
>  
>  #define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) & (BITS_PER_LONG - 1)))
>  #define BITMAP_LAST_WORD_MASK(nbits) (~0UL >> (-(nbits) & (BITS_PER_LONG - 1)))
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index bfc4690de4f4..1bef69e4b950 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -983,6 +983,27 @@ cpumap_print_to_pagebuf(bool list, char *buf, const struct cpumask *mask)
>  				      nr_cpu_ids);
>  }
>  
> +/**
> + * cpumap_print_to_buf  - copies the cpumask into the buffer either
> + *      as comma-separated list of cpus or hex values of cpumask
> + * @list: indicates whether the cpumap must be list
> + * @mask: the cpumask to copy
> + * @buf: the buffer to copy into
> + * @off: in the string from which we are copying, We copy to @buf + off
> + * @count: the maximum number of bytes to print
> + *
> + * The role of cpumap_print_to_buf() and cpumap_print_to_pagebuf() is
> + * the same, the difference is that buf of bitmap_print_to_buf()
> + * can be more than one pagesize.
> + */
> +static inline ssize_t
> +cpumap_print_to_buf(bool list, char *buf, const struct cpumask *mask,
> +		    loff_t off, size_t count)
> +{
> +	return bitmap_print_to_buf(list, buf, cpumask_bits(mask),
> +				   nr_cpu_ids, off, count);
> +}
> +
>  #if NR_CPUS <= BITS_PER_LONG
>  #define CPU_MASK_ALL							\
>  (cpumask_t) { {								\
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index 74ceb02f45e3..ed7ef6e2edca 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -461,6 +461,40 @@ int bitmap_parse_user(const char __user *ubuf,
>  }
>  EXPORT_SYMBOL(bitmap_parse_user);
>  
> +/**
> + * bitmap_print_to_buf - convert bitmap to list or hex format ASCII string
> + * @list: indicates whether the bitmap must be list
> + * @buf: the kernel space buffer to read to
> + * @maskp: pointer to bitmap to convert
> + * @nmaskbits: size of bitmap, in bits
> + * @off: offset in data buffer below

Below there's a @count parameter. Maybe above?

> + * @count: the maximum number of bytes to print
> + *
> + * The role of bitmap_print_to_buf() and bitmap_print_to_pagebuf() is
> + * the same, the difference is that buf of bitmap_print_to_buf()
> + * can be more than one pagesize.

You're introducing a function that duplicates an existing function.
I can understand it only if there is important performance and/or
security consideration. Otherwise, I'd recommend you to extend
bitmap_print_to_pagebuf() with one more parameter.

> + */
> +int bitmap_print_to_buf(bool list, char *buf, const unsigned long *maskp,
> +			int nmaskbits, loff_t off, size_t count)
> +{
> +	const char *fmt = list ? "%*pbl\n" : "%*pb\n";
> +	ssize_t size;
> +	void *data;
> +
> +	if (off == LLONG_MAX && count == PAGE_SIZE - offset_in_page(buf))
> +		return scnprintf(buf, count, fmt, nmaskbits, maskp);

Agree with Greg. This requires explanation.

> +
> +	data = kasprintf(GFP_KERNEL, fmt, nmaskbits, maskp);
> +	if (!data)
> +		return -ENOMEM;

So here:
 - you make user of your function to guess about how much memory he needs
   to provide to the function;
 - then kasprintf() allocates enough memory for you automatically;
 - then you copy the data to potentially insufficient buffer;
 - and then free a valid 'data' buffer provided by kasprintf(), even if
   the following memory_read_from_buffer() fails;
 - goto 'So here'. 

It doesn't look like a good design.

kasprintf() does everything you need. Why don't you use it directly in
your driver?

> +
> +	size = memory_read_from_buffer(buf, count, &off, data, strlen(data) + 1);

strlen() is O(n), just as memory_read_from_buffer(). In this line you introduce
a 2*O(n) operation. Consider strncpy().

> +	kfree(data);
> +
> +	return size;
> +}
> +EXPORT_SYMBOL(bitmap_print_to_buf);
> +
>  /**
>   * bitmap_print_to_pagebuf - convert bitmap to list or hex format ASCII string
>   * @list: indicates whether the bitmap must be list
> @@ -482,8 +516,7 @@ int bitmap_print_to_pagebuf(bool list, char *buf, const unsigned long *maskp,
>  {
>  	ptrdiff_t len = PAGE_SIZE - offset_in_page(buf);
>  
> -	return list ? scnprintf(buf, len, "%*pbl\n", nmaskbits, maskp) :
> -		      scnprintf(buf, len, "%*pb\n", nmaskbits, maskp);
> +	return bitmap_print_to_buf(list, buf, maskp, nmaskbits, LLONG_MAX, len);
>  }
>  EXPORT_SYMBOL(bitmap_print_to_pagebuf);
>  
> -- 
> 2.25.1

Re: [PATCH v4 0/4] use bin_attribute to avoid cpumap buff overflow

[Yury Norov]

On Thu, Jun 17, 2021 at 10:19:06PM +1200, Barry Song wrote:
> patch #1 adds a new function cpumap_print_to_buf and patch #2 uses
> this function in drivers/base/topology.c, and patch #3 uses this new
> function in drivers/base/node.c.
> patch #4 adds test cases for the new API.

So, the root problem here is that some machines have so many CPUs that
their cpumask text representation may not fit into the full page in the
worst case. Is my understanding correct? Can you share an example of
such configuration?

I think that the proper solution would be to create a function like 
void *cpumap_alloc_pagebuf(bool list), so that cpumap_print_to_pagebuf()
will be always passed with enough portion of memory, and we'll just drop
the PAGE_SIZE limit in cpumap_print_to_pagebuf().

> 
> v4:
> add test cases for bitmap_print_to_buf API;
> add Reviewed-by of Jonathan Cameron for patches 1-3, thanks!
> 
> v3:
> fixed the strlen issue and patch #1,#2,#3 minor formatting issues, thanks
> to Andy Shevchenko and Jonathan Cameron.
> 
> v2:
> split the original patch #1 into two patches and use kasprintf() in
> patch #1 to simplify the code. do some minor formatting adjustments.
> 
> Barry Song (1):
>   lib: test_bitmap: add bitmap_print_to_buf test cases
> 
> Tian Tao (3):
>   lib: bitmap: introduce bitmap_print_to_buf
>   topology: use bin_attribute to avoid buff overflow
>   drivers/base/node.c: use bin_attribute to avoid buff overflow
> 
>  drivers/base/node.c     |  52 +++++++++-----
>  drivers/base/topology.c | 115 +++++++++++++++++--------------
>  include/linux/bitmap.h  |   2 +
>  include/linux/cpumask.h |  21 ++++++
>  lib/bitmap.c            |  37 +++++++++-
>  lib/test_bitmap.c       | 149 ++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 304 insertions(+), 72 deletions(-)
> 
> -- 
> 2.25.1

RE: [PATCH v4 0/4] use bin_attribute to avoid cpumap buff overflow

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Yury Norov [mailto:yury.norov@gmail.com]
> Sent: Friday, June 18, 2021 9:18 AM
> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>
> Cc: gregkh@linuxfoundation.org; andriy.shevchenko@linux.intel.com;
> linux-kernel@vger.kernel.org; linux@rasmusvillemoes.dk; rafael@kernel.org;
> akpm@linux-foundation.org; rdunlap@infradead.org; agordeev@linux.ibm.com;
> sbrivio@redhat.com; jianpeng.ma@intel.com; valentin.schneider@arm.com;
> peterz@infradead.org; bristot@redhat.com; guodong.xu@linaro.org;
> tangchengchang <tangchengchang@huawei.com>; Zengtao (B)
> <prime.zeng@hisilicon.com>; yangyicong <yangyicong@huawei.com>;
> tim.c.chen@linux.intel.com; tiantao (H) <tiantao6@hisilicon.com>; Jonathan
> Cameron <jonathan.cameron@huawei.com>; Linuxarm <linuxarm@huawei.com>
> Subject: Re: [PATCH v4 0/4] use bin_attribute to avoid cpumap buff overflow
> 
> On Thu, Jun 17, 2021 at 10:19:06PM +1200, Barry Song wrote:
> > patch #1 adds a new function cpumap_print_to_buf and patch #2 uses
> > this function in drivers/base/topology.c, and patch #3 uses this new
> > function in drivers/base/node.c.
> > patch #4 adds test cases for the new API.
> 
> So, the root problem here is that some machines have so many CPUs that
> their cpumask text representation may not fit into the full page in the
> worst case. Is my understanding correct? Can you share an example of
> such configuration?

in my understanding, I have not found any machine which has really
caused the problem till now. But the whole story began from this
thread when Jonatah and me tried to add a new topology level-cluster
which exists on kunpeng920 and X86 Jacobsville:
https://lore.kernel.org/lkml/YFRGIedW1fUlnmi+@kroah.com/
https://lore.kernel.org/lkml/YFR2kwakbcGiI37w@kroah.com/

in the discussion, Greg had some concern about the potential
overflow of sysfs ABI for topology. Greg's comment is reasonable
and I think we should address the problem.

For this moment, both numa node and cpu use cpu bitmap like 3,ffffffff to
expose hardware topology. When cpu number is large, the page buffer of
sysfs will overflow. This doesn't really happen nowadays as the maximum
NR_CPUS is 8196 for X86_64 and 4096 for ARM64 since 8196 * 9 / 32 = 2305
is still smaller than 4KB page size.

So the existing BUILD_BUG_ON() in drivers/base/node.c is pretty much
preventing future problems similar with Y2K when hardware gets more
and more CPUs:
static ssize_t node_read_cpumap(struct device *dev, bool list, char *buf)
{
 	cpumask_var_t mask;
 	struct node *node_dev = to_node(dev);
 
	/* 2008/04/07: buf currently PAGE_SIZE, need 9 chars per 32 bits. */
	BUILD_BUG_ON((NR_CPUS/32 * 9) > (PAGE_SIZE-1));
}

But other ABIs exposing cpu lists are much more tricky, they could be
like: 0, 3, 5, 7, 9, 11... etc. so nobody knows the size till the last
moment.

So in the previous discussion, we agreed to remove this kind of
limitation totally and remove the BUILD_BUG_ON().

> 
> I think that the proper solution would be to create a function like
> void *cpumap_alloc_pagebuf(bool list), so that cpumap_print_to_pagebuf()
> will be always passed with enough portion of memory, and we'll just drop
> the PAGE_SIZE limit in cpumap_print_to_pagebuf().

I am sorry we missed you in the previous discussion:
https://lore.kernel.org/lkml/1619679819-45256-1-git-send-email-tiantao6@hisilicon.com/#t

I think we have figured out bin_attribute is the way to workaround
the limitation of sysfs:

https://lore.kernel.org/lkml/fd78ac30-dd3b-a7d7-eae8-193b09a7d49a@intel.com/
https://lore.kernel.org/lkml/YIueOR4fOYa1dSAb@kroah.com/

Again sorry for you were missed in the previous discussion.

> 
> >
> > v4:
> > add test cases for bitmap_print_to_buf API;
> > add Reviewed-by of Jonathan Cameron for patches 1-3, thanks!
> >
> > v3:
> > fixed the strlen issue and patch #1,#2,#3 minor formatting issues, thanks
> > to Andy Shevchenko and Jonathan Cameron.
> >
> > v2:
> > split the original patch #1 into two patches and use kasprintf() in
> > patch #1 to simplify the code. do some minor formatting adjustments.
> >
> > Barry Song (1):
> >   lib: test_bitmap: add bitmap_print_to_buf test cases
> >
> > Tian Tao (3):
> >   lib: bitmap: introduce bitmap_print_to_buf
> >   topology: use bin_attribute to avoid buff overflow
> >   drivers/base/node.c: use bin_attribute to avoid buff overflow
> >
> >  drivers/base/node.c     |  52 +++++++++-----
> >  drivers/base/topology.c | 115 +++++++++++++++++--------------
> >  include/linux/bitmap.h  |   2 +
> >  include/linux/cpumask.h |  21 ++++++
> >  lib/bitmap.c            |  37 +++++++++-
> >  lib/test_bitmap.c       | 149 ++++++++++++++++++++++++++++++++++++++++
> >  6 files changed, 304 insertions(+), 72 deletions(-)
> >
> > --
> > 2.25.1

Thanks
Barry

RE: [PATCH v4 1/4] lib: bitmap: introduce bitmap_print_to_buf

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Thursday, June 17, 2021 10:47 PM
> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>
> Cc: andriy.shevchenko@linux.intel.com; linux-kernel@vger.kernel.org;
> linux@rasmusvillemoes.dk; rafael@kernel.org; akpm@linux-foundation.org;
> rdunlap@infradead.org; agordeev@linux.ibm.com; sbrivio@redhat.com;
> jianpeng.ma@intel.com; yury.norov@gmail.com; valentin.schneider@arm.com;
> peterz@infradead.org; bristot@redhat.com; guodong.xu@linaro.org;
> tangchengchang <tangchengchang@huawei.com>; Zengtao (B)
> <prime.zeng@hisilicon.com>; yangyicong <yangyicong@huawei.com>;
> tim.c.chen@linux.intel.com; tiantao (H) <tiantao6@hisilicon.com>; Jonathan
> Cameron <jonathan.cameron@huawei.com>; Linuxarm <linuxarm@huawei.com>
> Subject: Re: [PATCH v4 1/4] lib: bitmap: introduce bitmap_print_to_buf
> 
> On Thu, Jun 17, 2021 at 10:19:07PM +1200, Barry Song wrote:
> > From: Tian Tao <tiantao6@hisilicon.com>
> >
> > New API bitmap_print_to_buf() with bin_attribute to avoid maskp
> > exceeding PAGE_SIZE.
> 
> I can not parse this sentance at all, sorry.
> 
> What does a "bin_attribute" mean to this change?  What does PAGE_SIZE
> have to do with anything at all here?
> 
> > bitmap_print_to_pagebuf() is a special case
> > of bitmap_print_to_buf(), so in bitmap_print_to_pagebuf() call
> > bitmap_print_to_buf().
> 
> Again, I can not parse this, sorry.  Can you please try writing this in
> a different way to be more descriptive?
> 
> >
> > Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> > Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Randy Dunlap <rdunlap@infradead.org>
> > Cc: Stefano Brivio <sbrivio@redhat.com>
> > Cc: Alexander Gordeev <agordeev@linux.ibm.com>
> > Cc: "Ma, Jianpeng" <jianpeng.ma@intel.com>
> > Cc: Yury Norov <yury.norov@gmail.com>
> > Cc: Valentin Schneider <valentin.schneider@arm.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Daniel Bristot de Oliveira <bristot@redhat.com>
> > ---
> >  include/linux/bitmap.h  |  2 ++
> >  include/linux/cpumask.h | 21 +++++++++++++++++++++
> >  lib/bitmap.c            | 37 +++++++++++++++++++++++++++++++++++--
> >  3 files changed, 58 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> > index a36cfcec4e77..0de6effa2797 100644
> > --- a/include/linux/bitmap.h
> > +++ b/include/linux/bitmap.h
> > @@ -226,6 +226,8 @@ void bitmap_copy_le(unsigned long *dst, const unsigned
> long *src, unsigned int n
> >  unsigned int bitmap_ord_to_pos(const unsigned long *bitmap, unsigned int ord,
> unsigned int nbits);
> >  int bitmap_print_to_pagebuf(bool list, char *buf,
> >  				   const unsigned long *maskp, int nmaskbits);
> > +int bitmap_print_to_buf(bool list, char *buf, const unsigned long *maskp,
> > +			int nmaskbits, loff_t off, size_t count);
> >
> >  #define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) & (BITS_PER_LONG -
> 1)))
> >  #define BITMAP_LAST_WORD_MASK(nbits) (~0UL >> (-(nbits) & (BITS_PER_LONG -
> 1)))
> > diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> > index bfc4690de4f4..1bef69e4b950 100644
> > --- a/include/linux/cpumask.h
> > +++ b/include/linux/cpumask.h
> > @@ -983,6 +983,27 @@ cpumap_print_to_pagebuf(bool list, char *buf, const
> struct cpumask *mask)
> >  				      nr_cpu_ids);
> >  }
> >
> > +/**
> > + * cpumap_print_to_buf  - copies the cpumask into the buffer either
> > + *      as comma-separated list of cpus or hex values of cpumask
> 
> How do you choose between a comma or a hex value?
> 
> > + * @list: indicates whether the cpumap must be list
> 
> Should this be the thing that determines a comma or not?  If true does
> that mean a comma?  Or if false?  Can you please document this?
> 
> 
> > + * @mask: the cpumask to copy
> > + * @buf: the buffer to copy into
> > + * @off: in the string from which we are copying, We copy to @buf + off
> > + * @count: the maximum number of bytes to print
> > + *
> > + * The role of cpumap_print_to_buf() and cpumap_print_to_pagebuf() is
> > + * the same, the difference is that buf of bitmap_print_to_buf()
> > + * can be more than one pagesize.
> > + */
> > +static inline ssize_t
> > +cpumap_print_to_buf(bool list, char *buf, const struct cpumask *mask,
> > +		    loff_t off, size_t count)
> > +{
> > +	return bitmap_print_to_buf(list, buf, cpumask_bits(mask),
> > +				   nr_cpu_ids, off, count);
> > +}
> > +
> >  #if NR_CPUS <= BITS_PER_LONG
> >  #define CPU_MASK_ALL							\
> >  (cpumask_t) { {								\
> > diff --git a/lib/bitmap.c b/lib/bitmap.c
> > index 74ceb02f45e3..ed7ef6e2edca 100644
> > --- a/lib/bitmap.c
> > +++ b/lib/bitmap.c
> > @@ -461,6 +461,40 @@ int bitmap_parse_user(const char __user *ubuf,
> >  }
> >  EXPORT_SYMBOL(bitmap_parse_user);
> >
> > +/**
> > + * bitmap_print_to_buf - convert bitmap to list or hex format ASCII string
> > + * @list: indicates whether the bitmap must be list
> > + * @buf: the kernel space buffer to read to
> > + * @maskp: pointer to bitmap to convert
> > + * @nmaskbits: size of bitmap, in bits
> > + * @off: offset in data buffer below
> > + * @count: the maximum number of bytes to print
> > + *
> > + * The role of bitmap_print_to_buf() and bitmap_print_to_pagebuf() is
> > + * the same, the difference is that buf of bitmap_print_to_buf()
> > + * can be more than one pagesize.
> 
> "pagesize" does not make sense here.
> 
> Why does PAGE_SIZE have anything to do with this function at all?
> 
> > + */
> > +int bitmap_print_to_buf(bool list, char *buf, const unsigned long *maskp,
> > +			int nmaskbits, loff_t off, size_t count)
> > +{
> > +	const char *fmt = list ? "%*pbl\n" : "%*pb\n";
> > +	ssize_t size;
> > +	void *data;
> > +
> > +	if (off == LLONG_MAX && count == PAGE_SIZE - offset_in_page(buf))
> > +		return scnprintf(buf, count, fmt, nmaskbits, maskp);
> 
> I do not understand this check, can you please comment why you are doing
> it this way?

Discussed with Tiantao and figured out the reason. In the previous
version, this trick didn't exist.

In order that bitmap_print_to_pagebuf() can directly call
bitmap_print_to_buf() with the awareness of 
" ptrdiff_t len = PAGE_SIZE - offset_in_page(buf);" of the
original code:

int bitmap_print_to_pagebuf(bool list, char *buf, const unsigned long *maskp,
			    int nmaskbits)
{
	ptrdiff_t len = PAGE_SIZE - offset_in_page(buf);

	return list ? scnprintf(buf, len, "%*pbl\n", nmaskbits, maskp) :
		      scnprintf(buf, len, "%*pb\n", nmaskbits, maskp);
}
EXPORT_SYMBOL(bitmap_print_to_pagebuf);

This version is playing the trick by using LLONG_MAX as a "magic"
offset so that bitmap_print_to_buf() can do the particular job for
bitmap_print_to_pagebuf(). bitmap_print_to_pagebuf is working
under the assumption the buffer is just ONE page so it always
uses the unfinished part of the page as the length to print.

So the trick in bitmap_print_to_pagebuf() is like:

@@ -482,8 +516,7 @@ int bitmap_print_to_pagebuf(bool list, char *buf, const unsigned long *maskp,
 {
 	ptrdiff_t len = PAGE_SIZE - offset_in_page(buf);
 
-	return list ? scnprintf(buf, len, "%*pbl\n", nmaskbits, maskp) :
-		      scnprintf(buf, len, "%*pb\n", nmaskbits, maskp);
+	return bitmap_print_to_buf(list, buf, maskp, nmaskbits, LLONG_MAX, len);
 }
 EXPORT_SYMBOL(bitmap_print_to_pagebuf);

This is weird and this is ugly. So my understanding is that
is no necessity to touch bitmap_print_to_pagebuf? We can keep
bitmap_print_to_pagebuf as is. If users are quite sure their
ABIs won't be more than 1 page, they can still use this version
which doesn't have the complexity to calculate the exact
size of the final bitmap.


> 
> 
> thanks,
> 
> greg k-h

Thanks
Barry

RE: [PATCH v4 1/4] lib: bitmap: introduce bitmap_print_to_buf

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Yury Norov [mailto:yury.norov@gmail.com]
> Sent: Friday, June 18, 2021 8:54 AM
> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>
> Cc: gregkh@linuxfoundation.org; andriy.shevchenko@linux.intel.com;
> linux-kernel@vger.kernel.org; linux@rasmusvillemoes.dk; rafael@kernel.org;
> akpm@linux-foundation.org; rdunlap@infradead.org; agordeev@linux.ibm.com;
> sbrivio@redhat.com; jianpeng.ma@intel.com; valentin.schneider@arm.com;
> peterz@infradead.org; bristot@redhat.com; guodong.xu@linaro.org;
> tangchengchang <tangchengchang@huawei.com>; Zengtao (B)
> <prime.zeng@hisilicon.com>; yangyicong <yangyicong@huawei.com>;
> tim.c.chen@linux.intel.com; tiantao (H) <tiantao6@hisilicon.com>; Jonathan
> Cameron <jonathan.cameron@huawei.com>; Linuxarm <linuxarm@huawei.com>
> Subject: Re: [PATCH v4 1/4] lib: bitmap: introduce bitmap_print_to_buf
> 
> On Thu, Jun 17, 2021 at 10:19:07PM +1200, Barry Song wrote:
> > From: Tian Tao <tiantao6@hisilicon.com>
> >
> > New API bitmap_print_to_buf() with bin_attribute to avoid maskp
> > exceeding PAGE_SIZE. bitmap_print_to_pagebuf() is a special case
> > of bitmap_print_to_buf(), so in bitmap_print_to_pagebuf() call
> > bitmap_print_to_buf().
> >
> > Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> > Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Randy Dunlap <rdunlap@infradead.org>
> > Cc: Stefano Brivio <sbrivio@redhat.com>
> > Cc: Alexander Gordeev <agordeev@linux.ibm.com>
> > Cc: "Ma, Jianpeng" <jianpeng.ma@intel.com>
> > Cc: Yury Norov <yury.norov@gmail.com>
> > Cc: Valentin Schneider <valentin.schneider@arm.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Daniel Bristot de Oliveira <bristot@redhat.com>
> > ---
> >  include/linux/bitmap.h  |  2 ++
> >  include/linux/cpumask.h | 21 +++++++++++++++++++++
> >  lib/bitmap.c            | 37 +++++++++++++++++++++++++++++++++++--
> >  3 files changed, 58 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> > index a36cfcec4e77..0de6effa2797 100644
> > --- a/include/linux/bitmap.h
> > +++ b/include/linux/bitmap.h
> > @@ -226,6 +226,8 @@ void bitmap_copy_le(unsigned long *dst, const unsigned
> long *src, unsigned int n
> >  unsigned int bitmap_ord_to_pos(const unsigned long *bitmap, unsigned int ord,
> unsigned int nbits);
> >  int bitmap_print_to_pagebuf(bool list, char *buf,
> >  				   const unsigned long *maskp, int nmaskbits);
> > +int bitmap_print_to_buf(bool list, char *buf, const unsigned long *maskp,
> > +			int nmaskbits, loff_t off, size_t count);
> >
> >  #define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) & (BITS_PER_LONG -
> 1)))
> >  #define BITMAP_LAST_WORD_MASK(nbits) (~0UL >> (-(nbits) & (BITS_PER_LONG -
> 1)))
> > diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> > index bfc4690de4f4..1bef69e4b950 100644
> > --- a/include/linux/cpumask.h
> > +++ b/include/linux/cpumask.h
> > @@ -983,6 +983,27 @@ cpumap_print_to_pagebuf(bool list, char *buf, const
> struct cpumask *mask)
> >  				      nr_cpu_ids);
> >  }
> >
> > +/**
> > + * cpumap_print_to_buf  - copies the cpumask into the buffer either
> > + *      as comma-separated list of cpus or hex values of cpumask
> > + * @list: indicates whether the cpumap must be list
> > + * @mask: the cpumask to copy
> > + * @buf: the buffer to copy into
> > + * @off: in the string from which we are copying, We copy to @buf + off
> > + * @count: the maximum number of bytes to print
> > + *
> > + * The role of cpumap_print_to_buf() and cpumap_print_to_pagebuf() is
> > + * the same, the difference is that buf of bitmap_print_to_buf()
> > + * can be more than one pagesize.
> > + */
> > +static inline ssize_t
> > +cpumap_print_to_buf(bool list, char *buf, const struct cpumask *mask,
> > +		    loff_t off, size_t count)
> > +{
> > +	return bitmap_print_to_buf(list, buf, cpumask_bits(mask),
> > +				   nr_cpu_ids, off, count);
> > +}
> > +
> >  #if NR_CPUS <= BITS_PER_LONG
> >  #define CPU_MASK_ALL							\
> >  (cpumask_t) { {								\
> > diff --git a/lib/bitmap.c b/lib/bitmap.c
> > index 74ceb02f45e3..ed7ef6e2edca 100644
> > --- a/lib/bitmap.c
> > +++ b/lib/bitmap.c
> > @@ -461,6 +461,40 @@ int bitmap_parse_user(const char __user *ubuf,
> >  }
> >  EXPORT_SYMBOL(bitmap_parse_user);
> >
> > +/**
> > + * bitmap_print_to_buf - convert bitmap to list or hex format ASCII string
> > + * @list: indicates whether the bitmap must be list
> > + * @buf: the kernel space buffer to read to
> > + * @maskp: pointer to bitmap to convert
> > + * @nmaskbits: size of bitmap, in bits
> > + * @off: offset in data buffer below
> 
> Below there's a @count parameter. Maybe above?
> 
> > + * @count: the maximum number of bytes to print
> > + *
> > + * The role of bitmap_print_to_buf() and bitmap_print_to_pagebuf() is
> > + * the same, the difference is that buf of bitmap_print_to_buf()
> > + * can be more than one pagesize.
> 
> You're introducing a function that duplicates an existing function.
> I can understand it only if there is important performance and/or
> security consideration. Otherwise, I'd recommend you to extend
> bitmap_print_to_pagebuf() with one more parameter.

This would be possible. The only problem is that once we extend
bitmap_print_to_pagebuf() with one more parameter, we have to
change all its users. By involving one new api, we don't need
to touch those.

On the other hand, if we extend bitmap_print_to_pagebuf()with
one more parameter, we probably need to do the same job on
cpumap_print_to_pagebuf().

One parameter is, for sure, not enough as bin_attribute has
offset and count.

> 
> > + */
> > +int bitmap_print_to_buf(bool list, char *buf, const unsigned long *maskp,
> > +			int nmaskbits, loff_t off, size_t count)
> > +{
> > +	const char *fmt = list ? "%*pbl\n" : "%*pb\n";
> > +	ssize_t size;
> > +	void *data;
> > +
> > +	if (off == LLONG_MAX && count == PAGE_SIZE - offset_in_page(buf))
> > +		return scnprintf(buf, count, fmt, nmaskbits, maskp);
> 
> Agree with Greg. This requires explanation.

Explained in another email. This is a trick to let bitmap_print_to_buf
know bitmap_print_to_pagebuf is calling it.
But I'd remove it.

> 
> > +
> > +	data = kasprintf(GFP_KERNEL, fmt, nmaskbits, maskp);
> > +	if (!data)
> > +		return -ENOMEM;
> 
> So here:
>  - you make user of your function to guess about how much memory he needs
>    to provide to the function;
>  - then kasprintf() allocates enough memory for you automatically;
>  - then you copy the data to potentially insufficient buffer;
>  - and then free a valid 'data' buffer provided by kasprintf(), even if
>    the following memory_read_from_buffer() fails;
>  - goto 'So here'.
> 
> It doesn't look like a good design.
> 
> kasprintf() does everything you need. Why don't you use it directly in
> your driver?

I'm afraid I don't see the point of moving memory allocation to
the driver.
This api is mainly used by exporting cpumask bitmap to user in
a sysfs ABI. That means the destination buffer comes from sysfs,
and we have to copy the data to sysfs buf which is usually limited
to one page.

when cpumask bitmap or list is larger than one page, that means
sysfs's show entry will be called more than one time by different
offset and count. Not to mention user applications might
dynamically change the offset and count, then sysfs's show entry
might be called more than one time even though bitmap/list is
less than one page.

And drivers won't be able to re-use the allocated buffer in
multiple calling to the same sysfs show entry unless we make
some weird workaround to save the allocated memory address
somewhere.

Moving allocation to driver just means copying the same code
many times in different drivers. We have around 40 drivers
which are calling cpumap_print_to_pagebuf, all of them are
supposed to have the potential overflow issue and need update.

I'd argue putting the duplicated code of 40 drivers in the
common code is a better way.

But in this context, what is really useful is actually
cpumap_print_to_buf(). The safer way might be moving
kasprintf() to cpumap_print_to_buf(). Plus, I haven't
seen any other users of this new bitmap_print_to_buf()
API, so probably we can totally remove bitmap_print_to_buf()
and focus on making a better cpumap_print_to_buf.

The destination buffer which we are copying mask/list to
is actually quite safe in cpumap_print_to_buf() as
sysfs bin_attr guarantee the right offset and count.

If we remove bitmap_print_to_buf, we avoid the possible
abuse of this new bitmap API and avoid potential buffer
overflow issue. We are narrowing the scope of the
modification to cpumap.

I'd appreciate your further comments on that.

> 
> > +
> > +	size = memory_read_from_buffer(buf, count, &off, data, strlen(data) + 1);
> 
> strlen() is O(n), just as memory_read_from_buffer(). In this line you introduce
> a 2*O(n) operation. Consider strncpy().
> 
> > +	kfree(data);
> > +
> > +	return size;
> > +}
> > +EXPORT_SYMBOL(bitmap_print_to_buf);
> > +
> >  /**
> >   * bitmap_print_to_pagebuf - convert bitmap to list or hex format ASCII string
> >   * @list: indicates whether the bitmap must be list
> > @@ -482,8 +516,7 @@ int bitmap_print_to_pagebuf(bool list, char *buf, const
> unsigned long *maskp,
> >  {
> >  	ptrdiff_t len = PAGE_SIZE - offset_in_page(buf);
> >
> > -	return list ? scnprintf(buf, len, "%*pbl\n", nmaskbits, maskp) :
> > -		      scnprintf(buf, len, "%*pb\n", nmaskbits, maskp);
> > +	return bitmap_print_to_buf(list, buf, maskp, nmaskbits, LLONG_MAX, len);
> >  }
> >  EXPORT_SYMBOL(bitmap_print_to_pagebuf);
> >
> > --
> > 2.25.1

Thanks
Barry