Messages in this thread | | | Date | Wed, 2 Jun 2021 11:06:21 +0200 | From | Greg KH <> | 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@huawei.com> 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?
thanks,
greg k-h
| |