* [PATCH v4 0/4] use bin_attribute to avoid cpumap buff overflow @ 2021-06-17 10:19 Barry Song 2021-06-17 10:19 ` [PATCH v4 1/4] lib: bitmap: introduce bitmap_print_to_buf Barry Song ` (4 more replies) 0 siblings, 5 replies; 16+ messages in thread From: Barry Song @ 2021-06-17 10:19 UTC (permalink / raw) To: gregkh, andriy.shevchenko, linux-kernel Cc: linux, rafael, akpm, rdunlap, agordeev, sbrivio, jianpeng.ma, yury.norov, valentin.schneider, peterz, bristot, guodong.xu, tangchengchang, prime.zeng, yangyicong, tim.c.chen, tiantao6, Jonathan.Cameron, linuxarm, 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 1/4] lib: bitmap: introduce bitmap_print_to_buf 2021-06-17 10:19 [PATCH v4 0/4] use bin_attribute to avoid cpumap buff overflow Barry Song @ 2021-06-17 10:19 ` Barry Song 2021-06-17 10:47 ` Greg KH 2021-06-17 20:54 ` Yury Norov 2021-06-17 10:19 ` [PATCH v4 2/4] topology: use bin_attribute to avoid buff overflow Barry Song ` (3 subsequent siblings) 4 siblings, 2 replies; 16+ messages in thread From: Barry Song @ 2021-06-17 10:19 UTC (permalink / raw) To: gregkh, andriy.shevchenko, linux-kernel Cc: linux, rafael, akpm, rdunlap, agordeev, sbrivio, jianpeng.ma, yury.norov, valentin.schneider, peterz, bristot, guodong.xu, tangchengchang, prime.zeng, yangyicong, tim.c.chen, tiantao6, Jonathan.Cameron, linuxarm, Jonathan Cameron 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/4] lib: bitmap: introduce bitmap_print_to_buf 2021-06-17 10:19 ` [PATCH v4 1/4] lib: bitmap: introduce bitmap_print_to_buf Barry Song @ 2021-06-17 10:47 ` Greg KH 2021-06-18 2:14 ` Song Bao Hua (Barry Song) 2021-06-17 20:54 ` Yury Norov 1 sibling, 1 reply; 16+ messages in thread From: Greg KH @ 2021-06-17 10:47 UTC (permalink / raw) To: Barry Song Cc: andriy.shevchenko, linux-kernel, linux, rafael, akpm, rdunlap, agordeev, sbrivio, jianpeng.ma, yury.norov, valentin.schneider, peterz, bristot, guodong.xu, tangchengchang, prime.zeng, yangyicong, tim.c.chen, tiantao6, Jonathan.Cameron, linuxarm 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH v4 1/4] lib: bitmap: introduce bitmap_print_to_buf 2021-06-17 10:47 ` Greg KH @ 2021-06-18 2:14 ` Song Bao Hua (Barry Song) 0 siblings, 0 replies; 16+ messages in thread From: Song Bao Hua (Barry Song) @ 2021-06-18 2:14 UTC (permalink / raw) To: Greg KH Cc: andriy.shevchenko, linux-kernel, linux, rafael, akpm, rdunlap, agordeev, sbrivio, jianpeng.ma, yury.norov, valentin.schneider, peterz, bristot, guodong.xu, tangchengchang, Zengtao (B), yangyicong, tim.c.chen, tiantao (H), Jonathan Cameron, Linuxarm > -----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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/4] lib: bitmap: introduce bitmap_print_to_buf 2021-06-17 10:19 ` [PATCH v4 1/4] lib: bitmap: introduce bitmap_print_to_buf Barry Song 2021-06-17 10:47 ` Greg KH @ 2021-06-17 20:54 ` Yury Norov 2021-07-01 11:59 ` Song Bao Hua (Barry Song) 1 sibling, 1 reply; 16+ messages in thread From: Yury Norov @ 2021-06-17 20:54 UTC (permalink / raw) To: Barry Song Cc: gregkh, andriy.shevchenko, linux-kernel, linux, rafael, akpm, rdunlap, agordeev, sbrivio, jianpeng.ma, valentin.schneider, peterz, bristot, guodong.xu, tangchengchang, prime.zeng, yangyicong, tim.c.chen, tiantao6, Jonathan.Cameron, linuxarm 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH v4 1/4] lib: bitmap: introduce bitmap_print_to_buf 2021-06-17 20:54 ` Yury Norov @ 2021-07-01 11:59 ` Song Bao Hua (Barry Song) 0 siblings, 0 replies; 16+ messages in thread From: Song Bao Hua (Barry Song) @ 2021-07-01 11:59 UTC (permalink / raw) To: Yury Norov Cc: gregkh, andriy.shevchenko, linux-kernel, linux, rafael, akpm, rdunlap, agordeev, sbrivio, jianpeng.ma, valentin.schneider, peterz, bristot, guodong.xu, tangchengchang, Zengtao (B), yangyicong, tim.c.chen, tiantao (H), Jonathan Cameron, Linuxarm > -----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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 2/4] topology: use bin_attribute to avoid buff overflow 2021-06-17 10:19 [PATCH v4 0/4] use bin_attribute to avoid cpumap buff overflow Barry Song 2021-06-17 10:19 ` [PATCH v4 1/4] lib: bitmap: introduce bitmap_print_to_buf Barry Song @ 2021-06-17 10:19 ` Barry Song 2021-06-17 10:54 ` Greg KH 2021-06-17 10:55 ` Greg KH 2021-06-17 10:19 ` [PATCH v4 3/4] drivers/base/node.c: " Barry Song ` (2 subsequent siblings) 4 siblings, 2 replies; 16+ messages in thread From: Barry Song @ 2021-06-17 10:19 UTC (permalink / raw) To: gregkh, andriy.shevchenko, linux-kernel Cc: linux, rafael, akpm, rdunlap, agordeev, sbrivio, jianpeng.ma, yury.norov, valentin.schneider, peterz, bristot, guodong.xu, tangchengchang, prime.zeng, yangyicong, tim.c.chen, tiantao6, Jonathan.Cameron, linuxarm, Jonathan Cameron 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v4 2/4] topology: use bin_attribute to avoid buff overflow 2021-06-17 10:19 ` [PATCH v4 2/4] topology: use bin_attribute to avoid buff overflow Barry Song @ 2021-06-17 10:54 ` Greg KH 2021-06-17 11:06 ` Song Bao Hua (Barry Song) 2021-06-17 10:55 ` Greg KH 1 sibling, 1 reply; 16+ messages in thread From: Greg KH @ 2021-06-17 10:54 UTC (permalink / raw) To: Barry Song Cc: andriy.shevchenko, linux-kernel, linux, rafael, akpm, rdunlap, agordeev, sbrivio, jianpeng.ma, yury.norov, valentin.schneider, peterz, bristot, guodong.xu, tangchengchang, prime.zeng, yangyicong, tim.c.chen, tiantao6, Jonathan.Cameron, linuxarm 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH v4 2/4] topology: use bin_attribute to avoid buff overflow 2021-06-17 10:54 ` Greg KH @ 2021-06-17 11:06 ` Song Bao Hua (Barry Song) 0 siblings, 0 replies; 16+ messages in thread From: Song Bao Hua (Barry Song) @ 2021-06-17 11:06 UTC (permalink / raw) To: Greg KH Cc: andriy.shevchenko, linux-kernel, linux, rafael, akpm, rdunlap, agordeev, sbrivio, jianpeng.ma, yury.norov, valentin.schneider, peterz, bristot, guodong.xu, tangchengchang, Zengtao (B), yangyicong, tim.c.chen, tiantao (H), Jonathan Cameron, Linuxarm > -----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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 2/4] topology: use bin_attribute to avoid buff overflow 2021-06-17 10:19 ` [PATCH v4 2/4] topology: use bin_attribute to avoid buff overflow Barry Song 2021-06-17 10:54 ` Greg KH @ 2021-06-17 10:55 ` Greg KH 2021-06-17 11:03 ` Song Bao Hua (Barry Song) 1 sibling, 1 reply; 16+ messages in thread From: Greg KH @ 2021-06-17 10:55 UTC (permalink / raw) To: Barry Song Cc: andriy.shevchenko, linux-kernel, linux, rafael, akpm, rdunlap, agordeev, sbrivio, jianpeng.ma, yury.norov, valentin.schneider, peterz, bristot, guodong.xu, tangchengchang, prime.zeng, yangyicong, tim.c.chen, tiantao6, Jonathan.Cameron, linuxarm 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH v4 2/4] topology: use bin_attribute to avoid buff overflow 2021-06-17 10:55 ` Greg KH @ 2021-06-17 11:03 ` Song Bao Hua (Barry Song) 0 siblings, 0 replies; 16+ messages in thread From: Song Bao Hua (Barry Song) @ 2021-06-17 11:03 UTC (permalink / raw) To: Greg KH Cc: andriy.shevchenko, linux-kernel, linux, rafael, akpm, rdunlap, agordeev, sbrivio, jianpeng.ma, yury.norov, valentin.schneider, peterz, bristot, guodong.xu, tangchengchang, Zengtao (B), yangyicong, tim.c.chen, tiantao (H), Jonathan Cameron, Linuxarm > -----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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 3/4] drivers/base/node.c: use bin_attribute to avoid buff overflow 2021-06-17 10:19 [PATCH v4 0/4] use bin_attribute to avoid cpumap buff overflow Barry Song 2021-06-17 10:19 ` [PATCH v4 1/4] lib: bitmap: introduce bitmap_print_to_buf Barry Song 2021-06-17 10:19 ` [PATCH v4 2/4] topology: use bin_attribute to avoid buff overflow Barry Song @ 2021-06-17 10:19 ` Barry Song 2021-06-17 10:55 ` Greg KH 2021-06-17 10:19 ` [PATCH v4 4/4] lib: test_bitmap: add bitmap_print_to_buf test cases Barry Song 2021-06-17 21:17 ` [PATCH v4 0/4] use bin_attribute to avoid cpumap buff overflow Yury Norov 4 siblings, 1 reply; 16+ messages in thread From: Barry Song @ 2021-06-17 10:19 UTC (permalink / raw) To: gregkh, andriy.shevchenko, linux-kernel Cc: linux, rafael, akpm, rdunlap, agordeev, sbrivio, jianpeng.ma, yury.norov, valentin.schneider, peterz, bristot, guodong.xu, tangchengchang, prime.zeng, yangyicong, tim.c.chen, tiantao6, Jonathan.Cameron, linuxarm, Jonathan Cameron 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v4 3/4] drivers/base/node.c: use bin_attribute to avoid buff overflow 2021-06-17 10:19 ` [PATCH v4 3/4] drivers/base/node.c: " Barry Song @ 2021-06-17 10:55 ` Greg KH 0 siblings, 0 replies; 16+ messages in thread From: Greg KH @ 2021-06-17 10:55 UTC (permalink / raw) To: Barry Song Cc: andriy.shevchenko, linux-kernel, linux, rafael, akpm, rdunlap, agordeev, sbrivio, jianpeng.ma, yury.norov, valentin.schneider, peterz, bristot, guodong.xu, tangchengchang, prime.zeng, yangyicong, tim.c.chen, tiantao6, Jonathan.Cameron, linuxarm 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 4/4] lib: test_bitmap: add bitmap_print_to_buf test cases 2021-06-17 10:19 [PATCH v4 0/4] use bin_attribute to avoid cpumap buff overflow Barry Song ` (2 preceding siblings ...) 2021-06-17 10:19 ` [PATCH v4 3/4] drivers/base/node.c: " Barry Song @ 2021-06-17 10:19 ` Barry Song 2021-06-17 21:17 ` [PATCH v4 0/4] use bin_attribute to avoid cpumap buff overflow Yury Norov 4 siblings, 0 replies; 16+ messages in thread From: Barry Song @ 2021-06-17 10:19 UTC (permalink / raw) To: gregkh, andriy.shevchenko, linux-kernel Cc: linux, rafael, akpm, rdunlap, agordeev, sbrivio, jianpeng.ma, yury.norov, valentin.schneider, peterz, bristot, guodong.xu, tangchengchang, prime.zeng, yangyicong, tim.c.chen, tiantao6, Jonathan.Cameron, linuxarm, 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v4 0/4] use bin_attribute to avoid cpumap buff overflow 2021-06-17 10:19 [PATCH v4 0/4] use bin_attribute to avoid cpumap buff overflow Barry Song ` (3 preceding siblings ...) 2021-06-17 10:19 ` [PATCH v4 4/4] lib: test_bitmap: add bitmap_print_to_buf test cases Barry Song @ 2021-06-17 21:17 ` Yury Norov 2021-06-17 23:48 ` Song Bao Hua (Barry Song) 4 siblings, 1 reply; 16+ messages in thread From: Yury Norov @ 2021-06-17 21:17 UTC (permalink / raw) To: Barry Song Cc: gregkh, andriy.shevchenko, linux-kernel, linux, rafael, akpm, rdunlap, agordeev, sbrivio, jianpeng.ma, valentin.schneider, peterz, bristot, guodong.xu, tangchengchang, prime.zeng, yangyicong, tim.c.chen, tiantao6, Jonathan.Cameron, linuxarm 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH v4 0/4] use bin_attribute to avoid cpumap buff overflow 2021-06-17 21:17 ` [PATCH v4 0/4] use bin_attribute to avoid cpumap buff overflow Yury Norov @ 2021-06-17 23:48 ` Song Bao Hua (Barry Song) 0 siblings, 0 replies; 16+ messages in thread From: Song Bao Hua (Barry Song) @ 2021-06-17 23:48 UTC (permalink / raw) To: Yury Norov Cc: gregkh, andriy.shevchenko, linux-kernel, linux, rafael, akpm, rdunlap, agordeev, sbrivio, jianpeng.ma, valentin.schneider, peterz, bristot, guodong.xu, tangchengchang, Zengtao (B), yangyicong, tim.c.chen, tiantao (H), Jonathan Cameron, Linuxarm > -----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 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-07-01 11:59 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-17 10:19 [PATCH v4 0/4] use bin_attribute to avoid cpumap buff overflow Barry Song 2021-06-17 10:19 ` [PATCH v4 1/4] lib: bitmap: introduce bitmap_print_to_buf Barry Song 2021-06-17 10:47 ` Greg KH 2021-06-18 2:14 ` Song Bao Hua (Barry Song) 2021-06-17 20:54 ` Yury Norov 2021-07-01 11:59 ` Song Bao Hua (Barry Song) 2021-06-17 10:19 ` [PATCH v4 2/4] topology: use bin_attribute to avoid buff overflow Barry Song 2021-06-17 10:54 ` Greg KH 2021-06-17 11:06 ` Song Bao Hua (Barry Song) 2021-06-17 10:55 ` Greg KH 2021-06-17 11:03 ` Song Bao Hua (Barry Song) 2021-06-17 10:19 ` [PATCH v4 3/4] drivers/base/node.c: " Barry Song 2021-06-17 10:55 ` Greg KH 2021-06-17 10:19 ` [PATCH v4 4/4] lib: test_bitmap: add bitmap_print_to_buf test cases Barry Song 2021-06-17 21:17 ` [PATCH v4 0/4] use bin_attribute to avoid cpumap buff overflow Yury Norov 2021-06-17 23:48 ` Song Bao Hua (Barry Song)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).