| Message ID | 20210818050841.2226600-3-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add __alloc_size() for better bounds checking | expand |
On Tue, 2021-08-17 at 22:08 -0700, Kees Cook wrote: > As already done in GrapheneOS, add the __alloc_size attribute for > regular kmalloc interfaces, to provide additional hinting for better > bounds checking, assisting CONFIG_FORTIFY_SOURCE and other compiler > optimizations. [] > diff --git a/include/linux/slab.h b/include/linux/slab.h [] > @@ -181,7 +181,7 @@ int kmem_cache_shrink(struct kmem_cache *); > /* > * Common kmalloc functions provided by all allocators > */ > -void * __must_check krealloc(const void *, size_t, gfp_t); > +void * __must_check krealloc(const void *, size_t, gfp_t) __alloc_size(2); I suggest the __alloc_size attribute be placed at the beginning of the function declaration to be more similar to the common __printf attribute location uses. __alloc_size(2) void * __must_check krealloc(const void *, size_t, gfp_t); I really prefer the __must_check to be with the other attribute and that function declarations have argument names too like: __alloc_size(2) __must_check void *krealloc(const void *ptr, size_t size, gfp_t gfp); but there are a _lot_ of placement of __must_check after the return type Lastly __alloc_size should probably be added to checkpatch Maybe: --- scripts/checkpatch.pl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 161ce7fe5d1e5..1a166b5cf3447 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -489,7 +489,8 @@ our $Attribute = qr{ ____cacheline_aligned| ____cacheline_aligned_in_smp| ____cacheline_internodealigned_in_smp| - __weak + __weak| + __alloc_size\s*\(\s*\d+\s*(?:,\s*d+\s*){0,5}\) }x; our $Modifier; our $Inline = qr{inline|__always_inline|noinline|__inline|__inline__};
On Tue, Aug 17, 2021 at 10:31:32PM -0700, Joe Perches wrote: > On Tue, 2021-08-17 at 22:08 -0700, Kees Cook wrote: > > As already done in GrapheneOS, add the __alloc_size attribute for > > regular kmalloc interfaces, to provide additional hinting for better > > bounds checking, assisting CONFIG_FORTIFY_SOURCE and other compiler > > optimizations. > [] > > diff --git a/include/linux/slab.h b/include/linux/slab.h > [] > > @@ -181,7 +181,7 @@ int kmem_cache_shrink(struct kmem_cache *); > > /* > > * Common kmalloc functions provided by all allocators > > */ > > -void * __must_check krealloc(const void *, size_t, gfp_t); > > +void * __must_check krealloc(const void *, size_t, gfp_t) __alloc_size(2); > > I suggest the __alloc_size attribute be placed at the beginning of the > function declaration to be more similar to the common __printf attribute > location uses. Yeah, any consistent ordering suggestions are welcome here; thank you! These declarations were all over the place, and trying to follow each slightly different existing style made my eyes hurt. :) > __alloc_size(2) > void * __must_check krealloc(const void *, size_t, gfp_t); > > I really prefer the __must_check to be with the other attribute and that > function declarations have argument names too like: > > __alloc_size(2) __must_check > void *krealloc(const void *ptr, size_t size, gfp_t gfp); I'm happy with whatever makes the most sense. > but there are a _lot_ of placement of __must_check after the return type > > Lastly __alloc_size should probably be added to checkpatch Oh, yes! Thanks for the reminder. > Maybe: > --- > scripts/checkpatch.pl | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 161ce7fe5d1e5..1a166b5cf3447 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -489,7 +489,8 @@ our $Attribute = qr{ > ____cacheline_aligned| > ____cacheline_aligned_in_smp| > ____cacheline_internodealigned_in_smp| > - __weak > + __weak| > + __alloc_size\s*\(\s*\d+\s*(?:,\s*d+\s*){0,5}\) Why the "{0,5}" bit here? I was expecting just "?". (i.e. it can have either 1 or 2 arguments.) > }x; > our $Modifier; > our $Inline = qr{inline|__always_inline|noinline|__inline|__inline__}; > >
On Tue, 2021-08-17 at 23:16 -0700, Kees Cook wrote: > On Tue, Aug 17, 2021 at 10:31:32PM -0700, Joe Perches wrote: > > On Tue, 2021-08-17 at 22:08 -0700, Kees Cook wrote: > > > As already done in GrapheneOS, add the __alloc_size attribute for > > > regular kmalloc interfaces, to provide additional hinting for better > > > bounds checking, assisting CONFIG_FORTIFY_SOURCE and other compiler > > > optimizations. [] > > Lastly __alloc_size should probably be added to checkpatch > > Oh, yes! Thanks for the reminder. > > > Maybe: > > --- > > scripts/checkpatch.pl | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > index 161ce7fe5d1e5..1a166b5cf3447 100755 > > --- a/scripts/checkpatch.pl > > +++ b/scripts/checkpatch.pl > > @@ -489,7 +489,8 @@ our $Attribute = qr{ > > ____cacheline_aligned| > > ____cacheline_aligned_in_smp| > > ____cacheline_internodealigned_in_smp| > > - __weak > > + __weak| > > + __alloc_size\s*\(\s*\d+\s*(?:,\s*d+\s*){0,5}\) > > Why the "{0,5}" bit here? I was expecting just "?". (i.e. it can have > either 1 or 2 arguments.) You are right. I misread the doc. I also missed a \ before the last d. So that last added line should maybe be: (totally untested btw) + __alloc_size\s*\(\s*\d+\s*(?:,\s*\d+\s*)?\)
On Tue, Aug 17, 2021 at 10:31:32PM -0700, Joe Perches wrote: > Lastly __alloc_size should probably be added to checkpatch > > Maybe: > --- > scripts/checkpatch.pl | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 161ce7fe5d1e5..1a166b5cf3447 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -489,7 +489,8 @@ our $Attribute = qr{ > ____cacheline_aligned| > ____cacheline_aligned_in_smp| > ____cacheline_internodealigned_in_smp| > - __weak > + __weak| > + __alloc_size\s*\(\s*\d+\s*(?:,\s*d+\s*){0,5}\) Should probably be added to kernel-doc as well. Any other awful regexes that need to be changed to understand it? And can we commonise the regexes that do exist into a perl helper library?
On Thu, 2021-08-19 at 01:27 +0100, Matthew Wilcox wrote: > On Tue, Aug 17, 2021 at 10:31:32PM -0700, Joe Perches wrote: > > Lastly __alloc_size should probably be added to checkpatch > > > > Maybe: > > --- > > scripts/checkpatch.pl | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl [] > > @@ -489,7 +489,8 @@ our $Attribute = qr{ > > ____cacheline_aligned| > > ____cacheline_aligned_in_smp| > > ____cacheline_internodealigned_in_smp| > > - __weak > > + __weak| > > + __alloc_size\s*\(\s*\d+\s*(?:,\s*d+\s*){0,5}\) > > Should probably be added to kernel-doc as well. Any other awful regexes > that need to be changed to understand it? And can we commonise the > regexes that do exist into a perl helper library? probably, but there would need to be some library work done and changes made to both utilities so they could use the same $helpers. And there are several nominally incomplete regexes already in kernel-doc and I'm not at all familiar with kernel-doc. e.g.: kernel-doc has: my $attribute = qr{__attribute__\s*\(\([a-z0-9,_\*\s\(\)]*\)\)}i; but __attribute__ can have quotes like: __attribute__((section("foo"))) and spaces around and and I believe between (( and )) like: __attribute__ ((packed)) so those wouldn't match. The use of parentheses internal to attributes like __align__(8) may not work particularly well either given greedy matching.
On Wed, Aug 18, 2021 at 06:10:57PM -0700, Joe Perches wrote: > On Thu, 2021-08-19 at 01:27 +0100, Matthew Wilcox wrote: > > On Tue, Aug 17, 2021 at 10:31:32PM -0700, Joe Perches wrote: > > > Lastly __alloc_size should probably be added to checkpatch > > > > > > Maybe: > > > --- > > > scripts/checkpatch.pl | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > [] > > > @@ -489,7 +489,8 @@ our $Attribute = qr{ > > > ____cacheline_aligned| > > > ____cacheline_aligned_in_smp| > > > ____cacheline_internodealigned_in_smp| > > > - __weak > > > + __weak| > > > + __alloc_size\s*\(\s*\d+\s*(?:,\s*d+\s*){0,5}\) > > > > Should probably be added to kernel-doc as well. Any other awful regexes > > that need to be changed to understand it? And can we commonise the > > regexes that do exist into a perl helper library? > > probably, but there would need to be some library work done and > changes made to both utilities so they could use the same $helpers. > > And there are several nominally incomplete regexes already in > kernel-doc and I'm not at all familiar with kernel-doc. Yes, kernel-doc is an awful example of perl gone wild.
On Thu, 2021-08-19 at 03:16 +0100, Matthew Wilcox wrote:
> kernel-doc is an awful example of perl gone wild.
And checkpatch isn't?
diff --git a/include/linux/slab.h b/include/linux/slab.h index c0d46b6fa12a..b2181c176999 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -181,7 +181,7 @@ int kmem_cache_shrink(struct kmem_cache *); /* * Common kmalloc functions provided by all allocators */ -void * __must_check krealloc(const void *, size_t, gfp_t); +void * __must_check krealloc(const void *, size_t, gfp_t) __alloc_size(2); void kfree(const void *); void kfree_sensitive(const void *); size_t __ksize(const void *); @@ -425,7 +425,7 @@ static __always_inline unsigned int __kmalloc_index(size_t size, #define kmalloc_index(s) __kmalloc_index(s, true) #endif /* !CONFIG_SLOB */ -void *__kmalloc(size_t size, gfp_t flags) __assume_kmalloc_alignment __malloc; +void *__kmalloc(size_t size, gfp_t flags) __alloc_size(1) __assume_kmalloc_alignment __malloc; void *kmem_cache_alloc(struct kmem_cache *, gfp_t flags) __assume_slab_alignment __malloc; void kmem_cache_free(struct kmem_cache *, void *); @@ -449,7 +449,8 @@ static __always_inline void kfree_bulk(size_t size, void **p) } #ifdef CONFIG_NUMA -void *__kmalloc_node(size_t size, gfp_t flags, int node) __assume_kmalloc_alignment __malloc; +void *__kmalloc_node(size_t size, gfp_t flags, int node) __alloc_size(1) + __assume_kmalloc_alignment __malloc; void *kmem_cache_alloc_node(struct kmem_cache *, gfp_t flags, int node) __assume_slab_alignment __malloc; #else static __always_inline void *__kmalloc_node(size_t size, gfp_t flags, int node) @@ -574,7 +575,7 @@ static __always_inline void *kmalloc_large(size_t size, gfp_t flags) * Try really hard to succeed the allocation but fail * eventually. */ -static __always_inline void *kmalloc(size_t size, gfp_t flags) +static __always_inline __alloc_size(1) void *kmalloc(size_t size, gfp_t flags) { if (__builtin_constant_p(size)) { #ifndef CONFIG_SLOB @@ -596,7 +597,8 @@ static __always_inline void *kmalloc(size_t size, gfp_t flags) return __kmalloc(size, flags); } -static __always_inline void *kmalloc_node(size_t size, gfp_t flags, int node) +static __always_inline __alloc_size(1) void * +kmalloc_node(size_t size, gfp_t flags, int node) { #ifndef CONFIG_SLOB if (__builtin_constant_p(size) && @@ -620,7 +622,8 @@ static __always_inline void *kmalloc_node(size_t size, gfp_t flags, int node) * @size: element size. * @flags: the type of memory to allocate (see kmalloc). */ -static inline void *kmalloc_array(size_t n, size_t size, gfp_t flags) +static inline __alloc_size(1, 2) void * +kmalloc_array(size_t n, size_t size, gfp_t flags) { size_t bytes; @@ -638,7 +641,7 @@ static inline void *kmalloc_array(size_t n, size_t size, gfp_t flags) * @new_size: new size of a single member of the array * @flags: the type of memory to allocate (see kmalloc) */ -static __must_check inline void * +static __must_check inline __alloc_size(2, 3) void * krealloc_array(void *p, size_t new_n, size_t new_size, gfp_t flags) { size_t bytes; @@ -655,7 +658,8 @@ krealloc_array(void *p, size_t new_n, size_t new_size, gfp_t flags) * @size: element size. * @flags: the type of memory to allocate (see kmalloc). */ -static inline void *kcalloc(size_t n, size_t size, gfp_t flags) +static inline __alloc_size(1, 2) void * +kcalloc(size_t n, size_t size, gfp_t flags) { return kmalloc_array(n, size, flags | __GFP_ZERO); } @@ -684,7 +688,8 @@ static inline void *kmalloc_array_node(size_t n, size_t size, gfp_t flags, return __kmalloc_node(bytes, flags, node); } -static inline void *kcalloc_node(size_t n, size_t size, gfp_t flags, int node) +static inline __alloc_size(1, 2) void * +kcalloc_node(size_t n, size_t size, gfp_t flags, int node) { return kmalloc_array_node(n, size, flags | __GFP_ZERO, node); } @@ -716,7 +721,8 @@ static inline void *kmem_cache_zalloc(struct kmem_cache *k, gfp_t flags) * @size: how many bytes of memory are required. * @flags: the type of memory to allocate (see kmalloc). */ -static inline void *kzalloc(size_t size, gfp_t flags) +static inline __alloc_size(1) void * +kzalloc(size_t size, gfp_t flags) { return kmalloc(size, flags | __GFP_ZERO); } @@ -727,26 +733,31 @@ static inline void *kzalloc(size_t size, gfp_t flags) * @flags: the type of memory to allocate (see kmalloc). * @node: memory node from which to allocate */ -static inline void *kzalloc_node(size_t size, gfp_t flags, int node) +static inline __alloc_size(1) void * +kzalloc_node(size_t size, gfp_t flags, int node) { return kmalloc_node(size, flags | __GFP_ZERO, node); } -extern void *kvmalloc_node(size_t size, gfp_t flags, int node); -static inline void *kvmalloc(size_t size, gfp_t flags) +extern __alloc_size(1) void * +kvmalloc_node(size_t size, gfp_t flags, int node); +static inline __alloc_size(1) void *kvmalloc(size_t size, gfp_t flags) { return kvmalloc_node(size, flags, NUMA_NO_NODE); } -static inline void *kvzalloc_node(size_t size, gfp_t flags, int node) +static inline __alloc_size(1) void * +kvzalloc_node(size_t size, gfp_t flags, int node) { return kvmalloc_node(size, flags | __GFP_ZERO, node); } -static inline void *kvzalloc(size_t size, gfp_t flags) +static inline __alloc_size(1) void * +kvzalloc(size_t size, gfp_t flags) { return kvmalloc(size, flags | __GFP_ZERO); } -static inline void *kvmalloc_array(size_t n, size_t size, gfp_t flags) +static inline __alloc_size(1, 2) void * +kvmalloc_array(size_t n, size_t size, gfp_t flags) { size_t bytes; @@ -756,13 +767,14 @@ static inline void *kvmalloc_array(size_t n, size_t size, gfp_t flags) return kvmalloc(bytes, flags); } -static inline void *kvcalloc(size_t n, size_t size, gfp_t flags) +static inline __alloc_size(1, 2) void * +kvcalloc(size_t n, size_t size, gfp_t flags) { return kvmalloc_array(n, size, flags | __GFP_ZERO); } -extern void *kvrealloc(const void *p, size_t oldsize, size_t newsize, - gfp_t flags); +extern __alloc_size(3) void * +kvrealloc(const void *p, size_t oldsize, size_t newsize, gfp_t flags); extern void kvfree(const void *addr); extern void kvfree_sensitive(const void *addr, size_t len);