[PATCH v3 0/3] use bin_attribute to avoid buff overflow

[PATCH v3 0/3] use bin_attribute to avoid buff overflow

[Tian Tao]

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.

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.

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  |   3 ++
 include/linux/cpumask.h |  21 +++++++++
 lib/bitmap.c            |  37 +++++++++++++++-
 5 files changed, 156 insertions(+), 72 deletions(-)

-- 
2.7.4

[PATCH v3 1/3] lib: bitmap: introduce bitmap_print_to_buf

[Tian Tao]

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>
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 a36cfce..0de6eff 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 bfc4690..1bef69e 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 9401d39..c64407e 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -462,6 +462,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
  * @buf: page aligned buffer into which string is placed
@@ -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.7.4

[PATCH v3 2/3] topology: use bin_attribute to avoid buff overflow

[Tian Tao]

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>
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 4d254fc..dd39801 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.7.4

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

[Tian Tao]

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>
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 4cef82c..537d046 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
@@ -557,15 +560,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.7.4

Re: [PATCH v3 0/3] use bin_attribute to avoid buff overflow

[Jonathan Cameron]

On Thu, 3 Jun 2021 17:22:39 +0800
Tian Tao <tiantao6@hisilicon.com> 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.
> 
> 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.
> 

All 3 patches now look good to me. Now I just need to get access to
a system that actually has enough CPUs to need this ;)

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> 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  |   3 ++
>  include/linux/cpumask.h |  21 +++++++++
>  lib/bitmap.c            |  37 +++++++++++++++-
>  5 files changed, 156 insertions(+), 72 deletions(-)
> 

Re: [PATCH v3 1/3] lib: bitmap: introduce bitmap_print_to_buf

[Andy Shevchenko]

On Thu, Jun 03, 2021 at 05:22:40PM +0800, Tian Tao wrote:
> 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().

...

>  /**
> + * 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);

Are you sure you have put parameters in the correct order?

> +	kfree(data);
> +
> +	return size;
> +}

I guess you have to provide the test case(s).

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Thu, Jun 03, 2021 at 05:22:42PM +0800, Tian Tao wrote:
> 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.

...

>  }
>  
> -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);

So, you will have 2 blank lines in a row after this. Why one is not enough?
Same question for other similar cases, if any.

-- 
With Best Regards,
Andy Shevchenko

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

[tiantao (H)]

在 2021/6/3 17:53, Andy Shevchenko 写道:
> On Thu, Jun 03, 2021 at 05:22:42PM +0800, Tian Tao wrote:
>> 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.
> ...
>
>>   }
>>   
>> -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);
> So, you will have 2 blank lines in a row after this. Why one is not enough?
> Same question for other similar cases, if any.

  I can delete the line 55. this is the only problem, are there any 
other problems?

47 static inline ssize_t cpumap_read(struct file *file, struct kobject 
*kobj,

   48                                   struct bin_attribute *attr, char 
*buf,
   49                                   loff_t off, size_t count)
   50 {
   51         struct device *dev = kobj_to_dev(kobj);
   52
   53         return node_read_cpumap(dev, false, buf, off, count);
   54 }
   55
   56
   57 static BIN_ATTR_RO(cpumap, 0);
   58
   59 static inline ssize_t cpulist_read(struct file *file, struct 
kobject *kobj,
   60                                    struct bin_attribute *attr, 
char *buf,
   61                                    loff_t off, size_t count)
   62 {
   63         struct device *dev = kobj_to_dev(kobj);
   64
   65         return node_read_cpumap(dev, true, buf, off, count);
   66 }
   67
   68 static BIN_ATTR_RO(cpulist, 0);

>

Re: [PATCH v3 1/3] lib: bitmap: introduce bitmap_print_to_buf

[tiantao (H)]

在 2021/6/3 17:50, Andy Shevchenko 写道:
> On Thu, Jun 03, 2021 at 05:22:40PM +0800, Tian Tao wrote:
>> 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().
> ...
>
>>   /**
>> + * 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);
> Are you sure you have put parameters in the correct order?

yes, I already test it.

ssize_t memory_read_from_buffer(void *to, size_t count, loff_t *ppos,
                                 const void *from, size_t available)

>
>> +	kfree(data);
>> +
>> +	return size;
>> +}
> I guess you have to provide the test case(s).
>

Re: [PATCH v3 1/3] lib: bitmap: introduce bitmap_print_to_buf

[Greg KH]

On Thu, Jun 03, 2021 at 06:33:25PM +0800, tiantao (H) wrote:
> 
> 在 2021/6/3 17:50, Andy Shevchenko 写道:
> > On Thu, Jun 03, 2021 at 05:22:40PM +0800, Tian Tao wrote:
> > > 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().
> > ...
> > 
> > >   /**
> > > + * 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);
> > Are you sure you have put parameters in the correct order?
> 
> yes, I already test it.

Great, can you add the test to the patch series as well so that we can
make sure it does not break in the future?

thanks,

greg k-h

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

[Andy Shevchenko]

On Thu, Jun 03, 2021 at 06:31:57PM +0800, tiantao (H) wrote:
> 在 2021/6/3 17:53, Andy Shevchenko 写道:
> > On Thu, Jun 03, 2021 at 05:22:42PM +0800, Tian Tao wrote:
> > > 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.
> > ...
> > 
> > >   }
> > > -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);
> > So, you will have 2 blank lines in a row after this. Why one is not enough?
> > Same question for other similar cases, if any.
> 
>  I can delete the line 55. this is the only problem, are there any other
> problems?

I don't know. I'm not familiar with the area.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/3] lib: bitmap: introduce bitmap_print_to_buf

[Andy Shevchenko]

On Thu, Jun 03, 2021 at 06:33:25PM +0800, tiantao (H) wrote:
> 在 2021/6/3 17:50, Andy Shevchenko 写道:
> > On Thu, Jun 03, 2021 at 05:22:40PM +0800, Tian Tao wrote:
> > > 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().

...

> > > + * @count: the maximum number of bytes to print

> > > +	size = memory_read_from_buffer(buf, count, &off, data, strlen(data) + 1);
> > Are you sure you have put parameters in the correct order?
> 
> yes, I already test it.
> 
> ssize_t memory_read_from_buffer(void *to, size_t count, loff_t *ppos,
>                                 const void *from, size_t available)

Have you read the meaning of count and available?
Please, double check that they are filled with correct values.

> > I guess you have to provide the test case(s).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/3] lib: bitmap: introduce bitmap_print_to_buf

[Greg KH]

On Thu, Jun 03, 2021 at 07:21:30PM +0800, tiantao (H) wrote:
> 
> 在 2021/6/3 18:49, Greg KH 写道:
> > On Thu, Jun 03, 2021 at 06:33:25PM +0800, tiantao (H) wrote:
> > > 在 2021/6/3 17:50, Andy Shevchenko 写道:
> > > > On Thu, Jun 03, 2021 at 05:22:40PM +0800, Tian Tao wrote:
> > > > > 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().
> > > > ...
> > > > 
> > > > >    /**
> > > > > + * 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);
> > > > Are you sure you have put parameters in the correct order?
> > > yes, I already test it.
> > Great, can you add the test to the patch series as well so that we can
> > make sure it does not break in the future?
> How do I do this?  Do I need to provide a kselftest ?

That would be wonderful, great idea!

thanks,

greg k-h

Re: [PATCH v3 1/3] lib: bitmap: introduce bitmap_print_to_buf

[Greg KH]

On Thu, Jun 03, 2021 at 01:37:09PM +0200, Greg KH wrote:
> On Thu, Jun 03, 2021 at 07:21:30PM +0800, tiantao (H) wrote:
> > 
> > 在 2021/6/3 18:49, Greg KH 写道:
> > > On Thu, Jun 03, 2021 at 06:33:25PM +0800, tiantao (H) wrote:
> > > > 在 2021/6/3 17:50, Andy Shevchenko 写道:
> > > > > On Thu, Jun 03, 2021 at 05:22:40PM +0800, Tian Tao wrote:
> > > > > > 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().
> > > > > ...
> > > > > 
> > > > > >    /**
> > > > > > + * 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);
> > > > > Are you sure you have put parameters in the correct order?
> > > > yes, I already test it.
> > > Great, can you add the test to the patch series as well so that we can
> > > make sure it does not break in the future?
> > How do I do this?  Do I need to provide a kselftest ?
> 
> That would be wonderful, great idea!

Or, as this is an internal kernel api, using kunit might be easier.

Obviously you tested this somehow, so just take advantage of the code
you wrote for that.

thanks,

greg k-h

Re: [PATCH v3 1/3] lib: bitmap: introduce bitmap_print_to_buf

[Jonathan Cameron]

On Thu, 3 Jun 2021 14:11:16 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Thu, Jun 03, 2021 at 06:33:25PM +0800, tiantao (H) wrote:
> > 在 2021/6/3 17:50, Andy Shevchenko 写道:  
> > > On Thu, Jun 03, 2021 at 05:22:40PM +0800, Tian Tao wrote:  
> > > > 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().  
> 
> ...
> 
> > > > + * @count: the maximum number of bytes to print  
> 
> > > > +	size = memory_read_from_buffer(buf, count, &off, data, strlen(data) + 1);  
> > > Are you sure you have put parameters in the correct order?  
> > 
> > yes, I already test it.
> > 
> > ssize_t memory_read_from_buffer(void *to, size_t count, loff_t *ppos,
> >                                 const void *from, size_t available)  
> 
> Have you read the meaning of count and available?
> Please, double check that they are filled with correct values.

Ok, I don't get this one either so can you give us more of a hint?

/**
 * memory_read_from_buffer - copy data from the buffer
 * @to: the kernel space buffer to read to
 * @count: the maximum number of bytes to read
 * @ppos: the current position in the buffer
 * @from: the buffer to read from
 * @available: the size of the buffer
 *
 * The memory_read_from_buffer() function reads up to @count bytes from the
 * buffer @from at offset @ppos into the kernel space address starting at @to.
 *
 * On success, the number of bytes read is returned and the offset @ppos is
 * advanced by this number, or negative value is returned on error.
 **/

These docs do end up rather confusing by using the term buffer for multiple things
but taking what is passed in.

Count is the maximum in the sense of how many bytes we are requesting are read
which indeed should be count here as that reflects what userspace asked for.

Avail is the size of the buffer we are reading from.  Now that's slightly
ambiguous in the docs in the sense of 'buffer' could mean the to buffer or
the from buffer.  However, I'd assume count is definitely <= size of the space
after address to in the to buffer, so I would assume that means available
is the size of the from buffer.  Here that is strlen() + 1, so looks fine.

This interpretation also lines up with the implementation.

So what are we missing?

Jonathan

> 
> > > I guess you have to provide the test case(s).  
> 

Re: [PATCH v3 1/3] lib: bitmap: introduce bitmap_print_to_buf

[Andy Shevchenko]

On Thu, Jun 03, 2021 at 01:39:19PM +0100, Jonathan Cameron wrote:
> On Thu, 3 Jun 2021 14:11:16 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > On Thu, Jun 03, 2021 at 06:33:25PM +0800, tiantao (H) wrote:
> > > 在 2021/6/3 17:50, Andy Shevchenko 写道:  
> > > > On Thu, Jun 03, 2021 at 05:22:40PM +0800, Tian Tao wrote:  
> > > > > 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().  
> > 
> > ...
> > 
> > > > > + * @count: the maximum number of bytes to print  
> > 
> > > > > +	size = memory_read_from_buffer(buf, count, &off, data, strlen(data) + 1);  
> > > > Are you sure you have put parameters in the correct order?  
> > > 
> > > yes, I already test it.
> > > 
> > > ssize_t memory_read_from_buffer(void *to, size_t count, loff_t *ppos,
> > >                                 const void *from, size_t available)  
> > 
> > Have you read the meaning of count and available?
> > Please, double check that they are filled with correct values.
> 
> Ok, I don't get this one either so can you give us more of a hint?

There is no hint, as you noticed the documentation of the function is a bit
confusing.

> /**
>  * memory_read_from_buffer - copy data from the buffer
>  * @to: the kernel space buffer to read to
>  * @count: the maximum number of bytes to read
>  * @ppos: the current position in the buffer
>  * @from: the buffer to read from
>  * @available: the size of the buffer
>  *
>  * The memory_read_from_buffer() function reads up to @count bytes from the
>  * buffer @from at offset @ppos into the kernel space address starting at @to.
>  *
>  * On success, the number of bytes read is returned and the offset @ppos is
>  * advanced by this number, or negative value is returned on error.
>  **/
> 
> These docs do end up rather confusing by using the term buffer for multiple things
> but taking what is passed in.
> 
> Count is the maximum in the sense of how many bytes we are requesting are read
> which indeed should be count here as that reflects what userspace asked for.
> 
> Avail is the size of the buffer we are reading from.  Now that's slightly
> ambiguous in the docs in the sense of 'buffer' could mean the to buffer or
> the from buffer.  However, I'd assume count is definitely <= size of the space
> after address to in the to buffer, so I would assume that means available
> is the size of the from buffer.  Here that is strlen() + 1, so looks fine.
> 
> This interpretation also lines up with the implementation.
> 
> So what are we missing?

Thanks for double checking and explaining.

> > > > I guess you have to provide the test case(s).

Just test cases is what we are missing. Then we can play around with different
input to see if it's all correct.

-- 
With Best Regards,
Andy Shevchenko