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

[Greg KH]

On Wed, Jun 02, 2021 at 09:20:45AM +0000, Song Bao Hua (Barry Song) wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@xxxxxxxxxxxxxxxxxxx]
> > Sent: Wednesday, June 2, 2021 9:06 PM
> > To: tiantao (H) <tiantao6@xxxxxxxxxxxxx>
> > Cc: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>; tiantao (H)
> > <tiantao6@xxxxxxxxxxxxx>; Linux Kernel Mailing List
> > <linux-kernel@xxxxxxxxxxxxxxx>; Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>;
> > Song Bao Hua (Barry Song) <song.bao.hua@xxxxxxxxxxxxx>; Andy Shevchenko
> > <andriy.shevchenko@xxxxxxxxxxxxxxx>; Rafael J. Wysocki <rafael@xxxxxxxxxx>;
> > Jonathan Cameron <jonathan.cameron@xxxxxxxxxx>
> > Subject: Re: [PATCH 1/2] topology: use bin_attribute to avoid buff overflow
> > 
> > On Wed, Jun 02, 2021 at 05:00:16PM +0800, tiantao (H) wrote:
> > >
> > > 在 2021/6/2 16:48, Andy Shevchenko 写道:
> > > > On Wed, Jun 2, 2021 at 9:45 AM tiantao (H) <tiantao6@xxxxxxxxxx> wrote:
> > > > > 在 2021/6/2 14:18, Greg KH 写道:
> > > > > > On Wed, Jun 02, 2021 at 02:14:49PM +0800, tiantao (H) wrote:
> > > > > > > 在 2021/6/1 12:58, Greg KH 写道:
> > > > > > > > On Tue, Jun 01, 2021 at 10:56:49AM +0800, Tian Tao wrote:
> > > > ...
> > > >
> > > > > > > > >     /**
> > > > > > > > > + * bitmap_print_to_buf - convert bitmap to list or hex format
> > ASCII string
> > > > > > > > > + * @list: indicates whether the bitmap must be list
> > > > > > > > > + * @buf: page aligned buffer into which string is placed
> > > > > > > > > + * @maskp: pointer to bitmap to convert
> > > > > > > > > + * @nmaskbits: size of bitmap, in bits
> > > > > > > > > + * @off: offset in buf
> > > > > > > > > + * @count: count that already output
> > > > > > > > > + *
> > > > > > > > > + * the role of bitmap_print_to_buf and bitmap_print_to_pagebuf
> > is
> > > > > > > > > + * the same, the difference is that the second parameter 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)
> > > > > > > > > +{
> > > > > > > > > +  int len, size;
> > > > > > > > > +  void *data;
> > > > > > > > > +  char *fmt = list ? "%*pbl\n" : "%*pb\n";
> > > > > > > > > +
> > > > > > > > > +  len = snprintf(NULL, 0, fmt, nmaskbits, maskp);
> > > > > > > > > +
> > > > > > > > > +  data = kvmalloc(len+1, GFP_KERNEL);
> > > > > > > > > +  if (!data)
> > > > > > > > > +          return -ENOMEM;
> > > > > > > > > +
> > > > > > > > > +  size = scnprintf(data, len+1, fmt, nmaskbits, maskp);
> > > > > > > > > +  size = memory_read_from_buffer(buf, count, &off, data, size);
> > > > > > > > > +  kvfree(data);
> > > > > > > > > +
> > > > > > > > > +  return size;
> > > > > > > > Why is this so different from bitmap_print_to_pagebuf()?  Can't you
> > just
> > > > > > > > use this function as the "real" function and then change
> > > > > > > > bitmap_print_to_pagebuf() to call it with a size of PAGE_SIZE?
> > > > > > > Do you mean do following change, is that correct? :-)
> > > > > > Maybe, it is whitespace corrupted, and it still feels like this function
> > > > > > is much bigger than it needs to be given the function it is replacing
> > is
> > > > > > only a simple sprintf() call.
> > > > > >
> > > > > > > +int bitmap_print_to_buf(bool list, char *buf, const unsigned long
> > *maskp,
> > > > > > > +                       int nmaskbits, loff_t off, size_t count)
> > > > > > > +{
> > > > > > > +       int len, size;
> > > > > > > +       void *data;
> > > > > > > +       const char *fmt = list ? "%*pbl\n" : "%*pb\n";
> > > > > > > +
> > > > > > > +       if (off == LLONG_MAX && count == PAGE_SIZE - offset_in_page(buf))
> > > > > > > +               return scnprintf(buf, count, fmt, nmaskbits, maskp);
> > > > > > > +
> > > > > > > +       len = snprintf(NULL, 0, fmt, nmaskbits, maskp);
> > > > > > > +
> > > > > > > +       data = kvmalloc(len+1, GFP_KERNEL);
> > > > > > Why do you need to allocate more memory?  And why kvmalloc()?
> > > > > Because the memory here will exceed a pagesize and we don't know the
> > > > > exact size, we have to call
> > > > >
> > > > > snprintf first to get the actual size. kvmalloc() is used because when
> > > > > physical memory is tight, kmalloc
> > > > >
> > > > > may fail, but vmalloc will succeed. It is not so bad that the memory is
> > > > > not requested here.
> > > > To me it sounds like the function is overengineered / lacks thought
> > > > through / optimization.
> > > > Can you provide a few examples that require the above algorithm?
> > >
> > > so you think we should use kmalloc instead of kvmalloc ?
> > 
> > What size bitmap would trigger a vmalloc() call to be forced here?
> > 
> 
> According to kvmalloc_node(), only if size is larger than PAGE_SIZE,
> kvmalloc will move to vmalloc if kmalloc fails to get memory. Otherwise,
> it will return error.

I know, so how big of a bitmap would you need before a kmalloc call
would fail given that there is almost always free memory in these
"smaller" amounts.

And if there is no memory, why not fail like normal, this is not
something that needs to be complex and crazy, it's just a sysfs file to
provide information.

Please do not over-engineer something that does not need it, until you
have proof it is needed.

thanks,

greg k-h