Re: [PATCH] cpumask: Omit terminating null byte in cpumap_print_{list,bitmask}_to_buf

[Yury Norov <yury.norov@gmail.com>]

[CC Greg KH <gregkh@linuxfoundation.org>]

On Thu, Sep 16, 2021 at 10:53:39PM +0000, Song Bao Hua (Barry Song) wrote:
> 
> 
> > -----Original Message-----
> > From: Tobias Klauser [mailto:tklauser@distanz.ch]
> > Sent: Friday, September 17, 2021 10:27 AM
> > To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jonathan Cameron
> > <jonathan.cameron@huawei.com>; tiantao (H) <tiantao6@hisilicon.com>; Song Bao
> > Hua (Barry Song) <song.bao.hua@hisilicon.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>; Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com>; Yury Norov <yury.norov@gmail.com>; Peter
> > Zijlstra <peterz@infradead.org>; linux-kernel@vger.kernel.org
> > Subject: [PATCH] cpumask: Omit terminating null byte in
> > cpumap_print_{list,bitmask}_to_buf
> > 
> > The changes in the patch series [1] introduced a terminating null byte
> > when reading from cpulist or cpumap sysfs files, for example:
> > 
> >   $ xxd /sys/devices/system/node/node0/cpulist
> >   00000000: 302d 310a 00                             0-1..
> > 
> > Before this change, the output looked as follows:
> > 
> >   $ xxd /sys/devices/system/node/node0/cpulist
> >   00000000: 302d 310a                                0-1.
> 
> If we don't use xxd, I don't see any actual harm of this NULL byte
> by cat, lscpu, numactl etc. this doesn't break them at all.

Barry, Tobias' script that uses xxd is userspace. Linux kernel never breaks
userspace. 

> if we only want to make sure the output is exactly same with before
> for every single character, this patch is right.

We don't want to make the output exactly the same. The "0,1" would
also work for the example above. But garbage characters following \0
is a bug that should be fixed.

Greg, would you like to move this patch through your tree?

Acked-by: Yury Norov <yury.norov@gmail.com>

> > Fix this regression by excluding the terminating null byte from the
> > returned length in cpumap_print_list_to_buf and
> > cpumap_print_bitmask_to_buf.
> > 
> > [1]
> > https://lore.kernel.org/all/20210806110251.560-1-song.bao.hua@hisilicon.co
> > m/
> > 
> > Fixes: 1fae562983ca ("cpumask: introduce cpumap_print_list/bitmask_to_buf to
> > support large bitmask and list")
> > Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
> > ---
> >  include/linux/cpumask.h | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> > index 5d4d07a9e1ed..1e7399fc69c0 100644
> > --- a/include/linux/cpumask.h
> > +++ b/include/linux/cpumask.h
> > @@ -996,14 +996,15 @@ cpumap_print_to_pagebuf(bool list, char *buf, const struct
> > cpumask *mask)
> >   * cpumask; Typically used by bin_attribute to export cpumask bitmask
> >   * ABI.
> >   *
> > - * Returns the length of how many bytes have been copied.
> > + * Returns the length of how many bytes have been copied, excluding
> > + * terminating '\0'.
> >   */
> >  static inline ssize_t
> >  cpumap_print_bitmask_to_buf(char *buf, const struct cpumask *mask,
> >  		loff_t off, size_t count)
> >  {
> >  	return bitmap_print_bitmask_to_buf(buf, cpumask_bits(mask),
> > -				   nr_cpu_ids, off, count);
> > +				   nr_cpu_ids, off, count) - 1;
> >  }
> > 
> >  /**
> > @@ -1018,7 +1019,7 @@ cpumap_print_list_to_buf(char *buf, const struct cpumask
> > *mask,
> >  		loff_t off, size_t count)
> >  {
> >  	return bitmap_print_list_to_buf(buf, cpumask_bits(mask),
> > -				   nr_cpu_ids, off, count);
> > +				   nr_cpu_ids, off, count) - 1;
> >  }
> > 
> >  #if NR_CPUS <= BITS_PER_LONG
> > --
> > 2.33.0
> 
> Thanks
> Barry