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

[tiantao (H)]

在 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 ?


> > > > +       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;
> > > > +}
>
> --
> With Best Regards,
> Andy Shevchenko
> .