| Message ID | 20180214201154.10186-3-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2018-02-14 at 12:11 -0800, Matthew Wilcox wrote: > From: Matthew Wilcox <mawilcox@microsoft.com> > > We have kvmalloc_array in order to safely allocate an array with a > number of elements specified by userspace (avoiding arithmetic overflow > leading to a buffer overrun). But it's fairly common to have a header > in front of that array (eg specifying the length of the array), so we > need a helper function for that situation. > > kvmalloc_ab_c() is the workhorse that does the calculation, but in spite > of our best efforts to name the arguments, it's really hard to remember > which order to put the arguments in. kvzalloc_struct() eliminates that > effort; you tell it about the struct you're allocating, and it puts the > arguments in the right order for you (and checks that the arguments > you've given are at least plausible). > > For comparison between the three schemes: > > sev = kvzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems, > GFP_KERNEL); > sev = kvzalloc_ab_c(elems, sizeof(struct v4l2_kevent), sizeof(*sev), > GFP_KERNEL); > sev = kvzalloc_struct(sev, events, elems, GFP_KERNEL); Perhaps kv[zm]alloc_buf_and_array is better naming.
On Wed, Feb 14, 2018 at 12:45:52PM -0800, Joe Perches wrote: > On Wed, 2018-02-14 at 12:11 -0800, Matthew Wilcox wrote: > > We have kvmalloc_array in order to safely allocate an array with a > > number of elements specified by userspace (avoiding arithmetic overflow > > leading to a buffer overrun). But it's fairly common to have a header > > in front of that array (eg specifying the length of the array), so we > > need a helper function for that situation. > > > > kvmalloc_ab_c() is the workhorse that does the calculation, but in spite > > of our best efforts to name the arguments, it's really hard to remember > > which order to put the arguments in. kvzalloc_struct() eliminates that > > effort; you tell it about the struct you're allocating, and it puts the > > arguments in the right order for you (and checks that the arguments > > you've given are at least plausible). > > > > For comparison between the three schemes: > > > > sev = kvzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems, > > GFP_KERNEL); > > sev = kvzalloc_ab_c(elems, sizeof(struct v4l2_kevent), sizeof(*sev), > > GFP_KERNEL); > > sev = kvzalloc_struct(sev, events, elems, GFP_KERNEL); > > Perhaps kv[zm]alloc_buf_and_array is better naming. I think that's actively misleading. The programmer isn't allocating a buf, they're allocating a struct. kvzalloc_hdr_arr was the earlier name, and that made some sense; they're allocating an array with a header. But nobody thinks about it like that; they're allocating a structure with a variably sized array at the end of it. If C macros had decent introspection, I'd like it to be: sev = kvzalloc_struct(elems, GFP_KERNEL); and have the macro examine the structure pointed to by 'sev', check the last element was an array, calculate the size of the array element, and call kvzalloc_ab_c. But we don't live in that world, so I have to get the programmer to tell me the structure and the name of the last element in it.
On Wed, 2018-02-14 at 13:12 -0800, Matthew Wilcox wrote: > On Wed, Feb 14, 2018 at 12:45:52PM -0800, Joe Perches wrote: > > On Wed, 2018-02-14 at 12:11 -0800, Matthew Wilcox wrote: > > > We have kvmalloc_array in order to safely allocate an array with a > > > number of elements specified by userspace (avoiding arithmetic overflow > > > leading to a buffer overrun). But it's fairly common to have a header > > > in front of that array (eg specifying the length of the array), so we > > > need a helper function for that situation. > > > > > > kvmalloc_ab_c() is the workhorse that does the calculation, but in spite > > > of our best efforts to name the arguments, it's really hard to remember > > > which order to put the arguments in. kvzalloc_struct() eliminates that > > > effort; you tell it about the struct you're allocating, and it puts the > > > arguments in the right order for you (and checks that the arguments > > > you've given are at least plausible). > > > > > > For comparison between the three schemes: > > > > > > sev = kvzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems, > > > GFP_KERNEL); > > > sev = kvzalloc_ab_c(elems, sizeof(struct v4l2_kevent), sizeof(*sev), > > > GFP_KERNEL); > > > sev = kvzalloc_struct(sev, events, elems, GFP_KERNEL); > > > > Perhaps kv[zm]alloc_buf_and_array is better naming. > > I think that's actively misleading. The programmer isn't allocating a > buf, they're allocating a struct. kvzalloc_hdr_arr was the earlier name, > and that made some sense; they're allocating an array with a header. > But nobody thinks about it like that; they're allocating a structure > with a variably sized array at the end of it. > > If C macros had decent introspection, I'd like it to be: > > sev = kvzalloc_struct(elems, GFP_KERNEL); > > and have the macro examine the structure pointed to by 'sev', check > the last element was an array, calculate the size of the array element, > and call kvzalloc_ab_c. But we don't live in that world, so I have to > get the programmer to tell me the structure and the name of the last > element in it. Look at your patch 4 - dev_dax = kzalloc(sizeof(*dev_dax) + sizeof(*res) * count, GFP_KERNEL); + dev_dax = kvzalloc_struct(dev_dax, res, count, GFP_KERNEL); Here what is being allocated is exactly a struct and an array. And this doesn't compile either.
On Wed, 2018-02-14 at 13:24 -0800, Joe Perches wrote: > Look at your patch 4 > > - dev_dax = kzalloc(sizeof(*dev_dax) + sizeof(*res) * count, GFP_KERNEL); > + dev_dax = kvzalloc_struct(dev_dax, res, count, GFP_KERNEL); > > Here what is being allocated is exactly a struct > and an array. > > And this doesn't compile either. apologies, my mistake.
On Wed, Feb 14, 2018 at 01:24:09PM -0800, Joe Perches wrote: > On Wed, 2018-02-14 at 13:12 -0800, Matthew Wilcox wrote: > > On Wed, Feb 14, 2018 at 12:45:52PM -0800, Joe Perches wrote: > > > Perhaps kv[zm]alloc_buf_and_array is better naming. > > > > I think that's actively misleading. The programmer isn't allocating a > > buf, they're allocating a struct. kvzalloc_hdr_arr was the earlier name, > > and that made some sense; they're allocating an array with a header. > > But nobody thinks about it like that; they're allocating a structure > > with a variably sized array at the end of it. > > > > If C macros had decent introspection, I'd like it to be: > > > > sev = kvzalloc_struct(elems, GFP_KERNEL); > > > > and have the macro examine the structure pointed to by 'sev', check > > the last element was an array, calculate the size of the array element, > > and call kvzalloc_ab_c. But we don't live in that world, so I have to > > get the programmer to tell me the structure and the name of the last > > element in it. > > Look at your patch 4 > > - dev_dax = kzalloc(sizeof(*dev_dax) + sizeof(*res) * count, GFP_KERNEL); > + dev_dax = kvzalloc_struct(dev_dax, res, count, GFP_KERNEL); > > Here what is being allocated is exactly a struct > and an array. No, it's a struct *containing* an array. Look at patches 5 & 8 where I have to convert the structs to contain the array which was silently being allocated immediately after them. > And this doesn't compile either. Does for me. What error are you seeing?
On Wed, 14 Feb 2018 13:12:03 -0800 Matthew Wilcox <willy@infradead.org> wrote: > On Wed, Feb 14, 2018 at 12:45:52PM -0800, Joe Perches wrote: > > On Wed, 2018-02-14 at 12:11 -0800, Matthew Wilcox wrote: > > > We have kvmalloc_array in order to safely allocate an array with a > > > number of elements specified by userspace (avoiding arithmetic overflow > > > leading to a buffer overrun). But it's fairly common to have a header > > > in front of that array (eg specifying the length of the array), so we > > > need a helper function for that situation. > > > > > > kvmalloc_ab_c() is the workhorse that does the calculation, but in spite > > > of our best efforts to name the arguments, it's really hard to remember > > > which order to put the arguments in. kvzalloc_struct() eliminates that > > > effort; you tell it about the struct you're allocating, and it puts the > > > arguments in the right order for you (and checks that the arguments > > > you've given are at least plausible). > > > > > > For comparison between the three schemes: > > > > > > sev = kvzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems, > > > GFP_KERNEL); > > > sev = kvzalloc_ab_c(elems, sizeof(struct v4l2_kevent), sizeof(*sev), > > > GFP_KERNEL); > > > sev = kvzalloc_struct(sev, events, elems, GFP_KERNEL); > > > > Perhaps kv[zm]alloc_buf_and_array is better naming. > > I think that's actively misleading. The programmer isn't allocating a > buf, they're allocating a struct. kvzalloc_hdr_arr was the earlier name, > and that made some sense; they're allocating an array with a header. > But nobody thinks about it like that; they're allocating a structure > with a variably sized array at the end of it. > > If C macros had decent introspection, I'd like it to be: > > sev = kvzalloc_struct(elems, GFP_KERNEL); > > and have the macro examine the structure pointed to by 'sev', check > the last element was an array, calculate the size of the array element, > and call kvzalloc_ab_c. But we don't live in that world, so I have to > get the programmer to tell me the structure and the name of the last > element in it. hm, bikeshedding fun. struct foo { whatevs; struct bar[0]; } struct foo *a_foo; a_foo = kvzalloc_struct_buf(foo, bar, nr_bars); and macro magic will insert the `struct' keyword. This will help to force a miscompile if inappropriate types are used for foo and bar. Problem is, foo may be a union(?) and bar may be a scalar type. So a_foo = kvzalloc_struct_buf(struct foo, struct bar, nr_bars); or, of course. a_foo = kvzalloc_struct_buf(typeof(*a_foo), typeof(a_foo->bar[0]), nr_bars); or whatever. The basic idea is to use the wrapper macros to force compile errors if these things are misused. Also, > +/** > + * kvmalloc_ab_c() - Allocate (a * b + c) bytes of memory. > + * @n: Number of elements. > + * @size: Size of each element (should be constant). > + * @c: Size of header (should be constant). > + * @gfp: Memory allocation flags. > + * > + * Use this function to allocate @n * @size + @c bytes of memory. This > + * function is safe to use when @n is controlled from userspace; it will > + * return %NULL if the required amount of memory cannot be allocated. > + * Use kvfree() to free the allocated memory. > + * > + * The kvzalloc_struct() function is easier to use as it has typechecking > + * and you do not need to remember which of the arguments should be constants. > + * > + * Context: Process context. May sleep; the @gfp flags should be based on > + * %GFP_KERNEL. > + * Return: A pointer to the allocated memory or %NULL. > + */ > +static inline __must_check > +void *kvmalloc_ab_c(size_t n, size_t size, size_t c, gfp_t gfp) > +{ > + if (size != 0 && n > (SIZE_MAX - c) / size) > + return NULL; > + > + return kvmalloc(n * size + c, gfp); > +} Can we please avoid the single-char identifiers? void *kvmalloc_ab_c(size_t n_elems, size_t elem_size, size_t header_size, gfp_t gfp); makes the code so much more readable.
diff --git a/include/linux/mm.h b/include/linux/mm.h index 81bd7f0be286..3b07ba12c8cc 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -557,6 +557,57 @@ static inline void *kvmalloc_array(size_t n, size_t size, gfp_t flags) return kvmalloc(n * size, flags); } +/** + * kvmalloc_ab_c() - Allocate (a * b + c) bytes of memory. + * @n: Number of elements. + * @size: Size of each element (should be constant). + * @c: Size of header (should be constant). + * @gfp: Memory allocation flags. + * + * Use this function to allocate @n * @size + @c bytes of memory. This + * function is safe to use when @n is controlled from userspace; it will + * return %NULL if the required amount of memory cannot be allocated. + * Use kvfree() to free the allocated memory. + * + * The kvzalloc_struct() function is easier to use as it has typechecking + * and you do not need to remember which of the arguments should be constants. + * + * Context: Process context. May sleep; the @gfp flags should be based on + * %GFP_KERNEL. + * Return: A pointer to the allocated memory or %NULL. + */ +static inline __must_check +void *kvmalloc_ab_c(size_t n, size_t size, size_t c, gfp_t gfp) +{ + if (size != 0 && n > (SIZE_MAX - c) / size) + return NULL; + + return kvmalloc(n * size + c, gfp); +} +#define kvzalloc_ab_c(a, b, c, gfp) kvmalloc_ab_c(a, b, c, (gfp) | __GFP_ZERO) + +/** + * kvzalloc_struct() - Allocate and zero-fill a structure containing a + * variable length array. + * @p: Pointer to the structure. + * @member: Name of the array member. + * @n: Number of elements in the array. + * @gfp: Memory allocation flags. + * + * Allocate (and zero-fill) enough memory for a structure with an array + * of @n elements. This function is safe to use when @n is specified by + * userspace as the arithmetic will not overflow. + * Use kvfree() to free the allocated memory. + * + * Context: Process context. May sleep; the @gfp flags should be based on + * %GFP_KERNEL. + * Return: Zero-filled memory or a NULL pointer. + */ +#define kvzalloc_struct(p, member, n, gfp) \ + (typeof(p))kvzalloc_ab_c(n, \ + sizeof(*(p)->member) + __must_be_array((p)->member), \ + offsetof(typeof(*(p)), member), gfp) + extern void kvfree(const void *addr); static inline atomic_t *compound_mapcount_ptr(struct page *page)