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
From: Tian Tao <[email protected]>
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 <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Stefano Brivio <[email protected]>
Cc: Alexander Gordeev <[email protected]>
Cc: "Ma, Jianpeng" <[email protected]>
Cc: Yury Norov <[email protected]>
Cc: Valentin Schneider <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Daniel Bristot de Oliveira <[email protected]>
---
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
On Thu, Jun 17, 2021 at 10:19:07PM +1200, Barry Song wrote:
> From: Tian Tao <[email protected]>
>
> 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 <[email protected]>
> Reviewed-by: Jonathan Cameron <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Andy Shevchenko <[email protected]>
> Cc: Randy Dunlap <[email protected]>
> Cc: Stefano Brivio <[email protected]>
> Cc: Alexander Gordeev <[email protected]>
> Cc: "Ma, Jianpeng" <[email protected]>
> Cc: Yury Norov <[email protected]>
> Cc: Valentin Schneider <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Daniel Bristot de Oliveira <[email protected]>
> ---
> 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
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
> -----Original Message-----
> From: Yury Norov [mailto:[email protected]]
> Sent: Friday, June 18, 2021 9:18 AM
> To: Song Bao Hua (Barry Song) <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> tangchengchang <[email protected]>; Zengtao (B)
> <[email protected]>; yangyicong <[email protected]>;
> [email protected]; tiantao (H) <[email protected]>; Jonathan
> Cameron <[email protected]>; Linuxarm <[email protected]>
> 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/[email protected]/
https://lore.kernel.org/lkml/[email protected]/
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/[email protected]/#t
I think we have figured out bin_attribute is the way to workaround
the limitation of sysfs:
https://lore.kernel.org/lkml/[email protected]/
https://lore.kernel.org/lkml/[email protected]/
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
> -----Original Message-----
> From: Yury Norov [mailto:[email protected]]
> Sent: Friday, June 18, 2021 8:54 AM
> To: Song Bao Hua (Barry Song) <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> tangchengchang <[email protected]>; Zengtao (B)
> <[email protected]>; yangyicong <[email protected]>;
> [email protected]; tiantao (H) <[email protected]>; Jonathan
> Cameron <[email protected]>; Linuxarm <[email protected]>
> 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 <[email protected]>
> >
> > 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 <[email protected]>
> > Reviewed-by: Jonathan Cameron <[email protected]>
> > Cc: Andrew Morton <[email protected]>
> > Cc: Andy Shevchenko <[email protected]>
> > Cc: Randy Dunlap <[email protected]>
> > Cc: Stefano Brivio <[email protected]>
> > Cc: Alexander Gordeev <[email protected]>
> > Cc: "Ma, Jianpeng" <[email protected]>
> > Cc: Yury Norov <[email protected]>
> > Cc: Valentin Schneider <[email protected]>
> > Cc: Peter Zijlstra <[email protected]>
> > Cc: Daniel Bristot de Oliveira <[email protected]>
> > ---
> > 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