All of lore.kernel.org
 help / color / mirror / Atom feed
* incoming
@ 2021-09-10  3:09 Andrew Morton
  2021-09-10  3:10 ` [patch 1/9] mm: move kvmalloc-related functions to slab.h Andrew Morton
                   ` (9 more replies)
  0 siblings, 10 replies; 49+ messages in thread
From: Andrew Morton @ 2021-09-10  3:09 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: linux-mm, mm-commits


More post linux-next material.

9 patches, based on f154c806676ad7153c6e161f30c53a44855329d6.

Subsystems affected by this patch series:

  mm/slab-generic
  rapidio
  mm/debug

Subsystem: mm/slab-generic

    "Matthew Wilcox (Oracle)" <willy@infradead.org>:
      mm: move kvmalloc-related functions to slab.h

Subsystem: rapidio

    Kees Cook <keescook@chromium.org>:
      rapidio: avoid bogus __alloc_size warning

Subsystem: mm/debug

    Kees Cook <keescook@chromium.org>:
    Patch series "Add __alloc_size() for better bounds checking", v2:
      Compiler Attributes: add __alloc_size() for better bounds checking
      checkpatch: add __alloc_size() to known $Attribute
      slab: clean up function declarations
      slab: add __alloc_size attributes for better bounds checking
      mm/page_alloc: add __alloc_size attributes for better bounds checking
      percpu: add __alloc_size attributes for better bounds checking
      mm/vmalloc: add __alloc_size attributes for better bounds checking

 Makefile                                 |   15 +++
 drivers/of/kexec.c                       |    1 
 drivers/rapidio/devices/rio_mport_cdev.c |    9 +-
 include/linux/compiler_attributes.h      |    6 +
 include/linux/gfp.h                      |    2 
 include/linux/mm.h                       |   34 --------
 include/linux/percpu.h                   |    3 
 include/linux/slab.h                     |  122 ++++++++++++++++++++++---------
 include/linux/vmalloc.h                  |   11 ++
 scripts/checkpatch.pl                    |    3 
 10 files changed, 132 insertions(+), 74 deletions(-)


^ permalink raw reply	[flat|nested] 49+ messages in thread

* [patch 1/9] mm: move kvmalloc-related functions to slab.h
  2021-09-10  3:09 incoming Andrew Morton
@ 2021-09-10  3:10 ` Andrew Morton
  2021-09-10  3:10 ` [patch 2/9] rapidio: avoid bogus __alloc_size warning Andrew Morton
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 49+ messages in thread
From: Andrew Morton @ 2021-09-10  3:10 UTC (permalink / raw)
  To: akpm, cl, iamjoonsoo.kim, linux-mm, mm-commits, penberg,
	rientjes, torvalds, vbabka, willy

From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
Subject: mm: move kvmalloc-related functions to slab.h

Not all files in the kernel should include mm.h.  Migrating callers from
kmalloc to kvmalloc is easier if the kvmalloc functions are in slab.h.

[akpm@linux-foundation.org: move the new kvrealloc() also]
Link: https://lkml.kernel.org/r/20210622215757.3525604-1-willy@infradead.org
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Acked-by: Pekka Enberg <penberg@kernel.org>
Cc: Christoph Lameter <cl@linux.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/of/kexec.c   |    1 +
 include/linux/mm.h   |   34 ----------------------------------
 include/linux/slab.h |   34 ++++++++++++++++++++++++++++++++++
 3 files changed, 35 insertions(+), 34 deletions(-)

--- a/drivers/of/kexec.c~mm-move-kvmalloc-related-functions-to-slabh
+++ a/drivers/of/kexec.c
@@ -16,6 +16,7 @@
 #include <linux/of.h>
 #include <linux/of_fdt.h>
 #include <linux/random.h>
+#include <linux/slab.h>
 #include <linux/types.h>
 
 #define RNG_SEED_SIZE		128
--- a/include/linux/mm.h~mm-move-kvmalloc-related-functions-to-slabh
+++ a/include/linux/mm.h
@@ -799,40 +799,6 @@ static inline int is_vmalloc_or_module_a
 }
 #endif
 
-extern void *kvmalloc_node(size_t size, gfp_t flags, int node);
-static inline 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)
-{
-	return kvmalloc_node(size, flags | __GFP_ZERO, node);
-}
-static inline 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)
-{
-	size_t bytes;
-
-	if (unlikely(check_mul_overflow(n, size, &bytes)))
-		return NULL;
-
-	return kvmalloc(bytes, flags);
-}
-
-static inline 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 void kvfree(const void *addr);
-extern void kvfree_sensitive(const void *addr, size_t len);
-
 static inline int head_compound_mapcount(struct page *head)
 {
 	return atomic_read(compound_mapcount_ptr(head)) + 1;
--- a/include/linux/slab.h~mm-move-kvmalloc-related-functions-to-slabh
+++ a/include/linux/slab.h
@@ -732,6 +732,40 @@ static inline void *kzalloc_node(size_t
 	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)
+{
+	return kvmalloc_node(size, flags, NUMA_NO_NODE);
+}
+static inline 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)
+{
+	return kvmalloc(size, flags | __GFP_ZERO);
+}
+
+static inline void *kvmalloc_array(size_t n, size_t size, gfp_t flags)
+{
+	size_t bytes;
+
+	if (unlikely(check_mul_overflow(n, size, &bytes)))
+		return NULL;
+
+	return kvmalloc(bytes, flags);
+}
+
+static inline 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 void kvfree(const void *addr);
+extern void kvfree_sensitive(const void *addr, size_t len);
+
 unsigned int kmem_cache_size(struct kmem_cache *s);
 void __init kmem_cache_init_late(void);
 
_

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [patch 2/9] rapidio: avoid bogus __alloc_size warning
  2021-09-10  3:09 incoming Andrew Morton
  2021-09-10  3:10 ` [patch 1/9] mm: move kvmalloc-related functions to slab.h Andrew Morton
@ 2021-09-10  3:10 ` Andrew Morton
  2021-09-10  3:10 ` [patch 3/9] Compiler Attributes: add __alloc_size() for better bounds checking Andrew Morton
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 49+ messages in thread
From: Andrew Morton @ 2021-09-10  3:10 UTC (permalink / raw)
  To: akpm, alex.bou9, gustavoars, ira.weiny, jhubbard, jingxiangfeng,
	jrdr.linux, keescook, linux-mm, lkp, mm-commits, mporter,
	torvalds

From: Kees Cook <keescook@chromium.org>
Subject: rapidio: avoid bogus __alloc_size warning

GCC 9.3 (but not later) incorrectly evaluates the arguments to
check_copy_size(), getting seemingly confused by the size being returned
from array_size().  Instead, perform the calculation once, which both
makes the code more readable and avoids the bug in GCC.

   In file included from arch/x86/include/asm/preempt.h:7,
                    from include/linux/preempt.h:78,
                    from include/linux/spinlock.h:55,
                    from include/linux/mm_types.h:9,
                    from include/linux/buildid.h:5,
                    from include/linux/module.h:14,
                    from drivers/rapidio/devices/rio_mport_cdev.c:13:
   In function 'check_copy_size',
       inlined from 'copy_from_user' at include/linux/uaccess.h:191:6,
       inlined from 'rio_mport_transfer_ioctl' at drivers/rapidio/devices/rio_mport_cdev.c:983:6:
   include/linux/thread_info.h:213:4: error: call to '__bad_copy_to' declared with attribute error: copy destination size is too small
     213 |    __bad_copy_to();
         |    ^~~~~~~~~~~~~~~

But the allocation size and the copy size are identical:

	transfer = vmalloc(array_size(sizeof(*transfer), transaction.count));
	if (!transfer)
		return -ENOMEM;

	if (unlikely(copy_from_user(transfer,
				    (void __user *)(uintptr_t)transaction.block,
				    array_size(sizeof(*transfer), transaction.count)))) {

Link: https://lkml.kernel.org/r/20210909161409.2250920-1-keescook@chromium.org
Link: https://lore.kernel.org/linux-mm/202109091134.FHnRmRxu-lkp@intel.com/
Signed-off-by: Kees Cook <keescook@chromium.org>
Reported-by: kernel test robot <lkp@intel.com>
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
Cc: Matt Porter <mporter@kernel.crashing.org>
Cc: Alexandre Bounine <alex.bou9@gmail.com>
Cc: Jing Xiangfeng <jingxiangfeng@huawei.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Souptick Joarder <jrdr.linux@gmail.com>
Cc: Gustavo A. R. Silva <gustavoars@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/rapidio/devices/rio_mport_cdev.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

--- a/drivers/rapidio/devices/rio_mport_cdev.c~rapidio-avoid-bogus-__alloc_size-warning
+++ a/drivers/rapidio/devices/rio_mport_cdev.c
@@ -965,6 +965,7 @@ static int rio_mport_transfer_ioctl(stru
 	struct rio_transfer_io *transfer;
 	enum dma_data_direction dir;
 	int i, ret = 0;
+	size_t size;
 
 	if (unlikely(copy_from_user(&transaction, arg, sizeof(transaction))))
 		return -EFAULT;
@@ -976,13 +977,14 @@ static int rio_mport_transfer_ioctl(stru
 	     priv->md->properties.transfer_mode) == 0)
 		return -ENODEV;
 
-	transfer = vmalloc(array_size(sizeof(*transfer), transaction.count));
+	size = array_size(sizeof(*transfer), transaction.count);
+	transfer = vmalloc(size);
 	if (!transfer)
 		return -ENOMEM;
 
 	if (unlikely(copy_from_user(transfer,
 				    (void __user *)(uintptr_t)transaction.block,
-				    array_size(sizeof(*transfer), transaction.count)))) {
+				    size))) {
 		ret = -EFAULT;
 		goto out_free;
 	}
@@ -994,8 +996,7 @@ static int rio_mport_transfer_ioctl(stru
 			transaction.sync, dir, &transfer[i]);
 
 	if (unlikely(copy_to_user((void __user *)(uintptr_t)transaction.block,
-				  transfer,
-				  array_size(sizeof(*transfer), transaction.count))))
+				  transfer, size)))
 		ret = -EFAULT;
 
 out_free:
_

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [patch 3/9] Compiler Attributes: add __alloc_size() for better bounds checking
  2021-09-10  3:09 incoming Andrew Morton
  2021-09-10  3:10 ` [patch 1/9] mm: move kvmalloc-related functions to slab.h Andrew Morton
  2021-09-10  3:10 ` [patch 2/9] rapidio: avoid bogus __alloc_size warning Andrew Morton
@ 2021-09-10  3:10 ` Andrew Morton
  2021-09-10  3:10 ` [patch 4/9] checkpatch: add __alloc_size() to known $Attribute Andrew Morton
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 49+ messages in thread
From: Andrew Morton @ 2021-09-10  3:10 UTC (permalink / raw)
  To: akpm, apw, cl, danielmicay, dennis, dwaipayanray1,
	iamjoonsoo.kim, joe, keescook, linux-mm, lukas.bulwahn,
	mm-commits, nathan, ndesaulniers, ojeda, penberg, rdunlap,
	rientjes, tj, torvalds, vbabka

From: Kees Cook <keescook@chromium.org>
Subject: Compiler Attributes: add __alloc_size() for better bounds checking

Patch series "Add __alloc_size() for better bounds checking", v2.

GCC and Clang both use the "alloc_size" attribute to assist with bounds
checking around the use of allocation functions.  Add the attribute,
adjust the Makefile to silence needless warnings, and add the hints to the
allocators where possible.  These changes have been in use for a while now
in GrapheneOS.


This patch (of 2):

GCC and Clang can use the "alloc_size" attribute to better inform the
results of __builtin_object_size() (for compile-time constant values). 
Clang can additionally use alloc_size to inform the results of
__builtin_dynamic_object_size() (for run-time values).

Because GCC sees the frequent use of struct_size() as an allocator size
argument, and notices it can return SIZE_MAX (the overflow indication), it
complains about these call sites may overflow (since SIZE_MAX is greater
than the default -Walloc-size-larger-than=PTRDIFF_MAX).  This isn't
helpful since we already know a SIZE_MAX will be caught at run-time (this
was an intentional design).  Instead, just disable this check as it is
both a false positive and redundant.  (Clang does not have this warning
option.)

[keescook@chromium.org: adjust warning logic for pre-9.1 gcc behaviors]
  Link: https://lkml.kernel.org/r/20210827151327.2729736-1-keescook@chromium.org
Link: https://lkml.kernel.org/r/20210818214021.2476230-1-keescook@chromium.org
Link: https://lkml.kernel.org/r/20210818214021.2476230-2-keescook@chromium.org
Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Miguel Ojeda <ojeda@kernel.org>
Reviewed-by: Nathan Chancellor <nathan@kernel.org>
Tested-by: Randy Dunlap <rdunlap@infradead.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Andy Whitcroft <apw@canonical.com>
Cc: Christoph Lameter <cl@linux.com>
Cc: Daniel Micay <danielmicay@gmail.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Dennis Zhou <dennis@kernel.org>
Cc: Dwaipayan Ray <dwaipayanray1@gmail.com>
Cc: Joe Perches <joe@perches.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 Makefile                            |   15 +++++++++++++++
 include/linux/compiler_attributes.h |    6 ++++++
 2 files changed, 21 insertions(+)

--- a/include/linux/compiler_attributes.h~compiler-attributes-add-__alloc_size-for-better-bounds-checking
+++ a/include/linux/compiler_attributes.h
@@ -54,6 +54,12 @@
 #define __aligned_largest               __attribute__((__aligned__))
 
 /*
+ *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-alloc_005fsize-function-attribute
+ * clang: https://clang.llvm.org/docs/AttributeReference.html#alloc-size
+ */
+#define __alloc_size(x, ...)		__attribute__((__alloc_size__(x, ## __VA_ARGS__)))
+
+/*
  * Note: users of __always_inline currently do not write "inline" themselves,
  * which seems to be required by gcc to apply the attribute according
  * to its docs (and also "warning: always_inline function might not be
--- a/Makefile~compiler-attributes-add-__alloc_size-for-better-bounds-checking
+++ a/Makefile
@@ -1014,6 +1014,21 @@ ifdef CONFIG_CC_IS_GCC
 KBUILD_CFLAGS += -Wno-maybe-uninitialized
 endif
 
+ifdef CONFIG_CC_IS_GCC
+# The allocators already balk at large sizes, so silence the compiler
+# warnings for bounds checks involving those possible values. While
+# -Wno-alloc-size-larger-than would normally be used here, earlier versions
+# of gcc (<9.1) weirdly don't handle the option correctly when _other_
+# warnings are produced (?!). Using -Walloc-size-larger-than=SIZE_MAX
+# doesn't work (as it is documented to), silently resolving to "0" prior to
+# version 9.1 (and producing an error more recently). Numeric values larger
+# than PTRDIFF_MAX also don't work prior to version 9.1, which are silently
+# ignored, continuing to default to PTRDIFF_MAX. So, left with no other
+# choice, we must perform a versioned check to disable this warning.
+# https://lore.kernel.org/lkml/20210824115859.187f272f@canb.auug.org.au
+KBUILD_CFLAGS += $(call cc-ifversion, -ge, 0901, -Wno-alloc-size-larger-than)
+endif
+
 # disable invalid "can't wrap" optimizations for signed / pointers
 KBUILD_CFLAGS	+= -fno-strict-overflow
 
_

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [patch 4/9] checkpatch: add __alloc_size() to known $Attribute
  2021-09-10  3:09 incoming Andrew Morton
                   ` (2 preceding siblings ...)
  2021-09-10  3:10 ` [patch 3/9] Compiler Attributes: add __alloc_size() for better bounds checking Andrew Morton
@ 2021-09-10  3:10 ` Andrew Morton
  2021-09-10  3:10 ` [patch 5/9] slab: clean up function declarations Andrew Morton
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 49+ messages in thread
From: Andrew Morton @ 2021-09-10  3:10 UTC (permalink / raw)
  To: akpm, apw, cl, danielmicay, dennis, dwaipayanray1,
	iamjoonsoo.kim, joe, keescook, linux-mm, lukas.bulwahn,
	mm-commits, nathan, ndesaulniers, ojeda, penberg, rientjes, tj,
	torvalds, vbabka

From: Kees Cook <keescook@chromium.org>
Subject: checkpatch: add __alloc_size() to known $Attribute

Make sure checkpatch.pl doesn't get confused about finding the
__alloc_size attribute on functions.

Link: https://lkml.kernel.org/r/20210818214021.2476230-3-keescook@chromium.org
Signed-off-by: Kees Cook <keescook@chromium.org>
Suggested-by: Joe Perches <joe@perches.com>
Cc: Andy Whitcroft <apw@canonical.com>
Cc: Dwaipayan Ray <dwaipayanray1@gmail.com>
Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Cc: Christoph Lameter <cl@linux.com>
Cc: Daniel Micay <danielmicay@gmail.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Dennis Zhou <dennis@kernel.org>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Miguel Ojeda <ojeda@kernel.org>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 scripts/checkpatch.pl |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- a/scripts/checkpatch.pl~checkpatch-add-__alloc_size-to-known-attribute
+++ a/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*)?\)
 		  }x;
 our $Modifier;
 our $Inline	= qr{inline|__always_inline|noinline|__inline|__inline__};
_

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [patch 5/9] slab: clean up function declarations
  2021-09-10  3:09 incoming Andrew Morton
                   ` (3 preceding siblings ...)
  2021-09-10  3:10 ` [patch 4/9] checkpatch: add __alloc_size() to known $Attribute Andrew Morton
@ 2021-09-10  3:10 ` Andrew Morton
  2021-09-10  3:10 ` [patch 6/9] slab: add __alloc_size attributes for better bounds checking Andrew Morton
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 49+ messages in thread
From: Andrew Morton @ 2021-09-10  3:10 UTC (permalink / raw)
  To: akpm, apw, cl, danielmicay, dennis, dwaipayanray1,
	iamjoonsoo.kim, joe, keescook, linux-mm, lukas.bulwahn,
	mm-commits, nathan, ndesaulniers, ojeda, penberg, rientjes, tj,
	torvalds, vbabka

From: Kees Cook <keescook@chromium.org>
Subject: slab: clean up function declarations

In order to have more readable and regular declarations, move __must_check
to the line above the main function declaration and add all the missing
parameter names.

Link: https://lkml.kernel.org/r/20210818214021.2476230-4-keescook@chromium.org
Signed-off-by: Kees Cook <keescook@chromium.org>
Suggested-by: Joe Perches <joe@perches.com>
Cc: Christoph Lameter <cl@linux.com>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Andy Whitcroft <apw@canonical.com>
Cc: Daniel Micay <danielmicay@gmail.com>
Cc: Dennis Zhou <dennis@kernel.org>
Cc: Dwaipayan Ray <dwaipayanray1@gmail.com>
Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Cc: Miguel Ojeda <ojeda@kernel.org>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Tejun Heo <tj@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/linux/slab.h |   68 +++++++++++++++++++++--------------------
 1 file changed, 35 insertions(+), 33 deletions(-)

--- a/include/linux/slab.h~slab-clean-up-function-declarations
+++ a/include/linux/slab.h
@@ -152,8 +152,8 @@ struct kmem_cache *kmem_cache_create_use
 			slab_flags_t flags,
 			unsigned int useroffset, unsigned int usersize,
 			void (*ctor)(void *));
-void kmem_cache_destroy(struct kmem_cache *);
-int kmem_cache_shrink(struct kmem_cache *);
+void kmem_cache_destroy(struct kmem_cache *s);
+int kmem_cache_shrink(struct kmem_cache *s);
 
 /*
  * Please use this macro to create slab caches. Simply specify the
@@ -181,11 +181,12 @@ 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 kfree(const void *);
-void kfree_sensitive(const void *);
-size_t __ksize(const void *);
-size_t ksize(const void *);
+__must_check
+void *krealloc(const void *objp, size_t new_size, gfp_t flags);
+void kfree(const void *objp);
+void kfree_sensitive(const void *objp);
+size_t __ksize(const void *objp);
+size_t ksize(const void *objp);
 #ifdef CONFIG_PRINTK
 bool kmem_valid_obj(void *object);
 void kmem_dump_obj(void *object);
@@ -426,8 +427,8 @@ static __always_inline unsigned int __km
 #endif /* !CONFIG_SLOB */
 
 void *__kmalloc(size_t size, gfp_t flags) __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 *);
+void *kmem_cache_alloc(struct kmem_cache *s, gfp_t flags) __assume_kmalloc_alignment __malloc;
+void kmem_cache_free(struct kmem_cache *s, void *objp);
 
 /*
  * Bulk allocation and freeing operations. These are accelerated in an
@@ -436,8 +437,8 @@ void kmem_cache_free(struct kmem_cache *
  *
  * Note that interrupts must be enabled when calling these functions.
  */
-void kmem_cache_free_bulk(struct kmem_cache *, size_t, void **);
-int kmem_cache_alloc_bulk(struct kmem_cache *, gfp_t, size_t, void **);
+void kmem_cache_free_bulk(struct kmem_cache *orig_s, size_t size, void **p);
+int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size, void **p);
 
 /*
  * Caller must not use kfree_bulk() on memory not originally allocated
@@ -449,8 +450,9 @@ static __always_inline void kfree_bulk(s
 }
 
 #ifdef CONFIG_NUMA
-void *__kmalloc_node(size_t size, gfp_t flags, int node) __assume_kmalloc_alignment __malloc;
-void *kmem_cache_alloc_node(struct kmem_cache *, gfp_t flags, int node) __assume_slab_alignment __malloc;
+void *__kmalloc_node(size_t size, gfp_t flags, int node) __assume_slab_alignment __malloc;
+void *kmem_cache_alloc_node(struct kmem_cache *s, gfp_t flags, int node)
+			    __assume_slab_alignment __malloc;
 #else
 static __always_inline void *__kmalloc_node(size_t size, gfp_t flags, int node)
 {
@@ -464,17 +466,15 @@ static __always_inline void *kmem_cache_
 #endif
 
 #ifdef CONFIG_TRACING
-extern void *kmem_cache_alloc_trace(struct kmem_cache *, gfp_t, size_t) __assume_slab_alignment __malloc;
+extern void *kmem_cache_alloc_trace(struct kmem_cache *s, gfp_t flags, size_t size)
+				    __assume_slab_alignment __malloc;
 
 #ifdef CONFIG_NUMA
-extern void *kmem_cache_alloc_node_trace(struct kmem_cache *s,
-					   gfp_t gfpflags,
-					   int node, size_t size) __assume_slab_alignment __malloc;
+extern void *kmem_cache_alloc_node_trace(struct kmem_cache *s, gfp_t gfpflags,
+					 int node, size_t size) __assume_slab_alignment __malloc;
 #else
-static __always_inline void *
-kmem_cache_alloc_node_trace(struct kmem_cache *s,
-			      gfp_t gfpflags,
-			      int node, size_t size)
+static __always_inline void *kmem_cache_alloc_node_trace(struct kmem_cache *s,
+					gfp_t gfpflags, int node, size_t size)
 {
 	return kmem_cache_alloc_trace(s, gfpflags, size);
 }
@@ -490,10 +490,8 @@ static __always_inline void *kmem_cache_
 	return ret;
 }
 
-static __always_inline void *
-kmem_cache_alloc_node_trace(struct kmem_cache *s,
-			      gfp_t gfpflags,
-			      int node, size_t size)
+static __always_inline void *kmem_cache_alloc_node_trace(struct kmem_cache *s,
+					gfp_t gfpflags, int node, size_t size)
 {
 	void *ret = kmem_cache_alloc_node(s, gfpflags, node);
 
@@ -502,13 +500,15 @@ kmem_cache_alloc_node_trace(struct kmem_
 }
 #endif /* CONFIG_TRACING */
 
-extern void *kmalloc_order(size_t size, gfp_t flags, unsigned int order) __assume_page_alignment __malloc;
+extern void *kmalloc_order(size_t size, gfp_t flags, unsigned int order)
+			   __assume_page_alignment __malloc;
 
 #ifdef CONFIG_TRACING
-extern void *kmalloc_order_trace(size_t size, gfp_t flags, unsigned int order) __assume_page_alignment __malloc;
+extern void *kmalloc_order_trace(size_t size, gfp_t flags, unsigned int order)
+				 __assume_page_alignment __malloc;
 #else
-static __always_inline void *
-kmalloc_order_trace(size_t size, gfp_t flags, unsigned int order)
+static __always_inline void *kmalloc_order_trace(size_t size, gfp_t flags,
+						 unsigned int order)
 {
 	return kmalloc_order(size, flags, order);
 }
@@ -638,8 +638,9 @@ static inline void *kmalloc_array(size_t
  * @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 *
-krealloc_array(void *p, size_t new_n, size_t new_size, gfp_t flags)
+__must_check
+static inline void *krealloc_array(void *p, size_t new_n, size_t new_size,
+				   gfp_t flags)
 {
 	size_t bytes;
 
@@ -668,7 +669,7 @@ static inline void *kcalloc(size_t n, si
  * allocator where we care about the real place the memory allocation
  * request comes from.
  */
-extern void *__kmalloc_track_caller(size_t, gfp_t, unsigned long);
+extern void *__kmalloc_track_caller(size_t size, gfp_t flags, unsigned long caller);
 #define kmalloc_track_caller(size, flags) \
 	__kmalloc_track_caller(size, flags, _RET_IP_)
 
@@ -691,7 +692,8 @@ static inline void *kcalloc_node(size_t
 
 
 #ifdef CONFIG_NUMA
-extern void *__kmalloc_node_track_caller(size_t, gfp_t, int, unsigned long);
+extern void *__kmalloc_node_track_caller(size_t size, gfp_t flags, int node,
+					 unsigned long caller);
 #define kmalloc_node_track_caller(size, flags, node) \
 	__kmalloc_node_track_caller(size, flags, node, \
 			_RET_IP_)
_

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [patch 6/9] slab: add __alloc_size attributes for better bounds checking
  2021-09-10  3:09 incoming Andrew Morton
                   ` (4 preceding siblings ...)
  2021-09-10  3:10 ` [patch 5/9] slab: clean up function declarations Andrew Morton
@ 2021-09-10  3:10 ` Andrew Morton
  2021-09-10  3:10 ` [patch 7/9] mm/page_alloc: " Andrew Morton
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 49+ messages in thread
From: Andrew Morton @ 2021-09-10  3:10 UTC (permalink / raw)
  To: akpm, apw, cl, danielmicay, dennis, dwaipayanray1,
	iamjoonsoo.kim, joe, keescook, linux-mm, lukas.bulwahn,
	mm-commits, nathan, ndesaulniers, ojeda, penberg, rientjes, tj,
	torvalds, vbabka

From: Kees Cook <keescook@chromium.org>
Subject: slab: add __alloc_size attributes for better bounds checking

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.

Link: https://lkml.kernel.org/r/20210818214021.2476230-5-keescook@chromium.org
Signed-off-by: Kees Cook <keescook@chromium.org>
Co-developed-by: Daniel Micay <danielmicay@gmail.com>
Signed-off-by: Daniel Micay <danielmicay@gmail.com>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Cc: Christoph Lameter <cl@linux.com>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Andy Whitcroft <apw@canonical.com>
Cc: Dennis Zhou <dennis@kernel.org>
Cc: Dwaipayan Ray <dwaipayanray1@gmail.com>
Cc: Joe Perches <joe@perches.com>
Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Cc: Miguel Ojeda <ojeda@kernel.org>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Tejun Heo <tj@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/linux/slab.h |   20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

--- a/include/linux/slab.h~slab-add-__alloc_size-attributes-for-better-bounds-checking
+++ a/include/linux/slab.h
@@ -181,7 +181,7 @@ int kmem_cache_shrink(struct kmem_cache
 /*
  * Common kmalloc functions provided by all allocators
  */
-__must_check
+__must_check __alloc_size(2)
 void *krealloc(const void *objp, size_t new_size, gfp_t flags);
 void kfree(const void *objp);
 void kfree_sensitive(const void *objp);
@@ -426,6 +426,7 @@ static __always_inline unsigned int __km
 #define kmalloc_index(s) __kmalloc_index(s, true)
 #endif /* !CONFIG_SLOB */
 
+__alloc_size(1)
 void *__kmalloc(size_t size, gfp_t flags) __assume_kmalloc_alignment __malloc;
 void *kmem_cache_alloc(struct kmem_cache *s, gfp_t flags) __assume_kmalloc_alignment __malloc;
 void kmem_cache_free(struct kmem_cache *s, void *objp);
@@ -450,6 +451,7 @@ static __always_inline void kfree_bulk(s
 }
 
 #ifdef CONFIG_NUMA
+__alloc_size(1)
 void *__kmalloc_node(size_t size, gfp_t flags, int node) __assume_slab_alignment __malloc;
 void *kmem_cache_alloc_node(struct kmem_cache *s, gfp_t flags, int node)
 			    __assume_slab_alignment __malloc;
@@ -574,6 +576,7 @@ static __always_inline void *kmalloc_lar
  *	Try really hard to succeed the allocation but fail
  *	eventually.
  */
+__alloc_size(1)
 static __always_inline void *kmalloc(size_t size, gfp_t flags)
 {
 	if (__builtin_constant_p(size)) {
@@ -596,6 +599,7 @@ static __always_inline void *kmalloc(siz
 	return __kmalloc(size, flags);
 }
 
+__alloc_size(1)
 static __always_inline void *kmalloc_node(size_t size, gfp_t flags, int node)
 {
 #ifndef CONFIG_SLOB
@@ -620,6 +624,7 @@ static __always_inline void *kmalloc_nod
  * @size: element size.
  * @flags: the type of memory to allocate (see kmalloc).
  */
+__alloc_size(1, 2)
 static inline void *kmalloc_array(size_t n, size_t size, gfp_t flags)
 {
 	size_t bytes;
@@ -638,7 +643,7 @@ static inline void *kmalloc_array(size_t
  * @new_size: new size of a single member of the array
  * @flags: the type of memory to allocate (see kmalloc)
  */
-__must_check
+__must_check __alloc_size(2, 3)
 static inline void *krealloc_array(void *p, size_t new_n, size_t new_size,
 				   gfp_t flags)
 {
@@ -656,6 +661,7 @@ static inline void *krealloc_array(void
  * @size: element size.
  * @flags: the type of memory to allocate (see kmalloc).
  */
+__alloc_size(1, 2)
 static inline void *kcalloc(size_t n, size_t size, gfp_t flags)
 {
 	return kmalloc_array(n, size, flags | __GFP_ZERO);
@@ -685,6 +691,7 @@ static inline void *kmalloc_array_node(s
 	return __kmalloc_node(bytes, flags, node);
 }
 
+__alloc_size(1, 2)
 static inline void *kcalloc_node(size_t n, size_t size, gfp_t flags, int node)
 {
 	return kmalloc_array_node(n, size, flags | __GFP_ZERO, node);
@@ -718,6 +725,7 @@ static inline void *kmem_cache_zalloc(st
  * @size: how many bytes of memory are required.
  * @flags: the type of memory to allocate (see kmalloc).
  */
+__alloc_size(1)
 static inline void *kzalloc(size_t size, gfp_t flags)
 {
 	return kmalloc(size, flags | __GFP_ZERO);
@@ -729,25 +737,31 @@ static inline void *kzalloc(size_t size,
  * @flags: the type of memory to allocate (see kmalloc).
  * @node: memory node from which to allocate
  */
+__alloc_size(1)
 static inline void *kzalloc_node(size_t size, gfp_t flags, int node)
 {
 	return kmalloc_node(size, flags | __GFP_ZERO, node);
 }
 
+__alloc_size(1)
 extern void *kvmalloc_node(size_t size, gfp_t flags, int node);
+__alloc_size(1)
 static inline void *kvmalloc(size_t size, gfp_t flags)
 {
 	return kvmalloc_node(size, flags, NUMA_NO_NODE);
 }
+__alloc_size(1)
 static inline void *kvzalloc_node(size_t size, gfp_t flags, int node)
 {
 	return kvmalloc_node(size, flags | __GFP_ZERO, node);
 }
+__alloc_size(1)
 static inline void *kvzalloc(size_t size, gfp_t flags)
 {
 	return kvmalloc(size, flags | __GFP_ZERO);
 }
 
+__alloc_size(1, 2)
 static inline void *kvmalloc_array(size_t n, size_t size, gfp_t flags)
 {
 	size_t bytes;
@@ -758,11 +772,13 @@ static inline void *kvmalloc_array(size_
 	return kvmalloc(bytes, flags);
 }
 
+__alloc_size(1, 2)
 static inline void *kvcalloc(size_t n, size_t size, gfp_t flags)
 {
 	return kvmalloc_array(n, size, flags | __GFP_ZERO);
 }
 
+__alloc_size(3)
 extern void *kvrealloc(const void *p, size_t oldsize, size_t newsize,
 		gfp_t flags);
 extern void kvfree(const void *addr);
_

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [patch 7/9] mm/page_alloc: add __alloc_size attributes for better bounds checking
  2021-09-10  3:09 incoming Andrew Morton
                   ` (5 preceding siblings ...)
  2021-09-10  3:10 ` [patch 6/9] slab: add __alloc_size attributes for better bounds checking Andrew Morton
@ 2021-09-10  3:10 ` Andrew Morton
  2021-09-10  3:10 ` [patch 8/9] percpu: " Andrew Morton
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 49+ messages in thread
From: Andrew Morton @ 2021-09-10  3:10 UTC (permalink / raw)
  To: akpm, apw, cl, danielmicay, dennis, dwaipayanray1,
	iamjoonsoo.kim, joe, keescook, linux-mm, lukas.bulwahn,
	mm-commits, nathan, ndesaulniers, ojeda, penberg, rientjes, tj,
	torvalds, vbabka

From: Kees Cook <keescook@chromium.org>
Subject: mm/page_alloc: add __alloc_size attributes for better bounds checking

As already done in GrapheneOS, add the __alloc_size attribute for
appropriate page allocator interfaces, to provide additional hinting for
better bounds checking, assisting CONFIG_FORTIFY_SOURCE and other compiler
optimizations.

Link: https://lkml.kernel.org/r/20210818214021.2476230-6-keescook@chromium.org
Signed-off-by: Kees Cook <keescook@chromium.org>
Co-developed-by: Daniel Micay <danielmicay@gmail.com>
Signed-off-by: Daniel Micay <danielmicay@gmail.com>
Cc: Andy Whitcroft <apw@canonical.com>
Cc: Christoph Lameter <cl@linux.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Dennis Zhou <dennis@kernel.org>
Cc: Dwaipayan Ray <dwaipayanray1@gmail.com>
Cc: Joe Perches <joe@perches.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Cc: Miguel Ojeda <ojeda@kernel.org>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/linux/gfp.h |    2 ++
 1 file changed, 2 insertions(+)

--- a/include/linux/gfp.h~mm-page_alloc-add-__alloc_size-attributes-for-better-bounds-checking
+++ a/include/linux/gfp.h
@@ -608,8 +608,10 @@ static inline struct page *alloc_pages(g
 extern unsigned long __get_free_pages(gfp_t gfp_mask, unsigned int order);
 extern unsigned long get_zeroed_page(gfp_t gfp_mask);
 
+__alloc_size(1)
 void *alloc_pages_exact(size_t size, gfp_t gfp_mask);
 void free_pages_exact(void *virt, size_t size);
+__alloc_size(1)
 void * __meminit alloc_pages_exact_nid(int nid, size_t size, gfp_t gfp_mask);
 
 #define __get_free_page(gfp_mask) \
_

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [patch 8/9] percpu: add __alloc_size attributes for better bounds checking
  2021-09-10  3:09 incoming Andrew Morton
                   ` (6 preceding siblings ...)
  2021-09-10  3:10 ` [patch 7/9] mm/page_alloc: " Andrew Morton
@ 2021-09-10  3:10 ` Andrew Morton
  2021-09-10  3:10 ` [patch 9/9] mm/vmalloc: " Andrew Morton
  2021-09-10 17:11 ` incoming Kees Cook
  9 siblings, 0 replies; 49+ messages in thread
From: Andrew Morton @ 2021-09-10  3:10 UTC (permalink / raw)
  To: akpm, apw, cl, danielmicay, dennis, dwaipayanray1,
	iamjoonsoo.kim, joe, keescook, linux-mm, lukas.bulwahn,
	mm-commits, nathan, ndesaulniers, ojeda, penberg, rientjes, tj,
	torvalds, vbabka

From: Kees Cook <keescook@chromium.org>
Subject: percpu: add __alloc_size attributes for better bounds checking

As already done in GrapheneOS, add the __alloc_size attribute for
appropriate percpu allocator interfaces, to provide additional hinting for
better bounds checking, assisting CONFIG_FORTIFY_SOURCE and other compiler
optimizations.

Link: https://lkml.kernel.org/r/20210818214021.2476230-7-keescook@chromium.org
Signed-off-by: Kees Cook <keescook@chromium.org>
Co-developed-by: Daniel Micay <danielmicay@gmail.com>
Signed-off-by: Daniel Micay <danielmicay@gmail.com>
Cc: Dennis Zhou <dennis@kernel.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Christoph Lameter <cl@linux.com>
Cc: Andy Whitcroft <apw@canonical.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Dwaipayan Ray <dwaipayanray1@gmail.com>
Cc: Joe Perches <joe@perches.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Cc: Miguel Ojeda <ojeda@kernel.org>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/linux/percpu.h |    3 +++
 1 file changed, 3 insertions(+)

--- a/include/linux/percpu.h~percpu-add-__alloc_size-attributes-for-better-bounds-checking
+++ a/include/linux/percpu.h
@@ -123,6 +123,7 @@ extern int __init pcpu_page_first_chunk(
 				pcpu_fc_populate_pte_fn_t populate_pte_fn);
 #endif
 
+__alloc_size(1)
 extern void __percpu *__alloc_reserved_percpu(size_t size, size_t align);
 extern bool __is_kernel_percpu_address(unsigned long addr, unsigned long *can_addr);
 extern bool is_kernel_percpu_address(unsigned long addr);
@@ -131,7 +132,9 @@ extern bool is_kernel_percpu_address(uns
 extern void __init setup_per_cpu_areas(void);
 #endif
 
+__alloc_size(1)
 extern void __percpu *__alloc_percpu_gfp(size_t size, size_t align, gfp_t gfp);
+__alloc_size(1)
 extern void __percpu *__alloc_percpu(size_t size, size_t align);
 extern void free_percpu(void __percpu *__pdata);
 extern phys_addr_t per_cpu_ptr_to_phys(void *addr);
_

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [patch 9/9] mm/vmalloc: add __alloc_size attributes for better bounds checking
  2021-09-10  3:09 incoming Andrew Morton
                   ` (7 preceding siblings ...)
  2021-09-10  3:10 ` [patch 8/9] percpu: " Andrew Morton
@ 2021-09-10  3:10 ` Andrew Morton
  2021-09-10 17:23     ` Linus Torvalds
  2021-09-10 17:11 ` incoming Kees Cook
  9 siblings, 1 reply; 49+ messages in thread
From: Andrew Morton @ 2021-09-10  3:10 UTC (permalink / raw)
  To: akpm, apw, cl, danielmicay, dennis, dwaipayanray1,
	iamjoonsoo.kim, joe, keescook, linux-mm, lukas.bulwahn,
	mm-commits, nathan, ndesaulniers, ojeda, penberg, rientjes, tj,
	torvalds, vbabka

From: Kees Cook <keescook@chromium.org>
Subject: mm/vmalloc: add __alloc_size attributes for better bounds checking

As already done in GrapheneOS, add the __alloc_size attribute for
appropriate vmalloc allocator interfaces, to provide additional hinting
for better bounds checking, assisting CONFIG_FORTIFY_SOURCE and other
compiler optimizations.

Link: https://lkml.kernel.org/r/20210818214021.2476230-8-keescook@chromium.org
Signed-off-by: Kees Cook <keescook@chromium.org>
Co-developed-by: Daniel Micay <danielmicay@gmail.com>
Signed-off-by: Daniel Micay <danielmicay@gmail.com>
Cc: Andy Whitcroft <apw@canonical.com>
Cc: Christoph Lameter <cl@linux.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Dennis Zhou <dennis@kernel.org>
Cc: Dwaipayan Ray <dwaipayanray1@gmail.com>
Cc: Joe Perches <joe@perches.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Cc: Miguel Ojeda <ojeda@kernel.org>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/linux/vmalloc.h |   11 +++++++++++
 1 file changed, 11 insertions(+)

--- a/include/linux/vmalloc.h~mm-vmalloc-add-__alloc_size-attributes-for-better-bounds-checking
+++ a/include/linux/vmalloc.h
@@ -136,20 +136,31 @@ static inline void vmalloc_init(void)
 static inline unsigned long vmalloc_nr_pages(void) { return 0; }
 #endif
 
+__alloc_size(1)
 extern void *vmalloc(unsigned long size);
+__alloc_size(1)
 extern void *vzalloc(unsigned long size);
+__alloc_size(1)
 extern void *vmalloc_user(unsigned long size);
+__alloc_size(1)
 extern void *vmalloc_node(unsigned long size, int node);
+__alloc_size(1)
 extern void *vzalloc_node(unsigned long size, int node);
+__alloc_size(1)
 extern void *vmalloc_32(unsigned long size);
+__alloc_size(1)
 extern void *vmalloc_32_user(unsigned long size);
+__alloc_size(1)
 extern void *__vmalloc(unsigned long size, gfp_t gfp_mask);
+__alloc_size(1)
 extern void *__vmalloc_node_range(unsigned long size, unsigned long align,
 			unsigned long start, unsigned long end, gfp_t gfp_mask,
 			pgprot_t prot, unsigned long vm_flags, int node,
 			const void *caller);
+__alloc_size(1)
 void *__vmalloc_node(unsigned long size, unsigned long align, gfp_t gfp_mask,
 		int node, const void *caller);
+__alloc_size(1)
 void *vmalloc_no_huge(unsigned long size);
 
 extern void vfree(const void *addr);
_

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: incoming
  2021-09-10  3:09 incoming Andrew Morton
                   ` (8 preceding siblings ...)
  2021-09-10  3:10 ` [patch 9/9] mm/vmalloc: " Andrew Morton
@ 2021-09-10 17:11 ` Kees Cook
  2021-09-10 20:13   ` incoming Kees Cook
  9 siblings, 1 reply; 49+ messages in thread
From: Kees Cook @ 2021-09-10 17:11 UTC (permalink / raw)
  To: Linus Torvalds, Andrew Morton; +Cc: linux-mm, mm-commits

On Thu, Sep 09, 2021 at 08:09:48PM -0700, Andrew Morton wrote:
> 
> More post linux-next material.
> 
> 9 patches, based on f154c806676ad7153c6e161f30c53a44855329d6.
> 
> Subsystems affected by this patch series:
> 
>   mm/slab-generic
>   rapidio
>   mm/debug
> 
> Subsystem: mm/slab-generic
> 
>     "Matthew Wilcox (Oracle)" <willy@infradead.org>:
>       mm: move kvmalloc-related functions to slab.h
> 
> Subsystem: rapidio
> 
>     Kees Cook <keescook@chromium.org>:
>       rapidio: avoid bogus __alloc_size warning
> 
> Subsystem: mm/debug
> 
>     Kees Cook <keescook@chromium.org>:
>     Patch series "Add __alloc_size() for better bounds checking", v2:
>       Compiler Attributes: add __alloc_size() for better bounds checking
>       checkpatch: add __alloc_size() to known $Attribute
>       slab: clean up function declarations
>       slab: add __alloc_size attributes for better bounds checking
>       mm/page_alloc: add __alloc_size attributes for better bounds checking
>       percpu: add __alloc_size attributes for better bounds checking
>       mm/vmalloc: add __alloc_size attributes for better bounds checking

Hi,

FYI, in overnight build testing I found yet another corner case in
GCC's handling of the __alloc_size attribute. It's the gift that keeps
on giving. The fix is here:

https://lore.kernel.org/lkml/20210910165851.3296624-1-keescook@chromium.org/

> 
>  Makefile                                 |   15 +++
>  drivers/of/kexec.c                       |    1 
>  drivers/rapidio/devices/rio_mport_cdev.c |    9 +-
>  include/linux/compiler_attributes.h      |    6 +
>  include/linux/gfp.h                      |    2 
>  include/linux/mm.h                       |   34 --------
>  include/linux/percpu.h                   |    3 
>  include/linux/slab.h                     |  122 ++++++++++++++++++++++---------
>  include/linux/vmalloc.h                  |   11 ++
>  scripts/checkpatch.pl                    |    3 
>  10 files changed, 132 insertions(+), 74 deletions(-)
> 

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [patch 9/9] mm/vmalloc: add __alloc_size attributes for better bounds checking
  2021-09-10  3:10 ` [patch 9/9] mm/vmalloc: " Andrew Morton
@ 2021-09-10 17:23     ` Linus Torvalds
  0 siblings, 0 replies; 49+ messages in thread
From: Linus Torvalds @ 2021-09-10 17:23 UTC (permalink / raw)
  To: Andrew Morton
  Cc: apw, Christoph Lameter, Daniel Micay, Dennis Zhou, dwaipayanray1,
	Joonsoo Kim, Joe Perches, Kees Cook, Linux-MM, Lukas Bulwahn,
	mm-commits, Nathan Chancellor, Nick Desaulniers, Miguel Ojeda,
	Pekka Enberg, David Rientjes, Tejun Heo, Vlastimil Babka

On Thu, Sep 9, 2021 at 8:10 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> +__alloc_size(1)
>  extern void *vmalloc(unsigned long size);
[...]

All of these are added in the wrong place - inconsistent with the very
compiler documentation the patches add.

The function attributes are generally added _after_ the function,
although admittedly we've been quite confused here before.

But the very compiler documentation you point to in the patch that
adds these macros gives that as the examples both for gcc and clang:

+ *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-alloc_005fsize-function-attribute
+ * clang: https://clang.llvm.org/docs/AttributeReference.html#alloc-size

and honestly I think that is the preferred format because this is
about the *function*, not about the return type.

Do both placements work? Yes.

Have we been confused about this before? Yes. I note that our __printf
attributes in particular have been added in odd places. And our
existing __malloc annotations seem to correct in <linux/slab.h> and
<linux/device.h> but then randomly applied in some other places.

I really think it's pointlessly stupid and hard to read/grep for to
make it be a separate line before the whole thing.

I also think it should have taken over the "__malloc" name that is
almost unused right now. Because why would you ever have
__alloc_size() without having __malloc().

So wouldn't this all have looked much more natural as

     void *vmalloc(unsigned long size) __malloc(1);

instead? Hmm?

              Linus

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [patch 9/9] mm/vmalloc: add __alloc_size attributes for better bounds checking
@ 2021-09-10 17:23     ` Linus Torvalds
  0 siblings, 0 replies; 49+ messages in thread
From: Linus Torvalds @ 2021-09-10 17:23 UTC (permalink / raw)
  To: Andrew Morton
  Cc: apw, Christoph Lameter, Daniel Micay, Dennis Zhou, dwaipayanray1,
	Joonsoo Kim, Joe Perches, Kees Cook, Linux-MM, Lukas Bulwahn,
	mm-commits, Nathan Chancellor, Nick Desaulniers, Miguel Ojeda,
	Pekka Enberg, David Rientjes, Tejun Heo, Vlastimil Babka

On Thu, Sep 9, 2021 at 8:10 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> +__alloc_size(1)
>  extern void *vmalloc(unsigned long size);
[...]

All of these are added in the wrong place - inconsistent with the very
compiler documentation the patches add.

The function attributes are generally added _after_ the function,
although admittedly we've been quite confused here before.

But the very compiler documentation you point to in the patch that
adds these macros gives that as the examples both for gcc and clang:

+ *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-alloc_005fsize-function-attribute
+ * clang: https://clang.llvm.org/docs/AttributeReference.html#alloc-size

and honestly I think that is the preferred format because this is
about the *function*, not about the return type.

Do both placements work? Yes.

Have we been confused about this before? Yes. I note that our __printf
attributes in particular have been added in odd places. And our
existing __malloc annotations seem to correct in <linux/slab.h> and
<linux/device.h> but then randomly applied in some other places.

I really think it's pointlessly stupid and hard to read/grep for to
make it be a separate line before the whole thing.

I also think it should have taken over the "__malloc" name that is
almost unused right now. Because why would you ever have
__alloc_size() without having __malloc().

So wouldn't this all have looked much more natural as

     void *vmalloc(unsigned long size) __malloc(1);

instead? Hmm?

              Linus


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [patch 9/9] mm/vmalloc: add __alloc_size attributes for better bounds checking
  2021-09-10 17:23     ` Linus Torvalds
  (?)
@ 2021-09-10 18:43     ` Kees Cook
  2021-09-10 19:17         ` Linus Torvalds
  -1 siblings, 1 reply; 49+ messages in thread
From: Kees Cook @ 2021-09-10 18:43 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Andrew Morton, apw, Christoph Lameter, Daniel Micay, Dennis Zhou,
	dwaipayanray1, Joonsoo Kim, Joe Perches, Linux-MM, Lukas Bulwahn,
	mm-commits, Nathan Chancellor, Nick Desaulniers, Miguel Ojeda,
	Pekka Enberg, David Rientjes, Tejun Heo, Vlastimil Babka

On Fri, Sep 10, 2021 at 10:23:48AM -0700, Linus Torvalds wrote:
> On Thu, Sep 9, 2021 at 8:10 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > +__alloc_size(1)
> >  extern void *vmalloc(unsigned long size);
> [...]
> 
> All of these are added in the wrong place - inconsistent with the very
> compiler documentation the patches add.
> 
> The function attributes are generally added _after_ the function,
> although admittedly we've been quite confused here before.
> 
> But the very compiler documentation you point to in the patch that
> adds these macros gives that as the examples both for gcc and clang:
> 
> + *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-alloc_005fsize-function-attribute
> + * clang: https://clang.llvm.org/docs/AttributeReference.html#alloc-size
> 
> and honestly I think that is the preferred format because this is
> about the *function*, not about the return type.
> 
> Do both placements work? Yes.
> 
> Have we been confused about this before? Yes. I note that our __printf
> attributes in particular have been added in odd places. And our
> existing __malloc annotations seem to correct in <linux/slab.h> and
> <linux/device.h> but then randomly applied in some other places.
> 
> I really think it's pointlessly stupid and hard to read/grep for to
> make it be a separate line before the whole thing.

This was bike-shed on the list, and this result seemed to be consensus,
but I kind of dislike all the options. Either things are on separate
lines or they're trailing attributes that get really long, etc. Ugh.

I'm happy to clean all of it up into whatever form can be agreed on for
the "correct" placement.

> I also think it should have taken over the "__malloc" name that is
> almost unused right now. Because why would you ever have
> __alloc_size() without having __malloc().

I had originally set out to do that, but the problem with merging with
__malloc is the bit in the docs about "and that the memory has undefined
content". So we can't do that for kmalloc() in the face of GFP_ZERO, as
well as a bunch of other helpers. I always get suspicious about "this
will improve optimization because we depend on claiming something is
'undefined'". :|

-Kees

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [patch 9/9] mm/vmalloc: add __alloc_size attributes for better bounds checking
  2021-09-10 18:43     ` Kees Cook
@ 2021-09-10 19:17         ` Linus Torvalds
  0 siblings, 0 replies; 49+ messages in thread
From: Linus Torvalds @ 2021-09-10 19:17 UTC (permalink / raw)
  To: Kees Cook
  Cc: Andrew Morton, apw, Christoph Lameter, Daniel Micay, Dennis Zhou,
	dwaipayanray1, Joonsoo Kim, Joe Perches, Linux-MM, Lukas Bulwahn,
	mm-commits, Nathan Chancellor, Nick Desaulniers, Miguel Ojeda,
	Pekka Enberg, David Rientjes, Tejun Heo, Vlastimil Babka

On Fri, Sep 10, 2021 at 11:43 AM Kees Cook <keescook@chromium.org> wrote:
>
> I had originally set out to do that, but the problem with merging with
> __malloc is the bit in the docs about "and that the memory has undefined
> content". So we can't do that for kmalloc() in the face of GFP_ZERO, as
> well as a bunch of other helpers. I always get suspicious about "this
> will improve optimization because we depend on claiming something is
> 'undefined'". :|

Oh, I had entirely missed that historical subtlety of __malloc.

Yeah, that would have been absolutely horrible. But it's not actually
really true.

It seems that the gcc people actually realized the problem, and fixed
the documentation:

  "Attribute malloc indicates that a function is malloc-like, i.e.,
that the pointer P returned by the function cannot alias any other
pointer valid when the function returns, and moreover no pointers to
valid objects occur in any storage addressed by P. In addition, the
GCC predicts that a function with the attribute returns non-null in
most cases"

IOW, it is purely about aliasing guarantees. Basically the guarantee
is that the memory that a "malloc" function returns can not alias
(directly or indirectly) any other allocations.

See

    https://gcc.gnu.org/onlinedocs/gcc-11.2.0/gcc/Common-Function-Attributes.html#Common-Function-Attributes

So I think it's ok, and your reaction was entirely correct, but came
from looking at old documentation.

             Linus


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [patch 9/9] mm/vmalloc: add __alloc_size attributes for better bounds checking
@ 2021-09-10 19:17         ` Linus Torvalds
  0 siblings, 0 replies; 49+ messages in thread
From: Linus Torvalds @ 2021-09-10 19:17 UTC (permalink / raw)
  To: Kees Cook
  Cc: Andrew Morton, apw, Christoph Lameter, Daniel Micay, Dennis Zhou,
	dwaipayanray1, Joonsoo Kim, Joe Perches, Linux-MM, Lukas Bulwahn,
	mm-commits, Nathan Chancellor, Nick Desaulniers, Miguel Ojeda,
	Pekka Enberg, David Rientjes, Tejun Heo, Vlastimil Babka

On Fri, Sep 10, 2021 at 11:43 AM Kees Cook <keescook@chromium.org> wrote:
>
> I had originally set out to do that, but the problem with merging with
> __malloc is the bit in the docs about "and that the memory has undefined
> content". So we can't do that for kmalloc() in the face of GFP_ZERO, as
> well as a bunch of other helpers. I always get suspicious about "this
> will improve optimization because we depend on claiming something is
> 'undefined'". :|

Oh, I had entirely missed that historical subtlety of __malloc.

Yeah, that would have been absolutely horrible. But it's not actually
really true.

It seems that the gcc people actually realized the problem, and fixed
the documentation:

  "Attribute malloc indicates that a function is malloc-like, i.e.,
that the pointer P returned by the function cannot alias any other
pointer valid when the function returns, and moreover no pointers to
valid objects occur in any storage addressed by P. In addition, the
GCC predicts that a function with the attribute returns non-null in
most cases"

IOW, it is purely about aliasing guarantees. Basically the guarantee
is that the memory that a "malloc" function returns can not alias
(directly or indirectly) any other allocations.

See

    https://gcc.gnu.org/onlinedocs/gcc-11.2.0/gcc/Common-Function-Attributes.html#Common-Function-Attributes

So I think it's ok, and your reaction was entirely correct, but came
from looking at old documentation.

             Linus

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [patch 9/9] mm/vmalloc: add __alloc_size attributes for better bounds checking
  2021-09-10 19:17         ` Linus Torvalds
  (?)
@ 2021-09-10 19:32         ` Kees Cook
  -1 siblings, 0 replies; 49+ messages in thread
From: Kees Cook @ 2021-09-10 19:32 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Andrew Morton, apw, Christoph Lameter, Daniel Micay, Dennis Zhou,
	dwaipayanray1, Joonsoo Kim, Joe Perches, Linux-MM, Lukas Bulwahn,
	mm-commits, Nathan Chancellor, Nick Desaulniers, Miguel Ojeda,
	Pekka Enberg, David Rientjes, Tejun Heo, Vlastimil Babka

On Fri, Sep 10, 2021 at 12:17:40PM -0700, Linus Torvalds wrote:
> On Fri, Sep 10, 2021 at 11:43 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > I had originally set out to do that, but the problem with merging with
> > __malloc is the bit in the docs about "and that the memory has undefined
> > content". So we can't do that for kmalloc() in the face of GFP_ZERO, as
> > well as a bunch of other helpers. I always get suspicious about "this
> > will improve optimization because we depend on claiming something is
> > 'undefined'". :|
> 
> Oh, I had entirely missed that historical subtlety of __malloc.
> 
> Yeah, that would have been absolutely horrible. But it's not actually
> really true.
> 
> It seems that the gcc people actually realized the problem, and fixed
> the documentation:
> 
>   "Attribute malloc indicates that a function is malloc-like, i.e.,
> that the pointer P returned by the function cannot alias any other
> pointer valid when the function returns, and moreover no pointers to
> valid objects occur in any storage addressed by P. In addition, the
> GCC predicts that a function with the attribute returns non-null in
> most cases"
> 
> IOW, it is purely about aliasing guarantees. Basically the guarantee
> is that the memory that a "malloc" function returns can not alias
> (directly or indirectly) any other allocations.
> 
> See
> 
>     https://gcc.gnu.org/onlinedocs/gcc-11.2.0/gcc/Common-Function-Attributes.html#Common-Function-Attributes
> 
> So I think it's ok, and your reaction was entirely correct, but came
> from looking at old documentation.

Okay, sounds good. The other reason for having them be separate is that
some of our allocators are implicitly sized. (i.e. kmem_cache_alloc()),
so there isn't actually a "size" argument to give. I suppose some kind
of VARARGS macro magic could be used to make __malloc() be valid, but
I don't like that in the face of future changes where people just don't
include the argument by accident.

How about the other way around, where __malloc is included in
__alloc_size()? Then the implicitly-sized allocators are left unchanged
with __malloc.

For the mechanical part of this, I'm left needing an answer to "what's
the style guide for this?" in the face of these longer definitions,
especially in the face of potential future trailing attributes.

e.g. all on one line would be 119 characters, way past even the updated
100 character limit:

__must_check static inline void *krealloc_array(void *p, size_t new_n, size_t new_size, gfp_t flags) __alloc_size(2, 3)
{
	...
}

Maybe this? I find it weird still:

__must_check static inline void *krealloc_array(void *p, size_t new_n,
						size_t new_size, gfp_t flags)
						__alloc_size(2, 3)
{
	...
}

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [patch 9/9] mm/vmalloc: add __alloc_size attributes for better bounds checking
  2021-09-10 17:23     ` Linus Torvalds
@ 2021-09-10 19:49       ` Nick Desaulniers
  -1 siblings, 0 replies; 49+ messages in thread
From: Nick Desaulniers @ 2021-09-10 19:49 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Andrew Morton, apw, Christoph Lameter, Daniel Micay, Dennis Zhou,
	dwaipayanray1, Joonsoo Kim, Joe Perches, Kees Cook, Linux-MM,
	Lukas Bulwahn, mm-commits, Nathan Chancellor, Miguel Ojeda,
	Pekka Enberg, David Rientjes, Tejun Heo, Vlastimil Babka

On Fri, Sep 10, 2021 at 10:24 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Thu, Sep 9, 2021 at 8:10 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > +__alloc_size(1)
> >  extern void *vmalloc(unsigned long size);
> [...]
>
> All of these are added in the wrong place

Huh?
$ grep -rn __always_inline
$ grep -rn noinline
$ grep -rn __cold
...

Not that we have explicit guidance here in any coding style guide,
perhaps we can make a clarification in
Documentation/process/coding-style.rst?

I'd say when using function attributes, we tend to have the linkage
(ie. explicitly static or implicitly extern), followed by function
attributes related to inlining (__always_inline, noinline), followed
by return type, followed by function level attributes, followed by
parameter list. But I don't think we're even internally consistent
here.
-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [patch 9/9] mm/vmalloc: add __alloc_size attributes for better bounds checking
@ 2021-09-10 19:49       ` Nick Desaulniers
  0 siblings, 0 replies; 49+ messages in thread
From: Nick Desaulniers @ 2021-09-10 19:49 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Andrew Morton, apw, Christoph Lameter, Daniel Micay, Dennis Zhou,
	dwaipayanray1, Joonsoo Kim, Joe Perches, Kees Cook, Linux-MM,
	Lukas Bulwahn, mm-commits, Nathan Chancellor, Miguel Ojeda,
	Pekka Enberg, David Rientjes, Tejun Heo, Vlastimil Babka

On Fri, Sep 10, 2021 at 10:24 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Thu, Sep 9, 2021 at 8:10 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > +__alloc_size(1)
> >  extern void *vmalloc(unsigned long size);
> [...]
>
> All of these are added in the wrong place

Huh?
$ grep -rn __always_inline
$ grep -rn noinline
$ grep -rn __cold
...

Not that we have explicit guidance here in any coding style guide,
perhaps we can make a clarification in
Documentation/process/coding-style.rst?

I'd say when using function attributes, we tend to have the linkage
(ie. explicitly static or implicitly extern), followed by function
attributes related to inlining (__always_inline, noinline), followed
by return type, followed by function level attributes, followed by
parameter list. But I don't think we're even internally consistent
here.
-- 
Thanks,
~Nick Desaulniers


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: incoming
  2021-09-10 17:11 ` incoming Kees Cook
@ 2021-09-10 20:13   ` Kees Cook
  0 siblings, 0 replies; 49+ messages in thread
From: Kees Cook @ 2021-09-10 20:13 UTC (permalink / raw)
  To: linux-kernel; +Cc: Linus Torvalds, Andrew Morton, linux-mm, mm-commits

On Fri, Sep 10, 2021 at 10:11:53AM -0700, Kees Cook wrote:
> On Thu, Sep 09, 2021 at 08:09:48PM -0700, Andrew Morton wrote:
> > 
> > More post linux-next material.
> > 
> > 9 patches, based on f154c806676ad7153c6e161f30c53a44855329d6.
> > 
> > Subsystems affected by this patch series:
> > 
> >   mm/slab-generic
> >   rapidio
> >   mm/debug
> > 
> > Subsystem: mm/slab-generic
> > 
> >     "Matthew Wilcox (Oracle)" <willy@infradead.org>:
> >       mm: move kvmalloc-related functions to slab.h
> > 
> > Subsystem: rapidio
> > 
> >     Kees Cook <keescook@chromium.org>:
> >       rapidio: avoid bogus __alloc_size warning
> > 
> > Subsystem: mm/debug
> > 
> >     Kees Cook <keescook@chromium.org>:
> >     Patch series "Add __alloc_size() for better bounds checking", v2:
> >       Compiler Attributes: add __alloc_size() for better bounds checking
> >       checkpatch: add __alloc_size() to known $Attribute
> >       slab: clean up function declarations
> >       slab: add __alloc_size attributes for better bounds checking
> >       mm/page_alloc: add __alloc_size attributes for better bounds checking
> >       percpu: add __alloc_size attributes for better bounds checking
> >       mm/vmalloc: add __alloc_size attributes for better bounds checking
> 
> Hi,
> 
> FYI, in overnight build testing I found yet another corner case in
> GCC's handling of the __alloc_size attribute. It's the gift that keeps
> on giving. The fix is here:
> 
> https://lore.kernel.org/lkml/20210910165851.3296624-1-keescook@chromium.org/

I'm so glad it's Friday. Here's the v2 fix... *sigh*

https://lore.kernel.org/lkml/20210910201132.3809437-1-keescook@chromium.org/

-Kees

> 
> > 
> >  Makefile                                 |   15 +++
> >  drivers/of/kexec.c                       |    1 
> >  drivers/rapidio/devices/rio_mport_cdev.c |    9 +-
> >  include/linux/compiler_attributes.h      |    6 +
> >  include/linux/gfp.h                      |    2 
> >  include/linux/mm.h                       |   34 --------
> >  include/linux/percpu.h                   |    3 
> >  include/linux/slab.h                     |  122 ++++++++++++++++++++++---------
> >  include/linux/vmalloc.h                  |   11 ++
> >  scripts/checkpatch.pl                    |    3 
> >  10 files changed, 132 insertions(+), 74 deletions(-)
> > 
> 
> -- 
> Kees Cook

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [patch 9/9] mm/vmalloc: add __alloc_size attributes for better bounds checking
  2021-09-10 19:49       ` Nick Desaulniers
@ 2021-09-10 20:16         ` Linus Torvalds
  -1 siblings, 0 replies; 49+ messages in thread
From: Linus Torvalds @ 2021-09-10 20:16 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Andrew Morton, apw, Christoph Lameter, Daniel Micay, Dennis Zhou,
	dwaipayanray1, Joonsoo Kim, Joe Perches, Kees Cook, Linux-MM,
	Lukas Bulwahn, mm-commits, Nathan Chancellor, Miguel Ojeda,
	Pekka Enberg, David Rientjes, Tejun Heo, Vlastimil Babka

On Fri, Sep 10, 2021 at 12:49 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> Huh?
> $ grep -rn __always_inline
> $ grep -rn noinline
> $ grep -rn __cold

Those are all examples of basically storage classes.

And yes, they go to the front - generally even before the return
value. Exactly like 'extern' and 'static'.

The In contrast, the "function argument descriptions" have
traditionally gone at the end, although you can certainly finds us
screwing that up too.

This, btw, tends to be how the compiler documentation also does it.

So to a close approximation

 - "storage class" goes first, so "static inline" etc.

 - return type next (including attributes directly related to the
returned value - like "__must_check")

 - then function name and argument declaration

 - and finally the "function argument type attributes" at the end.

can you do it in different orders? Yes. And the compiler won't even
generally warn about it. So we've gotten it wrong many many times.

I mean, compilers won't complain even about clear garbage that is _so_
bad that we generally get it right:

  int static myfn(void);

will build perfectly fine. That most certainly doesn't make it right.

Arguably "__malloc" could be seen about the returned type, rather than
being about the function declarations. But if it was about the
returned type, you'd call it a "restrict" pointer, so the very name
kind of implies that it's about the behaviot of the _function_ more
than the type of the return value.

And the "enumerate the arguments" of __alloc_size() makes it 100%
clear that it has absolutely NOTHING to do with the return type, and
is all about the function itself and which arguments give the size.

So the attribute goes at the end, not the front.

Are these a bit arbitrary? Sure. And because it's not checked, it's
not consistent. But I can only repeat: this is literally how the
compiler docs themselves tend to order things, pointed to in the very
patches that are under discussion.

So the rules may be arbitrary, but they are at least _somewhat_
internally consistent. Yes, you can always argue about whether some
behavior is about the returned type or whether it is about the
semantics of the function.

         Linus

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [patch 9/9] mm/vmalloc: add __alloc_size attributes for better bounds checking
@ 2021-09-10 20:16         ` Linus Torvalds
  0 siblings, 0 replies; 49+ messages in thread
From: Linus Torvalds @ 2021-09-10 20:16 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Andrew Morton, apw, Christoph Lameter, Daniel Micay, Dennis Zhou,
	dwaipayanray1, Joonsoo Kim, Joe Perches, Kees Cook, Linux-MM,
	Lukas Bulwahn, mm-commits, Nathan Chancellor, Miguel Ojeda,
	Pekka Enberg, David Rientjes, Tejun Heo, Vlastimil Babka

On Fri, Sep 10, 2021 at 12:49 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> Huh?
> $ grep -rn __always_inline
> $ grep -rn noinline
> $ grep -rn __cold

Those are all examples of basically storage classes.

And yes, they go to the front - generally even before the return
value. Exactly like 'extern' and 'static'.

The In contrast, the "function argument descriptions" have
traditionally gone at the end, although you can certainly finds us
screwing that up too.

This, btw, tends to be how the compiler documentation also does it.

So to a close approximation

 - "storage class" goes first, so "static inline" etc.

 - return type next (including attributes directly related to the
returned value - like "__must_check")

 - then function name and argument declaration

 - and finally the "function argument type attributes" at the end.

can you do it in different orders? Yes. And the compiler won't even
generally warn about it. So we've gotten it wrong many many times.

I mean, compilers won't complain even about clear garbage that is _so_
bad that we generally get it right:

  int static myfn(void);

will build perfectly fine. That most certainly doesn't make it right.

Arguably "__malloc" could be seen about the returned type, rather than
being about the function declarations. But if it was about the
returned type, you'd call it a "restrict" pointer, so the very name
kind of implies that it's about the behaviot of the _function_ more
than the type of the return value.

And the "enumerate the arguments" of __alloc_size() makes it 100%
clear that it has absolutely NOTHING to do with the return type, and
is all about the function itself and which arguments give the size.

So the attribute goes at the end, not the front.

Are these a bit arbitrary? Sure. And because it's not checked, it's
not consistent. But I can only repeat: this is literally how the
compiler docs themselves tend to order things, pointed to in the very
patches that are under discussion.

So the rules may be arbitrary, but they are at least _somewhat_
internally consistent. Yes, you can always argue about whether some
behavior is about the returned type or whether it is about the
semantics of the function.

         Linus


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [patch 9/9] mm/vmalloc: add __alloc_size attributes for better bounds checking
  2021-09-10 20:16         ` Linus Torvalds
  (?)
@ 2021-09-10 20:47         ` Kees Cook
  2021-09-10 20:58             ` Nick Desaulniers
  -1 siblings, 1 reply; 49+ messages in thread
From: Kees Cook @ 2021-09-10 20:47 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Nick Desaulniers, Andrew Morton, apw, Christoph Lameter,
	Daniel Micay, Dennis Zhou, dwaipayanray1, Joonsoo Kim,
	Joe Perches, Linux-MM, Lukas Bulwahn, mm-commits,
	Nathan Chancellor, Miguel Ojeda, Pekka Enberg, David Rientjes,
	Tejun Heo, Vlastimil Babka

On Fri, Sep 10, 2021 at 01:16:00PM -0700, Linus Torvalds wrote:
> So to a close approximation
> 
>  - "storage class" goes first, so "static inline" etc.
> 
>  - return type next (including attributes directly related to the
> returned value - like "__must_check")
> 
>  - then function name and argument declaration
> 
>  - and finally the "function argument type attributes" at the end.

I'm going to eventually forget this thread, so I want to get it into
our coding style so I can find it again more easily. :) How does this
look?

diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
index 42969ab37b34..3c72f0232f02 100644
--- a/Documentation/process/coding-style.rst
+++ b/Documentation/process/coding-style.rst
@@ -487,6 +487,29 @@ because it is a simple way to add valuable information for the reader.
 Do not use the ``extern`` keyword with function prototypes as this makes
 lines longer and isn't strictly necessary.
 
+.. code-block:: c
+
+	static __always_inline __must_check void *action(enum magic value,
+							 size_t size, u8 count,
+							 char *buffer)
+							__alloc_size(2, 3)
+	{
+		...
+	}
+
+When writing a function prototype, keep the order of elements regular. The
+desired order is "storage class", "return type attributes", "return
+type", name, arguments (as described earlier), followed by "function
+argument attributes". In the ``action`` function example above, ``static
+__always_inline`` is the "storage class" (even though ``__always_inline``
+is an attribute, it is treated like ``inline``). ``__must_check`` is
+a "return type attribute" (describing ``void *``). ``void *`` is the
+"return type". ``action`` is the function name, followed by the function
+arguments. Finally ``__alloc_size(2,3)`` is an "function argument attribute",
+describing things about the function arguments. Some attributes, like
+``__malloc``, describe the behavior of the function more than they
+describe the function return type, and are more appropriately included
+in the "function argument attributes".
 
 7) Centralized exiting of functions
 -----------------------------------

-- 
Kees Cook

^ permalink raw reply related	[flat|nested] 49+ messages in thread

* Re: [patch 9/9] mm/vmalloc: add __alloc_size attributes for better bounds checking
  2021-09-10 20:47         ` Kees Cook
@ 2021-09-10 20:58             ` Nick Desaulniers
  0 siblings, 0 replies; 49+ messages in thread
From: Nick Desaulniers @ 2021-09-10 20:58 UTC (permalink / raw)
  To: Kees Cook
  Cc: Linus Torvalds, Andrew Morton, apw, Christoph Lameter,
	Daniel Micay, Dennis Zhou, dwaipayanray1, Joonsoo Kim,
	Joe Perches, Linux-MM, Lukas Bulwahn, mm-commits,
	Nathan Chancellor, Miguel Ojeda, Pekka Enberg, David Rientjes,
	Tejun Heo, Vlastimil Babka

On Fri, Sep 10, 2021 at 1:47 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Fri, Sep 10, 2021 at 01:16:00PM -0700, Linus Torvalds wrote:
> > So to a close approximation
> >
> >  - "storage class" goes first, so "static inline" etc.
> >
> >  - return type next (including attributes directly related to the
> > returned value - like "__must_check")
> >
> >  - then function name and argument declaration
> >
> >  - and finally the "function argument type attributes" at the end.
>
> I'm going to eventually forget this thread, so I want to get it into
> our coding style so I can find it again more easily. :) How does this
> look?
>
> diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
> index 42969ab37b34..3c72f0232f02 100644
> --- a/Documentation/process/coding-style.rst
> +++ b/Documentation/process/coding-style.rst
> @@ -487,6 +487,29 @@ because it is a simple way to add valuable information for the reader.
>  Do not use the ``extern`` keyword with function prototypes as this makes
>  lines longer and isn't strictly necessary.
>
> +.. code-block:: c
> +
> +       static __always_inline __must_check void *action(enum magic value,
> +                                                        size_t size, u8 count,
> +                                                        char *buffer)
> +                                                       __alloc_size(2, 3)
> +       {
> +               ...
> +       }
> +
> +When writing a function prototype, keep the order of elements regular. The
> +desired order is "storage class", "return type attributes", "return
> +type", name, arguments (as described earlier), followed by "function
> +argument attributes". In the ``action`` function example above, ``static
> +__always_inline`` is the "storage class" (even though ``__always_inline``
> +is an attribute, it is treated like ``inline``). ``__must_check`` is

eh...mentioning inline as though it was a storage class doesn't seem
precise, but I think this is a good start. Thanks Kees.

Acked-by: Nick Desaulniers <ndesaulniers@google.com>

Worst case, consider "inlining related attributes like __always_inline
and noinline should follow the storage class (static, extern).

> +a "return type attribute" (describing ``void *``). ``void *`` is the
> +"return type". ``action`` is the function name, followed by the function
> +arguments. Finally ``__alloc_size(2,3)`` is an "function argument attribute",
> +describing things about the function arguments. Some attributes, like
> +``__malloc``, describe the behavior of the function more than they
> +describe the function return type, and are more appropriately included
> +in the "function argument attributes".
>
>  7) Centralized exiting of functions
>  -----------------------------------
>
> --
> Kees Cook



-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [patch 9/9] mm/vmalloc: add __alloc_size attributes for better bounds checking
@ 2021-09-10 20:58             ` Nick Desaulniers
  0 siblings, 0 replies; 49+ messages in thread
From: Nick Desaulniers @ 2021-09-10 20:58 UTC (permalink / raw)
  To: Kees Cook
  Cc: Linus Torvalds, Andrew Morton, apw, Christoph Lameter,
	Daniel Micay, Dennis Zhou, dwaipayanray1, Joonsoo Kim,
	Joe Perches, Linux-MM, Lukas Bulwahn, mm-commits,
	Nathan Chancellor, Miguel Ojeda, Pekka Enberg, David Rientjes,
	Tejun Heo, Vlastimil Babka

On Fri, Sep 10, 2021 at 1:47 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Fri, Sep 10, 2021 at 01:16:00PM -0700, Linus Torvalds wrote:
> > So to a close approximation
> >
> >  - "storage class" goes first, so "static inline" etc.
> >
> >  - return type next (including attributes directly related to the
> > returned value - like "__must_check")
> >
> >  - then function name and argument declaration
> >
> >  - and finally the "function argument type attributes" at the end.
>
> I'm going to eventually forget this thread, so I want to get it into
> our coding style so I can find it again more easily. :) How does this
> look?
>
> diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
> index 42969ab37b34..3c72f0232f02 100644
> --- a/Documentation/process/coding-style.rst
> +++ b/Documentation/process/coding-style.rst
> @@ -487,6 +487,29 @@ because it is a simple way to add valuable information for the reader.
>  Do not use the ``extern`` keyword with function prototypes as this makes
>  lines longer and isn't strictly necessary.
>
> +.. code-block:: c
> +
> +       static __always_inline __must_check void *action(enum magic value,
> +                                                        size_t size, u8 count,
> +                                                        char *buffer)
> +                                                       __alloc_size(2, 3)
> +       {
> +               ...
> +       }
> +
> +When writing a function prototype, keep the order of elements regular. The
> +desired order is "storage class", "return type attributes", "return
> +type", name, arguments (as described earlier), followed by "function
> +argument attributes". In the ``action`` function example above, ``static
> +__always_inline`` is the "storage class" (even though ``__always_inline``
> +is an attribute, it is treated like ``inline``). ``__must_check`` is

eh...mentioning inline as though it was a storage class doesn't seem
precise, but I think this is a good start. Thanks Kees.

Acked-by: Nick Desaulniers <ndesaulniers@google.com>

Worst case, consider "inlining related attributes like __always_inline
and noinline should follow the storage class (static, extern).

> +a "return type attribute" (describing ``void *``). ``void *`` is the
> +"return type". ``action`` is the function name, followed by the function
> +arguments. Finally ``__alloc_size(2,3)`` is an "function argument attribute",
> +describing things about the function arguments. Some attributes, like
> +``__malloc``, describe the behavior of the function more than they
> +describe the function return type, and are more appropriately included
> +in the "function argument attributes".
>
>  7) Centralized exiting of functions
>  -----------------------------------
>
> --
> Kees Cook



-- 
Thanks,
~Nick Desaulniers


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [patch 9/9] mm/vmalloc: add __alloc_size attributes for better bounds checking
  2021-09-10 20:58             ` Nick Desaulniers
  (?)
@ 2021-09-10 21:07             ` Kees Cook
  -1 siblings, 0 replies; 49+ messages in thread
From: Kees Cook @ 2021-09-10 21:07 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Linus Torvalds, Andrew Morton, apw, Christoph Lameter,
	Daniel Micay, Dennis Zhou, dwaipayanray1, Joonsoo Kim,
	Joe Perches, Linux-MM, Lukas Bulwahn, mm-commits,
	Nathan Chancellor, Miguel Ojeda, Pekka Enberg, David Rientjes,
	Tejun Heo, Vlastimil Babka

On Fri, Sep 10, 2021 at 01:58:17PM -0700, Nick Desaulniers wrote:
> On Fri, Sep 10, 2021 at 1:47 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Fri, Sep 10, 2021 at 01:16:00PM -0700, Linus Torvalds wrote:
> > > So to a close approximation
> > >
> > >  - "storage class" goes first, so "static inline" etc.
> > >
> > >  - return type next (including attributes directly related to the
> > > returned value - like "__must_check")
> > >
> > >  - then function name and argument declaration
> > >
> > >  - and finally the "function argument type attributes" at the end.
> >
> > I'm going to eventually forget this thread, so I want to get it into
> > our coding style so I can find it again more easily. :) How does this
> > look?
> >
> > diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
> > index 42969ab37b34..3c72f0232f02 100644
> > --- a/Documentation/process/coding-style.rst
> > +++ b/Documentation/process/coding-style.rst
> > @@ -487,6 +487,29 @@ because it is a simple way to add valuable information for the reader.
> >  Do not use the ``extern`` keyword with function prototypes as this makes
> >  lines longer and isn't strictly necessary.
> >
> > +.. code-block:: c
> > +
> > +       static __always_inline __must_check void *action(enum magic value,
> > +                                                        size_t size, u8 count,
> > +                                                        char *buffer)
> > +                                                       __alloc_size(2, 3)
> > +       {
> > +               ...
> > +       }
> > +
> > +When writing a function prototype, keep the order of elements regular. The
> > +desired order is "storage class", "return type attributes", "return
> > +type", name, arguments (as described earlier), followed by "function
> > +argument attributes". In the ``action`` function example above, ``static
> > +__always_inline`` is the "storage class" (even though ``__always_inline``
> > +is an attribute, it is treated like ``inline``). ``__must_check`` is
> 
> eh...mentioning inline as though it was a storage class doesn't seem
> precise, but I think this is a good start. Thanks Kees.

Well, hm, it's kinda like that? "where does it go?" "*everywhere*" :P
In looking at this a little longer, I do wonder about section attributes,
though. __cold is a hint, but ends up being a section attribute. And
section attributes appear to be used in the storage class (i.e.
"noinstr"). We treat "how the function should behave" attributes as
storage classes too, though (e.g. "notrace"). Is that right?

> 
> Acked-by: Nick Desaulniers <ndesaulniers@google.com>
> 
> Worst case, consider "inlining related attributes like __always_inline
> and noinline should follow the storage class (static, extern).
> 
> > +a "return type attribute" (describing ``void *``). ``void *`` is the
> > +"return type". ``action`` is the function name, followed by the function
> > +arguments. Finally ``__alloc_size(2,3)`` is an "function argument attribute",
> > +describing things about the function arguments. Some attributes, like
> > +``__malloc``, describe the behavior of the function more than they
> > +describe the function return type, and are more appropriately included
> > +in the "function argument attributes".
> >
> >  7) Centralized exiting of functions
> >  -----------------------------------
> >
> > --
> > Kees Cook
> 
> 
> 
> -- 
> Thanks,
> ~Nick Desaulniers

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [patch 9/9] mm/vmalloc: add __alloc_size attributes for better bounds checking
  2021-09-10 17:23     ` Linus Torvalds
@ 2021-09-11  5:29       ` Joe Perches
  -1 siblings, 0 replies; 49+ messages in thread
From: Joe Perches @ 2021-09-11  5:29 UTC (permalink / raw)
  To: Linus Torvalds, Andrew Morton
  Cc: apw, Christoph Lameter, Daniel Micay, Dennis Zhou, dwaipayanray1,
	Joonsoo Kim, Kees Cook, Linux-MM, Lukas Bulwahn, mm-commits,
	Nathan Chancellor, Nick Desaulniers, Miguel Ojeda, Pekka Enberg,
	David Rientjes, Tejun Heo, Vlastimil Babka

On Fri, 2021-09-10 at 10:23 -0700, Linus Torvalds wrote:
> On Thu, Sep 9, 2021 at 8:10 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> > 
> > +__alloc_size(1)
> >  extern void *vmalloc(unsigned long size);
[]
> So wouldn't this all have looked much more natural as
> 
>      void *vmalloc(unsigned long size) __malloc(1);
> 
> instead? Hmm?

I think not as the __malloc attribute and in fact all the other function
attributes are effectively uninteresting when it comes to grepping for
function declarations.

Putting the attribute lists after the function arguments in many
cases would just be visual noise.

My preference would be for declarations to be mostly like:

[optional attribute list[
<return type> function(arguments);

Frequently there are multiline function declarations with many
arguments similar to either of

[optional attribute list]
<return type> function(type arg1,
		       type arg2,
		       etc...);

or

[optional attribute list]
<return type>
function(type arg1,
	 type arg2,
	 etc...);

which always makes grep rather difficult.

And given the expansion is naming lengths there are more and more
of these multiline argument lists.  It doesn't matter if the line
length is increased above 80 columns or not.





^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [patch 9/9] mm/vmalloc: add __alloc_size attributes for better bounds checking
@ 2021-09-11  5:29       ` Joe Perches
  0 siblings, 0 replies; 49+ messages in thread
From: Joe Perches @ 2021-09-11  5:29 UTC (permalink / raw)
  To: Linus Torvalds, Andrew Morton
  Cc: apw, Christoph Lameter, Daniel Micay, Dennis Zhou, dwaipayanray1,
	Joonsoo Kim, Kees Cook, Linux-MM, Lukas Bulwahn, mm-commits,
	Nathan Chancellor, Nick Desaulniers, Miguel Ojeda, Pekka Enberg,
	David Rientjes, Tejun Heo, Vlastimil Babka

On Fri, 2021-09-10 at 10:23 -0700, Linus Torvalds wrote:
> On Thu, Sep 9, 2021 at 8:10 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> > 
> > +__alloc_size(1)
> >  extern void *vmalloc(unsigned long size);
[]
> So wouldn't this all have looked much more natural as
> 
>      void *vmalloc(unsigned long size) __malloc(1);
> 
> instead? Hmm?

I think not as the __malloc attribute and in fact all the other function
attributes are effectively uninteresting when it comes to grepping for
function declarations.

Putting the attribute lists after the function arguments in many
cases would just be visual noise.

My preference would be for declarations to be mostly like:

[optional attribute list[
<return type> function(arguments);

Frequently there are multiline function declarations with many
arguments similar to either of

[optional attribute list]
<return type> function(type arg1,
		       type arg2,
		       etc...);

or

[optional attribute list]
<return type>
function(type arg1,
	 type arg2,
	 etc...);

which always makes grep rather difficult.

And given the expansion is naming lengths there are more and more
of these multiline argument lists.  It doesn't matter if the line
length is increased above 80 columns or not.




^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [patch 9/9] mm/vmalloc: add __alloc_size attributes for better bounds checking
  2021-09-10 17:23     ` Linus Torvalds
                       ` (3 preceding siblings ...)
  (?)
@ 2021-09-21 23:37     ` Kees Cook
  2021-09-21 23:45         ` Joe Perches
  -1 siblings, 1 reply; 49+ messages in thread
From: Kees Cook @ 2021-09-21 23:37 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andrew Morton, apw, Christoph Lameter, Daniel Micay, Dennis Zhou,
	dwaipayanray1, Joonsoo Kim, Joe Perches, Linux-MM, Lukas Bulwahn,
	mm-commits, Nathan Chancellor, Nick Desaulniers, Miguel Ojeda,
	Pekka Enberg, David Rientjes, Tejun Heo, Vlastimil Babka

On Fri, Sep 10, 2021 at 10:23:48AM -0700, Linus Torvalds wrote:
> On Thu, Sep 9, 2021 at 8:10 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > +__alloc_size(1)
> >  extern void *vmalloc(unsigned long size);
> [...]
> 
> All of these are added in the wrong place - inconsistent with the very
> compiler documentation the patches add.
> 
> The function attributes are generally added _after_ the function,
> although admittedly we've been quite confused here before.
> 
> But the very compiler documentation you point to in the patch that
> adds these macros gives that as the examples both for gcc and clang:
> 
> + *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-alloc_005fsize-function-attribute
> + * clang: https://clang.llvm.org/docs/AttributeReference.html#alloc-size
> 
> and honestly I think that is the preferred format because this is
> about the *function*, not about the return type.
> 
> Do both placements work? Yes.

I'm cleaning this up now, and have discovered that the reason for the
before-function placement is consistency with static inlines. If I do this:

static __always_inline void * kmalloc(size_t size, gfp_t flags) __alloc_size(1)
{
	...
}

GCC is very angry:

./include/linux/slab.h:519:1: error: attributes should be specified before the declarator in a function definition
  519 | static __always_inline void *kmalloc_large(size_t size, gfp_t flags) __alloc_size(1)
      | ^~~~~~

It's happy if I treat it as a "return type attribute" in the ordering,
though:

static __always_inline void * __alloc_size(1) kmalloc(size_t size, gfp_t flags)

I'll do that unless you have a preference for somewhere else...

-Kees

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [patch 9/9] mm/vmalloc: add __alloc_size attributes for better bounds checking
  2021-09-21 23:37     ` Kees Cook
@ 2021-09-21 23:45         ` Joe Perches
  0 siblings, 0 replies; 49+ messages in thread
From: Joe Perches @ 2021-09-21 23:45 UTC (permalink / raw)
  To: Kees Cook, linux-kernel
  Cc: Andrew Morton, apw, Christoph Lameter, Daniel Micay, Dennis Zhou,
	dwaipayanray1, Joonsoo Kim, Linux-MM, Lukas Bulwahn, mm-commits,
	Nathan Chancellor, Nick Desaulniers, Miguel Ojeda, Pekka Enberg,
	David Rientjes, Tejun Heo, Vlastimil Babka

On Tue, 2021-09-21 at 16:37 -0700, Kees Cook wrote:
> On Fri, Sep 10, 2021 at 10:23:48AM -0700, Linus Torvalds wrote:
> > On Thu, Sep 9, 2021 at 8:10 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> > > 
> > > +__alloc_size(1)
> > >  extern void *vmalloc(unsigned long size);
> > [...]
> > 
> > All of these are added in the wrong place - inconsistent with the very
> > compiler documentation the patches add.
> > 
> > The function attributes are generally added _after_ the function,
> > although admittedly we've been quite confused here before.
> > 
> > But the very compiler documentation you point to in the patch that
> > adds these macros gives that as the examples both for gcc and clang:
> > 
> > + *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-alloc_005fsize-function-attribute
> > + * clang: https://clang.llvm.org/docs/AttributeReference.html#alloc-size
> > 
> > and honestly I think that is the preferred format because this is
> > about the *function*, not about the return type.
> > 
> > Do both placements work? Yes.
> 
> I'm cleaning this up now, and have discovered that the reason for the
> before-function placement is consistency with static inlines. If I do this:
> 
> static __always_inline void * kmalloc(size_t size, gfp_t flags) __alloc_size(1)
> {
> 	...
> }
> 
> GCC is very angry:
> 
> ./include/linux/slab.h:519:1: error: attributes should be specified before the declarator in a function definition
>   519 | static __always_inline void *kmalloc_large(size_t size, gfp_t flags) __alloc_size(1)
>       | ^~~~~~
> 
> It's happy if I treat it as a "return type attribute" in the ordering,
> though:
> 
> static __always_inline void * __alloc_size(1) kmalloc(size_t size, gfp_t flags)
> 
> I'll do that unless you have a preference for somewhere else...

_please_ put it before the return type on a separate line.

[__attributes]
[static inline const] <return type> function(<args...>)





^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [patch 9/9] mm/vmalloc: add __alloc_size attributes for better bounds checking
@ 2021-09-21 23:45         ` Joe Perches
  0 siblings, 0 replies; 49+ messages in thread
From: Joe Perches @ 2021-09-21 23:45 UTC (permalink / raw)
  To: Kees Cook, linux-kernel
  Cc: Andrew Morton, apw, Christoph Lameter, Daniel Micay, Dennis Zhou,
	dwaipayanray1, Joonsoo Kim, Linux-MM, Lukas Bulwahn, mm-commits,
	Nathan Chancellor, Nick Desaulniers, Miguel Ojeda, Pekka Enberg,
	David Rientjes, Tejun Heo, Vlastimil Babka

On Tue, 2021-09-21 at 16:37 -0700, Kees Cook wrote:
> On Fri, Sep 10, 2021 at 10:23:48AM -0700, Linus Torvalds wrote:
> > On Thu, Sep 9, 2021 at 8:10 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> > > 
> > > +__alloc_size(1)
> > >  extern void *vmalloc(unsigned long size);
> > [...]
> > 
> > All of these are added in the wrong place - inconsistent with the very
> > compiler documentation the patches add.
> > 
> > The function attributes are generally added _after_ the function,
> > although admittedly we've been quite confused here before.
> > 
> > But the very compiler documentation you point to in the patch that
> > adds these macros gives that as the examples both for gcc and clang:
> > 
> > + *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-alloc_005fsize-function-attribute
> > + * clang: https://clang.llvm.org/docs/AttributeReference.html#alloc-size
> > 
> > and honestly I think that is the preferred format because this is
> > about the *function*, not about the return type.
> > 
> > Do both placements work? Yes.
> 
> I'm cleaning this up now, and have discovered that the reason for the
> before-function placement is consistency with static inlines. If I do this:
> 
> static __always_inline void * kmalloc(size_t size, gfp_t flags) __alloc_size(1)
> {
> 	...
> }
> 
> GCC is very angry:
> 
> ./include/linux/slab.h:519:1: error: attributes should be specified before the declarator in a function definition
>   519 | static __always_inline void *kmalloc_large(size_t size, gfp_t flags) __alloc_size(1)
>       | ^~~~~~
> 
> It's happy if I treat it as a "return type attribute" in the ordering,
> though:
> 
> static __always_inline void * __alloc_size(1) kmalloc(size_t size, gfp_t flags)
> 
> I'll do that unless you have a preference for somewhere else...

_please_ put it before the return type on a separate line.

[__attributes]
[static inline const] <return type> function(<args...>)






^ permalink raw reply	[flat|nested] 49+ messages in thread

* function prototype element ordering
  2021-09-21 23:45         ` Joe Perches
  (?)
@ 2021-09-22  2:25         ` Kees Cook
  2021-09-22  4:24             ` Joe Perches
  2021-09-22  7:24           ` Alexey Dobriyan
  -1 siblings, 2 replies; 49+ messages in thread
From: Kees Cook @ 2021-09-22  2:25 UTC (permalink / raw)
  To: Linus Torvalds, Joe Perches
  Cc: linux-kernel, Andrew Morton, apw, Christoph Lameter,
	Daniel Micay, Dennis Zhou, dwaipayanray1, Joonsoo Kim, Linux-MM,
	Lukas Bulwahn, mm-commits, Nathan Chancellor, Nick Desaulniers,
	Miguel Ojeda, Pekka Enberg, David Rientjes, Tejun Heo,
	Vlastimil Babka, linux-doc

On Tue, Sep 21, 2021 at 04:45:44PM -0700, Joe Perches wrote:
> On Tue, 2021-09-21 at 16:37 -0700, Kees Cook wrote:
> > On Fri, Sep 10, 2021 at 10:23:48AM -0700, Linus Torvalds wrote:
> > > On Thu, Sep 9, 2021 at 8:10 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> > > > 
> > > > +__alloc_size(1)
> > > >  extern void *vmalloc(unsigned long size);
> > > [...]
> > > 
> > > All of these are added in the wrong place - inconsistent with the very
> > > compiler documentation the patches add.
> > > 
> > > The function attributes are generally added _after_ the function,
> > > although admittedly we've been quite confused here before.
> > > 
> > > But the very compiler documentation you point to in the patch that
> > > adds these macros gives that as the examples both for gcc and clang:
> > > 
> > > + *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-alloc_005fsize-function-attribute
> > > + * clang: https://clang.llvm.org/docs/AttributeReference.html#alloc-size
> > > 
> > > and honestly I think that is the preferred format because this is
> > > about the *function*, not about the return type.
> > > 
> > > Do both placements work? Yes.
> > 
> > I'm cleaning this up now, and have discovered that the reason for the
> > before-function placement is consistency with static inlines. If I do this:
> > 
> > static __always_inline void * kmalloc(size_t size, gfp_t flags) __alloc_size(1)
> > {
> > 	...
> > }
> > 
> > GCC is very angry:
> > 
> > ./include/linux/slab.h:519:1: error: attributes should be specified before the declarator in a function definition
> >   519 | static __always_inline void *kmalloc_large(size_t size, gfp_t flags) __alloc_size(1)
> >       | ^~~~~~
> > 
> > It's happy if I treat it as a "return type attribute" in the ordering,
> > though:
> > 
> > static __always_inline void * __alloc_size(1) kmalloc(size_t size, gfp_t flags)
> > 
> > I'll do that unless you have a preference for somewhere else...
> 
> _please_ put it before the return type on a separate line.
> 
> [__attributes]
> [static inline const] <return type> function(<args...>)

Somehow Linus wasn't in CC. :P

Linus, what do you want here? I keep getting conflicting (or
uncompilable) advice. I'm also trying to prepare a patch for
Documentation/process/coding-style.rst ...

Looking through what was written before[1] and through examples in the
source tree, I find the following categories:

1- storage class: static extern inline __always_inline
2- storage class attributes/hints/???: __init __cold
3- return type: void *
4- return type attributes: __must_check __noreturn __assume_aligned(n)
5- function attributes: __attribute_const__ __malloc
6- function argument attributes: __printf(n, m) __alloc_size(n)

Everyone seems to basically agree on:

[storage class] [return type] [return type attributes] [name]([arg1type] [arg1name], ...)

There is a lot of disagreement over where 5 and 6 should fit in above. And
there is a lot of confusion over 4 (mixed between before and after the
function name) and 2 (see below).

What's currently blocking me is that 6 cannot go after the function
(for definitions) because it angers GCC (see quoted bit above), but 5
can (e.g. __attribute_const__).

Another inconsistency seems to be 2 (mainly section markings like
__init). Sometimes it's after the storage class and sometimes after the
return type, but it certainly feels more like a storage class than a
return type attribute:

$ git grep 'static __init int' | wc -l
349
$ git grep 'static int __init' | wc -l
8402

But it's clearly positioned like a return type attribute in most of the
tree. What's correct?

Regardless, given the constraints above, it seems like what Linus may
want is (on "one line", though it will get wrapped in pathological cases
like kmem_cache_alloc_node_trace):

[storage class] [storage class attributes] [return type] [return type attributes] [function argument attributes] [name]([arg1type] [arg1name], ...) [function attributes]

Joe appears to want (on two lines):

[storage class attributes] [function attributes] [function argument attributes]
[storage class] [return type] [return type attributes] [name]([arg1type] [arg1name], ...)

I would just like to have an arrangement that won't get NAKed by
someone. ;) And I'm willing to document it. :)

-Kees

[1] https://lore.kernel.org/mm-commits/CAHk-=wiOCLRny5aifWNhr621kYrJwhfURsa0vFPeUEm8mF0ufg@mail.gmail.com/

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: function prototype element ordering
  2021-09-22  2:25         ` function prototype element ordering Kees Cook
@ 2021-09-22  4:24             ` Joe Perches
  2021-09-22  7:24           ` Alexey Dobriyan
  1 sibling, 0 replies; 49+ messages in thread
From: Joe Perches @ 2021-09-22  4:24 UTC (permalink / raw)
  To: Kees Cook, Linus Torvalds
  Cc: linux-kernel, Andrew Morton, apw, Christoph Lameter,
	Daniel Micay, Dennis Zhou, dwaipayanray1, Joonsoo Kim, Linux-MM,
	Lukas Bulwahn, mm-commits, Nathan Chancellor, Nick Desaulniers,
	Miguel Ojeda, Pekka Enberg, David Rientjes, Tejun Heo,
	Vlastimil Babka, linux-doc

On Tue, 2021-09-21 at 19:25 -0700, Kees Cook wrote:
> On Tue, Sep 21, 2021 at 04:45:44PM -0700, Joe Perches wrote:
> > On Tue, 2021-09-21 at 16:37 -0700, Kees Cook wrote:
> > > On Fri, Sep 10, 2021 at 10:23:48AM -0700, Linus Torvalds wrote:
> > > > On Thu, Sep 9, 2021 at 8:10 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> > > > > 
> > > > > +__alloc_size(1)
> > > > >  extern void *vmalloc(unsigned long size);
> > > > [...]
> > > > 
> > > > All of these are added in the wrong place - inconsistent with the very
> > > > compiler documentation the patches add.
> > > > 
> > > > The function attributes are generally added _after_ the function,
> > > > although admittedly we've been quite confused here before.
> > > > 
> > > > But the very compiler documentation you point to in the patch that
> > > > adds these macros gives that as the examples both for gcc and clang:
> > > > 
> > > > + *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-alloc_005fsize-function-attribute
> > > > + * clang: https://clang.llvm.org/docs/AttributeReference.html#alloc-size
> > > > 
> > > > and honestly I think that is the preferred format because this is
> > > > about the *function*, not about the return type.
> > > > 
> > > > Do both placements work? Yes.
> > > 
> > > I'm cleaning this up now, and have discovered that the reason for the
> > > before-function placement is consistency with static inlines. If I do this:
> > > 
> > > static __always_inline void * kmalloc(size_t size, gfp_t flags) __alloc_size(1)
> > > {
> > > 	...
> > > }
> > > 
> > > GCC is very angry:
> > > 
> > > ./include/linux/slab.h:519:1: error: attributes should be specified before the declarator in a function definition
> > >   519 | static __always_inline void *kmalloc_large(size_t size, gfp_t flags) __alloc_size(1)
> > >       | ^~~~~~
> > > 
> > > It's happy if I treat it as a "return type attribute" in the ordering,
> > > though:
> > > 
> > > static __always_inline void * __alloc_size(1) kmalloc(size_t size, gfp_t flags)
> > > 
> > > I'll do that unless you have a preference for somewhere else...
> > 
> > _please_ put it before the return type on a separate line.
> > 
> > [__attributes]
> > [static inline const] <return type> function(<args...>)
> 
> Somehow Linus wasn't in CC. :P
> 
> Linus, what do you want here? I keep getting conflicting (or
> uncompilable) advice. I'm also trying to prepare a patch for
> Documentation/process/coding-style.rst ...
> 
> Looking through what was written before[1] and through examples in the
> source tree, I find the following categories:
> 
> 1- storage class: static extern inline __always_inline
> 2- storage class attributes/hints/???: __init __cold
> 3- return type: void *
> 4- return type attributes: __must_check __noreturn __assume_aligned(n)
> 5- function attributes: __attribute_const__ __malloc
> 6- function argument attributes: __printf(n, m) __alloc_size(n)
> 
> Everyone seems to basically agree on:
> 
> [storage class] [return type] [return type attributes] [name]([arg1type] [arg1name], ...)
> 
> There is a lot of disagreement over where 5 and 6 should fit in above. And
> there is a lot of confusion over 4 (mixed between before and after the
> function name) and 2 (see below).
> 
> What's currently blocking me is that 6 cannot go after the function
> (for definitions) because it angers GCC (see quoted bit above), but 5
> can (e.g. __attribute_const__).
> 
> Another inconsistency seems to be 2 (mainly section markings like
> __init). Sometimes it's after the storage class and sometimes after the
> return type, but it certainly feels more like a storage class than a
> return type attribute:
> 
> $ git grep 'static __init int' | wc -l
> 349
> $ git grep 'static int __init' | wc -l
> 8402
> 
> But it's clearly positioned like a return type attribute in most of the
> tree. What's correct?

Neither really.  'Correct' is such a difficult concept.
'Preferred' might be better.

btw: there are about another 100 other uses with '__init' as the
initial attribute, mostly in trace.

And I still think that return type attributes like __init, which is
just a __section define, should go before the function storage class
and ideally on a separate line to simplify the parsing of the actual
function declaration.  Attributes like __section, __aligned, __cold,
etc... don't have much value when looking up a function definition.

> Regardless, given the constraints above, it seems like what Linus may
> want is (on "one line", though it will get wrapped in pathological cases
> like kmem_cache_alloc_node_trace):

Pathological is pretty common these days as the function name length
is rather longer now than earlier times.
 
> [storage class] [storage class attributes] [return type] [return type attributes] [function argument attributes] [name]([arg1type] [arg1name], ...) [function attributes]
> 
> Joe appears to want (on two lines):
> 
> [storage class attributes] [function attributes] [function argument attributes]
> [storage class] [return type] [return type attributes] [name]([arg1type] [arg1name], ...)

I would put [return type attributes] on the initial separate line
even though that's not the most common use today.

> I would just like to have an arrangement that won't get NAKed by
> someone. ;)

Bikeshed building dreamer...

btw:

Scouting through kernel code for frequency of use examples really
should have some age of code checking associated to the use.

Older code was far more freeform than more recently written code.

But IMO the desire here is to ask for a bit more uniformity, not
require it.



^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: function prototype element ordering
@ 2021-09-22  4:24             ` Joe Perches
  0 siblings, 0 replies; 49+ messages in thread
From: Joe Perches @ 2021-09-22  4:24 UTC (permalink / raw)
  To: Kees Cook, Linus Torvalds
  Cc: linux-kernel, Andrew Morton, apw, Christoph Lameter,
	Daniel Micay, Dennis Zhou, dwaipayanray1, Joonsoo Kim, Linux-MM,
	Lukas Bulwahn, mm-commits, Nathan Chancellor, Nick Desaulniers,
	Miguel Ojeda, Pekka Enberg, David Rientjes, Tejun Heo,
	Vlastimil Babka, linux-doc

On Tue, 2021-09-21 at 19:25 -0700, Kees Cook wrote:
> On Tue, Sep 21, 2021 at 04:45:44PM -0700, Joe Perches wrote:
> > On Tue, 2021-09-21 at 16:37 -0700, Kees Cook wrote:
> > > On Fri, Sep 10, 2021 at 10:23:48AM -0700, Linus Torvalds wrote:
> > > > On Thu, Sep 9, 2021 at 8:10 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> > > > > 
> > > > > +__alloc_size(1)
> > > > >  extern void *vmalloc(unsigned long size);
> > > > [...]
> > > > 
> > > > All of these are added in the wrong place - inconsistent with the very
> > > > compiler documentation the patches add.
> > > > 
> > > > The function attributes are generally added _after_ the function,
> > > > although admittedly we've been quite confused here before.
> > > > 
> > > > But the very compiler documentation you point to in the patch that
> > > > adds these macros gives that as the examples both for gcc and clang:
> > > > 
> > > > + *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-alloc_005fsize-function-attribute
> > > > + * clang: https://clang.llvm.org/docs/AttributeReference.html#alloc-size
> > > > 
> > > > and honestly I think that is the preferred format because this is
> > > > about the *function*, not about the return type.
> > > > 
> > > > Do both placements work? Yes.
> > > 
> > > I'm cleaning this up now, and have discovered that the reason for the
> > > before-function placement is consistency with static inlines. If I do this:
> > > 
> > > static __always_inline void * kmalloc(size_t size, gfp_t flags) __alloc_size(1)
> > > {
> > > 	...
> > > }
> > > 
> > > GCC is very angry:
> > > 
> > > ./include/linux/slab.h:519:1: error: attributes should be specified before the declarator in a function definition
> > >   519 | static __always_inline void *kmalloc_large(size_t size, gfp_t flags) __alloc_size(1)
> > >       | ^~~~~~
> > > 
> > > It's happy if I treat it as a "return type attribute" in the ordering,
> > > though:
> > > 
> > > static __always_inline void * __alloc_size(1) kmalloc(size_t size, gfp_t flags)
> > > 
> > > I'll do that unless you have a preference for somewhere else...
> > 
> > _please_ put it before the return type on a separate line.
> > 
> > [__attributes]
> > [static inline const] <return type> function(<args...>)
> 
> Somehow Linus wasn't in CC. :P
> 
> Linus, what do you want here? I keep getting conflicting (or
> uncompilable) advice. I'm also trying to prepare a patch for
> Documentation/process/coding-style.rst ...
> 
> Looking through what was written before[1] and through examples in the
> source tree, I find the following categories:
> 
> 1- storage class: static extern inline __always_inline
> 2- storage class attributes/hints/???: __init __cold
> 3- return type: void *
> 4- return type attributes: __must_check __noreturn __assume_aligned(n)
> 5- function attributes: __attribute_const__ __malloc
> 6- function argument attributes: __printf(n, m) __alloc_size(n)
> 
> Everyone seems to basically agree on:
> 
> [storage class] [return type] [return type attributes] [name]([arg1type] [arg1name], ...)
> 
> There is a lot of disagreement over where 5 and 6 should fit in above. And
> there is a lot of confusion over 4 (mixed between before and after the
> function name) and 2 (see below).
> 
> What's currently blocking me is that 6 cannot go after the function
> (for definitions) because it angers GCC (see quoted bit above), but 5
> can (e.g. __attribute_const__).
> 
> Another inconsistency seems to be 2 (mainly section markings like
> __init). Sometimes it's after the storage class and sometimes after the
> return type, but it certainly feels more like a storage class than a
> return type attribute:
> 
> $ git grep 'static __init int' | wc -l
> 349
> $ git grep 'static int __init' | wc -l
> 8402
> 
> But it's clearly positioned like a return type attribute in most of the
> tree. What's correct?

Neither really.  'Correct' is such a difficult concept.
'Preferred' might be better.

btw: there are about another 100 other uses with '__init' as the
initial attribute, mostly in trace.

And I still think that return type attributes like __init, which is
just a __section define, should go before the function storage class
and ideally on a separate line to simplify the parsing of the actual
function declaration.  Attributes like __section, __aligned, __cold,
etc... don't have much value when looking up a function definition.

> Regardless, given the constraints above, it seems like what Linus may
> want is (on "one line", though it will get wrapped in pathological cases
> like kmem_cache_alloc_node_trace):

Pathological is pretty common these days as the function name length
is rather longer now than earlier times.
 
> [storage class] [storage class attributes] [return type] [return type attributes] [function argument attributes] [name]([arg1type] [arg1name], ...) [function attributes]
> 
> Joe appears to want (on two lines):
> 
> [storage class attributes] [function attributes] [function argument attributes]
> [storage class] [return type] [return type attributes] [name]([arg1type] [arg1name], ...)

I would put [return type attributes] on the initial separate line
even though that's not the most common use today.

> I would just like to have an arrangement that won't get NAKed by
> someone. ;)

Bikeshed building dreamer...

btw:

Scouting through kernel code for frequency of use examples really
should have some age of code checking associated to the use.

Older code was far more freeform than more recently written code.

But IMO the desire here is to ask for a bit more uniformity, not
require it.




^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: function prototype element ordering
  2021-09-22  2:25         ` function prototype element ordering Kees Cook
  2021-09-22  4:24             ` Joe Perches
@ 2021-09-22  7:24           ` Alexey Dobriyan
  2021-09-22  8:51               ` Joe Perches
                               ` (2 more replies)
  1 sibling, 3 replies; 49+ messages in thread
From: Alexey Dobriyan @ 2021-09-22  7:24 UTC (permalink / raw)
  To: linux-kernel
  Cc: Linus Torvalds, Joe Perches, Andrew Morton, apw,
	Christoph Lameter, Daniel Micay, Dennis Zhou, dwaipayanray1,
	Joonsoo Kim, Linux-MM, Lukas Bulwahn, mm-commits,
	Nathan Chancellor, Nick Desaulniers, Miguel Ojeda, Pekka Enberg,
	David Rientjes, Tejun Heo, Vlastimil Babka, linux-doc

On Tue, Sep 21, 2021 at 07:25:53PM -0700, Kees Cook wrote:
> On Tue, Sep 21, 2021 at 04:45:44PM -0700, Joe Perches wrote:
> > On Tue, 2021-09-21 at 16:37 -0700, Kees Cook wrote:
> > > On Fri, Sep 10, 2021 at 10:23:48AM -0700, Linus Torvalds wrote:
> > > > On Thu, Sep 9, 2021 at 8:10 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> > > > > 
> > > > > +__alloc_size(1)
> > > > >  extern void *vmalloc(unsigned long size);
> > > > [...]
> > > > 
> > > > All of these are added in the wrong place - inconsistent with the very
> > > > compiler documentation the patches add.
> > > > 
> > > > The function attributes are generally added _after_ the function,
> > > > although admittedly we've been quite confused here before.
> > > > 
> > > > But the very compiler documentation you point to in the patch that
> > > > adds these macros gives that as the examples both for gcc and clang:
> > > > 
> > > > + *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-alloc_005fsize-function-attribute
> > > > + * clang: https://clang.llvm.org/docs/AttributeReference.html#alloc-size
> > > > 
> > > > and honestly I think that is the preferred format because this is
> > > > about the *function*, not about the return type.
> > > > 
> > > > Do both placements work? Yes.
> > > 
> > > I'm cleaning this up now, and have discovered that the reason for the
> > > before-function placement is consistency with static inlines. If I do this:
> > > 
> > > static __always_inline void * kmalloc(size_t size, gfp_t flags) __alloc_size(1)
> > > {
> > > 	...
> > > }
> > > 
> > > GCC is very angry:
> > > 
> > > ./include/linux/slab.h:519:1: error: attributes should be specified before the declarator in a function definition
> > >   519 | static __always_inline void *kmalloc_large(size_t size, gfp_t flags) __alloc_size(1)
> > >       | ^~~~~~
> > > 
> > > It's happy if I treat it as a "return type attribute" in the ordering,
> > > though:
> > > 
> > > static __always_inline void * __alloc_size(1) kmalloc(size_t size, gfp_t flags)
> > > 
> > > I'll do that unless you have a preference for somewhere else...
> > 
> > _please_ put it before the return type on a separate line.
> > 
> > [__attributes]
> > [static inline const] <return type> function(<args...>)
> 
> Somehow Linus wasn't in CC. :P
> 
> Linus, what do you want here? I keep getting conflicting (or
> uncompilable) advice. I'm also trying to prepare a patch for
> Documentation/process/coding-style.rst ...
> 
> Looking through what was written before[1] and through examples in the
> source tree, I find the following categories:
> 
> 1- storage class: static extern inline __always_inline
> 2- storage class attributes/hints/???: __init __cold
> 3- return type: void *
> 4- return type attributes: __must_check __noreturn __assume_aligned(n)
> 5- function attributes: __attribute_const__ __malloc
> 6- function argument attributes: __printf(n, m) __alloc_size(n)
> 
> Everyone seems to basically agree on:
> 
> [storage class] [return type] [return type attributes] [name]([arg1type] [arg1name], ...)
> 
> There is a lot of disagreement over where 5 and 6 should fit in above. And
> there is a lot of confusion over 4 (mixed between before and after the
> function name) and 2 (see below).
> 
> What's currently blocking me is that 6 cannot go after the function
> (for definitions) because it angers GCC (see quoted bit above), but 5
> can (e.g. __attribute_const__).
> 
> Another inconsistency seems to be 2 (mainly section markings like
> __init). Sometimes it's after the storage class and sometimes after the
> return type, but it certainly feels more like a storage class than a
> return type attribute:
> 
> $ git grep 'static __init int' | wc -l
> 349
> $ git grep 'static int __init' | wc -l
> 8402
> 
> But it's clearly positioned like a return type attribute in most of the
> tree. What's correct?
> 
> Regardless, given the constraints above, it seems like what Linus may
> want is (on "one line", though it will get wrapped in pathological cases
> like kmem_cache_alloc_node_trace):
> 
> [storage class] [storage class attributes] [return type] [return type attributes] [function argument attributes] [name]([arg1type] [arg1name], ...) [function attributes]
> 
> Joe appears to want (on two lines):
> 
> [storage class attributes] [function attributes] [function argument attributes]
> [storage class] [return type] [return type attributes] [name]([arg1type] [arg1name], ...)
> 
> I would just like to have an arrangement that won't get NAKed by
> someone. ;) And I'm willing to document it. :)

Attributes should be on their own line, they can be quite lengthy.

	__attribute__((...))
	[static] [inline] T f(A1 arg1, ...)
	{
		...
	}

There will be even more attributes in the future, both added by
compilers and developers (const, pure, WUR), so let's make "prototype lane"
for them.

Same for structures:

	__attribute__((packed))
	struct S {
	};

Kernel practice of hiding attributes under defines (__ro_after_init)
breaks ctags which parses the last identifier before semicolon as object
name. Naturally, it is ctags bug, but placing attributes before
declaration will autmatically unbreak such cases.

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: function prototype element ordering
  2021-09-22  7:24           ` Alexey Dobriyan
@ 2021-09-22  8:51               ` Joe Perches
  2021-09-22 11:19             ` Jani Nikula
  2021-09-22 21:15               ` Linus Torvalds
  2 siblings, 0 replies; 49+ messages in thread
From: Joe Perches @ 2021-09-22  8:51 UTC (permalink / raw)
  To: Alexey Dobriyan, linux-kernel
  Cc: Linus Torvalds, Andrew Morton, apw, Christoph Lameter,
	Daniel Micay, Dennis Zhou, dwaipayanray1, Joonsoo Kim, Linux-MM,
	Lukas Bulwahn, mm-commits, Nathan Chancellor, Nick Desaulniers,
	Miguel Ojeda, Pekka Enberg, David Rientjes, Tejun Heo,
	Vlastimil Babka, linux-doc

On Wed, 2021-09-22 at 10:24 +0300, Alexey Dobriyan wrote:

> Attributes should be on their own line, they can be quite lengthy.
> 
> 	__attribute__((...))
> 	[static] [inline] T f(A1 arg1, ...)
> 	{
> 		...
> 	}
> 
> There will be even more attributes in the future, both added by
> compilers and developers (const, pure, WUR), so let's make "prototype lane"
> for them.
> 
> Same for structures:
> 
> 	__attribute__((packed))
> 	struct S {
> 	};

Do you know if placing attributes like __packed/__aligned() before
definitions would work for all cases for structs/substructs/unions?



^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: function prototype element ordering
@ 2021-09-22  8:51               ` Joe Perches
  0 siblings, 0 replies; 49+ messages in thread
From: Joe Perches @ 2021-09-22  8:51 UTC (permalink / raw)
  To: Alexey Dobriyan, linux-kernel
  Cc: Linus Torvalds, Andrew Morton, apw, Christoph Lameter,
	Daniel Micay, Dennis Zhou, dwaipayanray1, Joonsoo Kim, Linux-MM,
	Lukas Bulwahn, mm-commits, Nathan Chancellor, Nick Desaulniers,
	Miguel Ojeda, Pekka Enberg, David Rientjes, Tejun Heo,
	Vlastimil Babka, linux-doc

On Wed, 2021-09-22 at 10:24 +0300, Alexey Dobriyan wrote:

> Attributes should be on their own line, they can be quite lengthy.
> 
> 	__attribute__((...))
> 	[static] [inline] T f(A1 arg1, ...)
> 	{
> 		...
> 	}
> 
> There will be even more attributes in the future, both added by
> compilers and developers (const, pure, WUR), so let's make "prototype lane"
> for them.
> 
> Same for structures:
> 
> 	__attribute__((packed))
> 	struct S {
> 	};

Do you know if placing attributes like __packed/__aligned() before
definitions would work for all cases for structs/substructs/unions?




^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: function prototype element ordering
  2021-09-22  8:51               ` Joe Perches
  (?)
@ 2021-09-22 10:45               ` Alexey Dobriyan
  -1 siblings, 0 replies; 49+ messages in thread
From: Alexey Dobriyan @ 2021-09-22 10:45 UTC (permalink / raw)
  To: Joe Perches
  Cc: linux-kernel, Linus Torvalds, Andrew Morton, apw,
	Christoph Lameter, Daniel Micay, Dennis Zhou, dwaipayanray1,
	Joonsoo Kim, Linux-MM, Lukas Bulwahn, mm-commits,
	Nathan Chancellor, Nick Desaulniers, Miguel Ojeda, Pekka Enberg,
	David Rientjes, Tejun Heo, Vlastimil Babka, linux-doc

On Wed, Sep 22, 2021 at 01:51:34AM -0700, Joe Perches wrote:
> On Wed, 2021-09-22 at 10:24 +0300, Alexey Dobriyan wrote:
> 
> > Attributes should be on their own line, they can be quite lengthy.
> > 
> > 	__attribute__((...))
> > 	[static] [inline] T f(A1 arg1, ...)
> > 	{
> > 		...
> > 	}
> > 
> > There will be even more attributes in the future, both added by
> > compilers and developers (const, pure, WUR), so let's make "prototype lane"
> > for them.
> > 
> > Same for structures:
> > 
> > 	__attribute__((packed))
> > 	struct S {
> > 	};
> 
> Do you know if placing attributes like __packed/__aligned() before
> definitions would work for all cases for structs/substructs/unions?

Somehow, it doesn't.

But it works for members:

	struct S {
       		__attribute__((aligned(16)))
	        int a;
	};

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: function prototype element ordering
  2021-09-22  7:24           ` Alexey Dobriyan
  2021-09-22  8:51               ` Joe Perches
@ 2021-09-22 11:19             ` Jani Nikula
  2021-09-22 21:15               ` Linus Torvalds
  2 siblings, 0 replies; 49+ messages in thread
From: Jani Nikula @ 2021-09-22 11:19 UTC (permalink / raw)
  To: Alexey Dobriyan, linux-kernel
  Cc: Linus Torvalds, Joe Perches, Andrew Morton, apw,
	Christoph Lameter, Daniel Micay, Dennis Zhou, dwaipayanray1,
	Joonsoo Kim, Linux-MM, Lukas Bulwahn, mm-commits,
	Nathan Chancellor, Nick Desaulniers, Miguel Ojeda, Pekka Enberg,
	David Rientjes, Tejun Heo, Vlastimil Babka, linux-doc

On Wed, 22 Sep 2021, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> On Tue, Sep 21, 2021 at 07:25:53PM -0700, Kees Cook wrote:
>> On Tue, Sep 21, 2021 at 04:45:44PM -0700, Joe Perches wrote:
>> > On Tue, 2021-09-21 at 16:37 -0700, Kees Cook wrote:
>> > > On Fri, Sep 10, 2021 at 10:23:48AM -0700, Linus Torvalds wrote:
>> > > > On Thu, Sep 9, 2021 at 8:10 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>> > > > > 
>> > > > > +__alloc_size(1)
>> > > > >  extern void *vmalloc(unsigned long size);
>> > > > [...]
>> > > > 
>> > > > All of these are added in the wrong place - inconsistent with the very
>> > > > compiler documentation the patches add.
>> > > > 
>> > > > The function attributes are generally added _after_ the function,
>> > > > although admittedly we've been quite confused here before.
>> > > > 
>> > > > But the very compiler documentation you point to in the patch that
>> > > > adds these macros gives that as the examples both for gcc and clang:
>> > > > 
>> > > > + *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-alloc_005fsize-function-attribute
>> > > > + * clang: https://clang.llvm.org/docs/AttributeReference.html#alloc-size
>> > > > 
>> > > > and honestly I think that is the preferred format because this is
>> > > > about the *function*, not about the return type.
>> > > > 
>> > > > Do both placements work? Yes.
>> > > 
>> > > I'm cleaning this up now, and have discovered that the reason for the
>> > > before-function placement is consistency with static inlines. If I do this:
>> > > 
>> > > static __always_inline void * kmalloc(size_t size, gfp_t flags) __alloc_size(1)
>> > > {
>> > > 	...
>> > > }
>> > > 
>> > > GCC is very angry:
>> > > 
>> > > ./include/linux/slab.h:519:1: error: attributes should be specified before the declarator in a function definition
>> > >   519 | static __always_inline void *kmalloc_large(size_t size, gfp_t flags) __alloc_size(1)
>> > >       | ^~~~~~
>> > > 
>> > > It's happy if I treat it as a "return type attribute" in the ordering,
>> > > though:
>> > > 
>> > > static __always_inline void * __alloc_size(1) kmalloc(size_t size, gfp_t flags)
>> > > 
>> > > I'll do that unless you have a preference for somewhere else...
>> > 
>> > _please_ put it before the return type on a separate line.
>> > 
>> > [__attributes]
>> > [static inline const] <return type> function(<args...>)
>> 
>> Somehow Linus wasn't in CC. :P
>> 
>> Linus, what do you want here? I keep getting conflicting (or
>> uncompilable) advice. I'm also trying to prepare a patch for
>> Documentation/process/coding-style.rst ...
>> 
>> Looking through what was written before[1] and through examples in the
>> source tree, I find the following categories:
>> 
>> 1- storage class: static extern inline __always_inline
>> 2- storage class attributes/hints/???: __init __cold
>> 3- return type: void *
>> 4- return type attributes: __must_check __noreturn __assume_aligned(n)
>> 5- function attributes: __attribute_const__ __malloc
>> 6- function argument attributes: __printf(n, m) __alloc_size(n)
>> 
>> Everyone seems to basically agree on:
>> 
>> [storage class] [return type] [return type attributes] [name]([arg1type] [arg1name], ...)
>> 
>> There is a lot of disagreement over where 5 and 6 should fit in above. And
>> there is a lot of confusion over 4 (mixed between before and after the
>> function name) and 2 (see below).
>> 
>> What's currently blocking me is that 6 cannot go after the function
>> (for definitions) because it angers GCC (see quoted bit above), but 5
>> can (e.g. __attribute_const__).
>> 
>> Another inconsistency seems to be 2 (mainly section markings like
>> __init). Sometimes it's after the storage class and sometimes after the
>> return type, but it certainly feels more like a storage class than a
>> return type attribute:
>> 
>> $ git grep 'static __init int' | wc -l
>> 349
>> $ git grep 'static int __init' | wc -l
>> 8402
>> 
>> But it's clearly positioned like a return type attribute in most of the
>> tree. What's correct?
>> 
>> Regardless, given the constraints above, it seems like what Linus may
>> want is (on "one line", though it will get wrapped in pathological cases
>> like kmem_cache_alloc_node_trace):
>> 
>> [storage class] [storage class attributes] [return type] [return type attributes] [function argument attributes] [name]([arg1type] [arg1name], ...) [function attributes]
>> 
>> Joe appears to want (on two lines):
>> 
>> [storage class attributes] [function attributes] [function argument attributes]
>> [storage class] [return type] [return type attributes] [name]([arg1type] [arg1name], ...)
>> 
>> I would just like to have an arrangement that won't get NAKed by
>> someone. ;) And I'm willing to document it. :)
>
> Attributes should be on their own line, they can be quite lengthy.
>
> 	__attribute__((...))
> 	[static] [inline] T f(A1 arg1, ...)
> 	{
> 		...
> 	}
>
> There will be even more attributes in the future, both added by
> compilers and developers (const, pure, WUR), so let's make "prototype lane"
> for them.
>
> Same for structures:
>
> 	__attribute__((packed))
> 	struct S {
> 	};
>
> Kernel practice of hiding attributes under defines (__ro_after_init)
> breaks ctags which parses the last identifier before semicolon as object
> name. Naturally, it is ctags bug, but placing attributes before
> declaration will autmatically unbreak such cases.

git grep seems to suggest __packed is preferred over
__attribute__((packed)), and at the end of the struct declaration
instead of at front:

	struct S {
		/* ... */
        } __packed;

And GNU Global handles this just fine. ;)


BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: function prototype element ordering
  2021-09-22  7:24           ` Alexey Dobriyan
@ 2021-09-22 21:15               ` Linus Torvalds
  2021-09-22 11:19             ` Jani Nikula
  2021-09-22 21:15               ` Linus Torvalds
  2 siblings, 0 replies; 49+ messages in thread
From: Linus Torvalds @ 2021-09-22 21:15 UTC (permalink / raw)
  To: Alexey Dobriyan
  Cc: Linux Kernel Mailing List, Joe Perches, Andrew Morton, apw,
	Christoph Lameter, Daniel Micay, Dennis Zhou, dwaipayanray1,
	Joonsoo Kim, Linux-MM, Lukas Bulwahn, mm-commits,
	Nathan Chancellor, Nick Desaulniers, Miguel Ojeda, Pekka Enberg,
	David Rientjes, Tejun Heo, Vlastimil Babka,
	open list:DOCUMENTATION

On Wed, Sep 22, 2021 at 12:24 AM Alexey Dobriyan <adobriyan@gmail.com> wrote:
>
> Attributes should be on their own line, they can be quite lengthy.

No, no no. They really shouldn't.

First off, no normal code should use that "__attribute__(())" syntax
anyway. It's ugly and big, and many of the attributes are
compiler-specific anyway.

So the "quite lengthy" argument is bogus, because the actual names you
should use are things like "__packed" or "__pure" or "__user" etc.

But the "on their own line" is complete garbage to begin with. That
will NEVER be a kernel rule. We should never have a rule that assumes
things are so long that they need to be on multiple lines.

We don't put function return types on their own lines either, even if
some other projects have that rule (just to get function names at the
beginning of lines or some other odd reason).

So no, attributes do not go on their own lines, and they also
generally don't go before the thing they describe.  Your examples are
wrong, and explicitly against kernel rules.

           Linus

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: function prototype element ordering
@ 2021-09-22 21:15               ` Linus Torvalds
  0 siblings, 0 replies; 49+ messages in thread
From: Linus Torvalds @ 2021-09-22 21:15 UTC (permalink / raw)
  To: Alexey Dobriyan
  Cc: Linux Kernel Mailing List, Joe Perches, Andrew Morton, apw,
	Christoph Lameter, Daniel Micay, Dennis Zhou, dwaipayanray1,
	Joonsoo Kim, Linux-MM, Lukas Bulwahn, mm-commits,
	Nathan Chancellor, Nick Desaulniers, Miguel Ojeda, Pekka Enberg,
	David Rientjes, Tejun Heo, Vlastimil Babka,
	open list:DOCUMENTATION

On Wed, Sep 22, 2021 at 12:24 AM Alexey Dobriyan <adobriyan@gmail.com> wrote:
>
> Attributes should be on their own line, they can be quite lengthy.

No, no no. They really shouldn't.

First off, no normal code should use that "__attribute__(())" syntax
anyway. It's ugly and big, and many of the attributes are
compiler-specific anyway.

So the "quite lengthy" argument is bogus, because the actual names you
should use are things like "__packed" or "__pure" or "__user" etc.

But the "on their own line" is complete garbage to begin with. That
will NEVER be a kernel rule. We should never have a rule that assumes
things are so long that they need to be on multiple lines.

We don't put function return types on their own lines either, even if
some other projects have that rule (just to get function names at the
beginning of lines or some other odd reason).

So no, attributes do not go on their own lines, and they also
generally don't go before the thing they describe.  Your examples are
wrong, and explicitly against kernel rules.

           Linus


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: function prototype element ordering
  2021-09-22 21:15               ` Linus Torvalds
@ 2021-09-23  5:10                 ` Joe Perches
  -1 siblings, 0 replies; 49+ messages in thread
From: Joe Perches @ 2021-09-23  5:10 UTC (permalink / raw)
  To: Linus Torvalds, Alexey Dobriyan
  Cc: Linux Kernel Mailing List, Andrew Morton, apw, Christoph Lameter,
	Daniel Micay, Dennis Zhou, dwaipayanray1, Joonsoo Kim, Linux-MM,
	Lukas Bulwahn, mm-commits, Nathan Chancellor, Nick Desaulniers,
	Miguel Ojeda, Pekka Enberg, David Rientjes, Tejun Heo,
	Vlastimil Babka, open list:DOCUMENTATION

On Wed, 2021-09-22 at 14:15 -0700, Linus Torvalds wrote:
> On Wed, Sep 22, 2021 at 12:24 AM Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > 
> > Attributes should be on their own line, they can be quite lengthy.
> 
> No, no no. They really shouldn't.
> 
> First off, no normal code should use that "__attribute__(())" syntax
> anyway. It's ugly and big, and many of the attributes are
> compiler-specific anyway.
> 
> So the "quite lengthy" argument is bogus, because the actual names you
> should use are things like "__packed" or "__pure" or "__user" etc.
> 
> But the "on their own line" is complete garbage to begin with. That
> will NEVER be a kernel rule. We should never have a rule that assumes
> things are so long that they need to be on multiple lines.

I think it's not so much that lines are long, it's more that the
information provided by these markings aren't particularly useful to
a caller/user of a function.

Under what circumstance is a marking like __pure/__cold or __section
useful to someone that just wants to call a particular function?

A secondary reason why these should be separate or at least put
at the begining of a function declaration is compatibility with
existing tools like ctags.



^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: function prototype element ordering
@ 2021-09-23  5:10                 ` Joe Perches
  0 siblings, 0 replies; 49+ messages in thread
From: Joe Perches @ 2021-09-23  5:10 UTC (permalink / raw)
  To: Linus Torvalds, Alexey Dobriyan
  Cc: Linux Kernel Mailing List, Andrew Morton, apw, Christoph Lameter,
	Daniel Micay, Dennis Zhou, dwaipayanray1, Joonsoo Kim, Linux-MM,
	Lukas Bulwahn, mm-commits, Nathan Chancellor, Nick Desaulniers,
	Miguel Ojeda, Pekka Enberg, David Rientjes, Tejun Heo,
	Vlastimil Babka, open list:DOCUMENTATION

On Wed, 2021-09-22 at 14:15 -0700, Linus Torvalds wrote:
> On Wed, Sep 22, 2021 at 12:24 AM Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > 
> > Attributes should be on their own line, they can be quite lengthy.
> 
> No, no no. They really shouldn't.
> 
> First off, no normal code should use that "__attribute__(())" syntax
> anyway. It's ugly and big, and many of the attributes are
> compiler-specific anyway.
> 
> So the "quite lengthy" argument is bogus, because the actual names you
> should use are things like "__packed" or "__pure" or "__user" etc.
> 
> But the "on their own line" is complete garbage to begin with. That
> will NEVER be a kernel rule. We should never have a rule that assumes
> things are so long that they need to be on multiple lines.

I think it's not so much that lines are long, it's more that the
information provided by these markings aren't particularly useful to
a caller/user of a function.

Under what circumstance is a marking like __pure/__cold or __section
useful to someone that just wants to call a particular function?

A secondary reason why these should be separate or at least put
at the begining of a function declaration is compatibility with
existing tools like ctags.




^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: function prototype element ordering
  2021-09-22  4:24             ` Joe Perches
  (?)
@ 2021-09-24 19:43             ` Kees Cook
  -1 siblings, 0 replies; 49+ messages in thread
From: Kees Cook @ 2021-09-24 19:43 UTC (permalink / raw)
  To: Joe Perches
  Cc: Linus Torvalds, linux-kernel, Andrew Morton, apw,
	Christoph Lameter, Daniel Micay, Dennis Zhou, dwaipayanray1,
	Joonsoo Kim, Linux-MM, Lukas Bulwahn, mm-commits,
	Nathan Chancellor, Nick Desaulniers, Miguel Ojeda, Pekka Enberg,
	David Rientjes, Tejun Heo, Vlastimil Babka, linux-doc

On Tue, Sep 21, 2021 at 09:24:04PM -0700, Joe Perches wrote:
> On Tue, 2021-09-21 at 19:25 -0700, Kees Cook wrote:
> > [...]
> > Looking through what was written before[1] and through examples in the
> > source tree, I find the following categories:
> > 
> > 1- storage class: static extern inline __always_inline
> > 2- storage class attributes/hints/???: __init __cold
> > 3- return type: void *
> > 4- return type attributes: __must_check __noreturn __assume_aligned(n)
> > 5- function attributes: __attribute_const__ __malloc
> > 6- function argument attributes: __printf(n, m) __alloc_size(n)
> > 
> > Everyone seems to basically agree on:
> > 
> > [storage class] [return type] [return type attributes] [name]([arg1type] [arg1name], ...)
> > 
> > There is a lot of disagreement over where 5 and 6 should fit in above. And
> > there is a lot of confusion over 4 (mixed between before and after the
> > function name) and 2 (see below).
> > 
> > What's currently blocking me is that 6 cannot go after the function
> > (for definitions) because it angers GCC (see quoted bit above), but 5
> > can (e.g. __attribute_const__).
> > 
> > Another inconsistency seems to be 2 (mainly section markings like
> > __init). Sometimes it's after the storage class and sometimes after the
> > return type, but it certainly feels more like a storage class than a
> > return type attribute:
> > 
> > $ git grep 'static __init int' | wc -l
> > 349
> > $ git grep 'static int __init' | wc -l
> > 8402
> > 
> > But it's clearly positioned like a return type attribute in most of the
> > tree. What's correct?
> 
> Neither really.  'Correct' is such a difficult concept.
> 'Preferred' might be better.

Right -- I expect it to be a guideline.

> btw: there are about another 100 other uses with '__init' as the
> initial attribute, mostly in trace.

Hah, yeah.

> And I still think that return type attributes like __init, which is
> just a __section define, should go before the function storage class
> and ideally on a separate line to simplify the parsing of the actual
> function declaration.  Attributes like __section, __aligned, __cold,
> etc... don't have much value when looking up a function definition.
> 
> > Regardless, given the constraints above, it seems like what Linus may
> > want is (on "one line", though it will get wrapped in pathological cases
> > like kmem_cache_alloc_node_trace):
> 
> Pathological is pretty common these days as the function name length
> is rather longer now than earlier times.

Agreed!

> > [storage class] [storage class attributes] [return type] [return type attributes] [function argument attributes] [name]([arg1type] [arg1name], ...) [function attributes]
> > 
> > Joe appears to want (on two lines):
> > 
> > [storage class attributes] [function attributes] [function argument attributes]
> > [storage class] [return type] [return type attributes] [name]([arg1type] [arg1name], ...)
> 
> I would put [return type attributes] on the initial separate line
> even though that's not the most common use today.

I found a few other people wanting separate lines too, so at the risk of
annoying Linus, I guess I'll attempt this (again).

> > I would just like to have an arrangement that won't get NAKed by
> > someone. ;)
> 
> Bikeshed building dreamer...

I just want to know the right place to put stuff. :P

> But IMO the desire here is to ask for a bit more uniformity, not
> require it.

Yeah.

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 49+ messages in thread

* RE: function prototype element ordering
  2021-09-22 21:15               ` Linus Torvalds
  (?)
  (?)
@ 2021-09-25 19:40               ` David Laight
  2021-09-26 21:03                   ` Linus Torvalds
  -1 siblings, 1 reply; 49+ messages in thread
From: David Laight @ 2021-09-25 19:40 UTC (permalink / raw)
  To: 'Linus Torvalds', Alexey Dobriyan
  Cc: Linux Kernel Mailing List, Joe Perches, Andrew Morton, apw,
	Christoph Lameter, Daniel Micay, Dennis Zhou, dwaipayanray1,
	Joonsoo Kim, Linux-MM, Lukas Bulwahn, mm-commits,
	Nathan Chancellor, Nick Desaulniers, Miguel Ojeda, Pekka Enberg,
	David Rientjes, Tejun Heo, Vlastimil Babka,
	open list:DOCUMENTATION

From: Linus Torvalds
> Sent: 22 September 2021 22:16
...
> We don't put function return types on their own lines either, even if
> some other projects have that rule (just to get function names at the
> beginning of lines or some other odd reason).

If the function name starts at the beginning of a line it is
much easier to grep for the definition.
Trying to find function definitions in the Linux kernel tree
is a PITA - unless they are exported when 'EXPORT.*(function_name)'
will tend to work.

Trying to compile:
static int x(int y) __attribute__((section("x"))) { return y;}
with gcc generates "error: attributes are not allowed on a function-definition".

Putting the attribute anywhere before the function name works fine.
gcc probably accepts:
__inline static __inline int __inline x(void) {return 0;} 

So any of those locations is plausible.
But after the arguments isn't allowed.
So an (extern) function declaration probably should not put them
there - if only for consistency.

I think I'd go for 'first' - optionally on their own line.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: function prototype element ordering
  2021-09-25 19:40               ` David Laight
@ 2021-09-26 21:03                   ` Linus Torvalds
  0 siblings, 0 replies; 49+ messages in thread
From: Linus Torvalds @ 2021-09-26 21:03 UTC (permalink / raw)
  To: David Laight
  Cc: Alexey Dobriyan, Linux Kernel Mailing List, Joe Perches,
	Andrew Morton, apw, Christoph Lameter, Daniel Micay, Dennis Zhou,
	dwaipayanray1, Joonsoo Kim, Linux-MM, Lukas Bulwahn, mm-commits,
	Nathan Chancellor, Nick Desaulniers, Miguel Ojeda, Pekka Enberg,
	David Rientjes, Tejun Heo, Vlastimil Babka,
	open list:DOCUMENTATION

On Sat, Sep 25, 2021 at 12:40 PM David Laight <David.Laight@aculab.com> wrote:
>
> If the function name starts at the beginning of a line it is
> much easier to grep for the definition.

That has always been a completely bogus argument. I grep to look up
the type as often as I grep for the function definition, plus it's not
at all unlikely that the "function" is actually a macro wrapper, so
grepping for the beginning of line is just completely wrong.

It's completely wrong for another reason too: it assumes a style of
programming that has never actually been all that common. It's a very
specific pattern to very specific projects, and anybody who learnt
that pattern for their project is going to be completely lost anywhere
else. So don't do it. It's just a bad idea.

So a broken "easier to grep for" is not an excuse for "make the code
harder to read" particularly when it just makes another type of
grepping harder, and it's not actually nearly universal enough to
actually be a useful pattern in the first place.

It's not only never been the pattern in the kernel, but it's generally
not been the pattern anywhere else either. It's literally one of the
broken GNU coding standards - and the fact that almost every other
part of the GNU coding standards were wrong (indentation, placement of
braces, you name it) should give you a hint about how good _that_ one
was.

Here's an exercise for you: go search for C coding examples on the
web, and see how many of them do

    int main(int argc, char **argv)

vs how many of them do

    int
    main(int argc, char **argv)

and then realize that in order for the "grep for ^main" pattern to be
useful, the second version has to not just be more common, it has to
be practically *universal*.

Hint: it isn't even remotely more common, much less universal. In
Debian code search, I had to go to the third page to find any example
at all of people putting the "int" and the "main" on different lines,
and even that one didn't place the "main()" at the beginning of the
line - they had been separated because of other reasons and looked
like this:

int
#ifdef _WIN32
    __cdecl
#endif // _WIN32
    main(int argc, char** argv)

instead.

Maybe Dbian code search isn't the place to go, but I think it proves
my case: the "function name at beginning of line" story is pure
make-believe, and has absolutely no relevance in the real world.

It's a bad straightjacket. Just get over it, and stop perpetuating the
idiotic myth.

If you care so much about grepping for function declarations, and you
use that old-fashioned GNU coding standard policy as an argument, just
be *properly* old-fashioned instead, and use etags or something.

Don't make the rest of us suffer.

Because I grep for functions all the time, and I'd rather have useful
output - which very much includes the type of the function. That's
often one reason _why_ I grep for things in the first place.

Other grep tricks for when the function really is used everywhere, and
you are having trouble finding the definition vs the use:

 - grep in the headers for the type, and actually use the type (either
of the function, or the first argument) as part of the pattern.

   If you really have no idea where it might be, you'll want to start
off with the header grep anyway, to find the macro case (or the inline
case)

   Yeah, splitting the declaration will screw the type information up.
So don't do that, then.

 - if it's so widely used that you find it all over, it's probably
exported. grep for 'EXPORT.*fnname' to see where it is defined.

   We used to (brokenly) export things separately from the definition.
If you find cases of that, let's fix them.

Of course, usually I know roughly where it is defined, so I just limit
the pathnames for 'git grep'.

But the 'add the type of the return value or first argument to the
pattern' is often my second go-to (particularly for the cases where
you might be looking for _multiple_ definitions because it's some
architecture-specific thing, or you have some partial pattern because
every filesystem does their own thing).

Other 'git grep' patterns that often work for kernel sources:

 - looking for a structure declaration? Use

      git grep 'struct name {'

   which mostly works, but obviously depends on coding style so it's
not guaranteed. Good first thing to try, though.

 - use

        git grep '\t\.name\>.*='

   to find things like particular inode operations.

That second case is because we have almost universally converted our
filesystem operation initializers to use that named format (and really
strive to have a policy of constant structures of function pointers
only), and it's really convenient if you are doing VFS changes and
need to find all the places that use a particular VFS interface (eg
".get_acl" or similar).

It used to be a nightmare to find those things back when most of our
initializers were using the traditional unnamed ordered structure
initializers, so this is one area where we've introduced coding style
policies to make it really easy to grep for things (but also much
easier to add new fields and not have to add pointless NULL
initializer elements, of course).

             Linus

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: function prototype element ordering
@ 2021-09-26 21:03                   ` Linus Torvalds
  0 siblings, 0 replies; 49+ messages in thread
From: Linus Torvalds @ 2021-09-26 21:03 UTC (permalink / raw)
  To: David Laight
  Cc: Alexey Dobriyan, Linux Kernel Mailing List, Joe Perches,
	Andrew Morton, apw, Christoph Lameter, Daniel Micay, Dennis Zhou,
	dwaipayanray1, Joonsoo Kim, Linux-MM, Lukas Bulwahn, mm-commits,
	Nathan Chancellor, Nick Desaulniers, Miguel Ojeda, Pekka Enberg,
	David Rientjes, Tejun Heo, Vlastimil Babka,
	open list:DOCUMENTATION

On Sat, Sep 25, 2021 at 12:40 PM David Laight <David.Laight@aculab.com> wrote:
>
> If the function name starts at the beginning of a line it is
> much easier to grep for the definition.

That has always been a completely bogus argument. I grep to look up
the type as often as I grep for the function definition, plus it's not
at all unlikely that the "function" is actually a macro wrapper, so
grepping for the beginning of line is just completely wrong.

It's completely wrong for another reason too: it assumes a style of
programming that has never actually been all that common. It's a very
specific pattern to very specific projects, and anybody who learnt
that pattern for their project is going to be completely lost anywhere
else. So don't do it. It's just a bad idea.

So a broken "easier to grep for" is not an excuse for "make the code
harder to read" particularly when it just makes another type of
grepping harder, and it's not actually nearly universal enough to
actually be a useful pattern in the first place.

It's not only never been the pattern in the kernel, but it's generally
not been the pattern anywhere else either. It's literally one of the
broken GNU coding standards - and the fact that almost every other
part of the GNU coding standards were wrong (indentation, placement of
braces, you name it) should give you a hint about how good _that_ one
was.

Here's an exercise for you: go search for C coding examples on the
web, and see how many of them do

    int main(int argc, char **argv)

vs how many of them do

    int
    main(int argc, char **argv)

and then realize that in order for the "grep for ^main" pattern to be
useful, the second version has to not just be more common, it has to
be practically *universal*.

Hint: it isn't even remotely more common, much less universal. In
Debian code search, I had to go to the third page to find any example
at all of people putting the "int" and the "main" on different lines,
and even that one didn't place the "main()" at the beginning of the
line - they had been separated because of other reasons and looked
like this:

int
#ifdef _WIN32
    __cdecl
#endif // _WIN32
    main(int argc, char** argv)

instead.

Maybe Dbian code search isn't the place to go, but I think it proves
my case: the "function name at beginning of line" story is pure
make-believe, and has absolutely no relevance in the real world.

It's a bad straightjacket. Just get over it, and stop perpetuating the
idiotic myth.

If you care so much about grepping for function declarations, and you
use that old-fashioned GNU coding standard policy as an argument, just
be *properly* old-fashioned instead, and use etags or something.

Don't make the rest of us suffer.

Because I grep for functions all the time, and I'd rather have useful
output - which very much includes the type of the function. That's
often one reason _why_ I grep for things in the first place.

Other grep tricks for when the function really is used everywhere, and
you are having trouble finding the definition vs the use:

 - grep in the headers for the type, and actually use the type (either
of the function, or the first argument) as part of the pattern.

   If you really have no idea where it might be, you'll want to start
off with the header grep anyway, to find the macro case (or the inline
case)

   Yeah, splitting the declaration will screw the type information up.
So don't do that, then.

 - if it's so widely used that you find it all over, it's probably
exported. grep for 'EXPORT.*fnname' to see where it is defined.

   We used to (brokenly) export things separately from the definition.
If you find cases of that, let's fix them.

Of course, usually I know roughly where it is defined, so I just limit
the pathnames for 'git grep'.

But the 'add the type of the return value or first argument to the
pattern' is often my second go-to (particularly for the cases where
you might be looking for _multiple_ definitions because it's some
architecture-specific thing, or you have some partial pattern because
every filesystem does their own thing).

Other 'git grep' patterns that often work for kernel sources:

 - looking for a structure declaration? Use

      git grep 'struct name {'

   which mostly works, but obviously depends on coding style so it's
not guaranteed. Good first thing to try, though.

 - use

        git grep '\t\.name\>.*='

   to find things like particular inode operations.

That second case is because we have almost universally converted our
filesystem operation initializers to use that named format (and really
strive to have a policy of constant structures of function pointers
only), and it's really convenient if you are doing VFS changes and
need to find all the places that use a particular VFS interface (eg
".get_acl" or similar).

It used to be a nightmare to find those things back when most of our
initializers were using the traditional unnamed ordered structure
initializers, so this is one area where we've introduced coding style
policies to make it really easy to grep for things (but also much
easier to add new fields and not have to add pointless NULL
initializer elements, of course).

             Linus


^ permalink raw reply	[flat|nested] 49+ messages in thread

* RE: function prototype element ordering
  2021-09-26 21:03                   ` Linus Torvalds
  (?)
@ 2021-09-27  8:21                   ` David Laight
  2021-09-27  9:22                     ` Willy Tarreau
  -1 siblings, 1 reply; 49+ messages in thread
From: David Laight @ 2021-09-27  8:21 UTC (permalink / raw)
  To: 'Linus Torvalds'
  Cc: Alexey Dobriyan, Linux Kernel Mailing List, Joe Perches,
	Andrew Morton, apw, Christoph Lameter, Daniel Micay, Dennis Zhou,
	dwaipayanray1, Joonsoo Kim, Linux-MM, Lukas Bulwahn, mm-commits,
	Nathan Chancellor, Nick Desaulniers, Miguel Ojeda, Pekka Enberg,
	David Rientjes, Tejun Heo, Vlastimil Babka,
	open list:DOCUMENTATION

From: Linus Torvalds
> Sent: 26 September 2021 22:04
> 
> On Sat, Sep 25, 2021 at 12:40 PM David Laight <David.Laight@aculab.com> wrote:
> >
> > If the function name starts at the beginning of a line it is
> > much easier to grep for the definition.
> 
> That has always been a completely bogus argument. I grep to look up
> the type as often as I grep for the function definition, plus it's not
> at all unlikely that the "function" is actually a macro wrapper, so
> grepping for the beginning of line is just completely wrong.
> 
> It's completely wrong for another reason too: it assumes a style of
> programming that has never actually been all that common. It's a very
> specific pattern to very specific projects, and anybody who learnt
> that pattern for their project is going to be completely lost anywhere
> else. So don't do it. It's just a bad idea.
> 
> So a broken "easier to grep for" is not an excuse for "make the code
> harder to read" particularly when it just makes another type of
> grepping harder, and it's not actually nearly universal enough to
> actually be a useful pattern in the first place.
> 
> It's not only never been the pattern in the kernel, but it's generally
> not been the pattern anywhere else either. It's literally one of the
> broken GNU coding standards - and the fact that almost every other
> part of the GNU coding standards were wrong (indentation, placement of
> braces, you name it) should give you a hint about how good _that_ one
> was.
> 
> Here's an exercise for you: go search for C coding examples on the
> web, and see how many of them do
> 
>     int main(int argc, char **argv)
> 
> vs how many of them do
> 
>     int
>     main(int argc, char **argv)

It makes a bigger difference with:

struct frobulate *find_frobulate(args)
which is going to need a line break somewhere.
Especially with the (strange) rule about aligning the continued
arguments with the (.

But I didn't expect such a long response :-)

I'm sure the netBSD tree (mostly) puts the function name in column 1.
But after that uses the K&R location for {} (as does Linux).

It true that a lot of 'coding standards' are horrid.
Putting '} else {' on one line is important when reading code.
Especially if the '}' would be at the bottom of the screen,
or worse still turning the page on a fan-fold paper listing to find
a floating 'else' = with no idea which 'if' it goes with.

The modern example of why { and } shouldn't be on their own lines is:
		...
	}
	while (...........................
	{
		...
Is that a loop bottom followed by a code block or
a conditional followed by a loop?

But none of this is related to the location of attributes unless
you need to split long lines and put the attribute before the
function name where you may need.

static struct frobulate *
__inline ....
find_frobulate(....)

Especially if you need #if around the attributes.

	David


	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: function prototype element ordering
  2021-09-27  8:21                   ` David Laight
@ 2021-09-27  9:22                     ` Willy Tarreau
  0 siblings, 0 replies; 49+ messages in thread
From: Willy Tarreau @ 2021-09-27  9:22 UTC (permalink / raw)
  To: David Laight
  Cc: 'Linus Torvalds',
	Alexey Dobriyan, Linux Kernel Mailing List, Joe Perches,
	Andrew Morton, apw, Christoph Lameter, Daniel Micay, Dennis Zhou,
	dwaipayanray1, Joonsoo Kim, Linux-MM, Lukas Bulwahn, mm-commits,
	Nathan Chancellor, Nick Desaulniers, Miguel Ojeda, Pekka Enberg,
	David Rientjes, Tejun Heo, Vlastimil Babka,
	open list:DOCUMENTATION

On Mon, Sep 27, 2021 at 08:21:24AM +0000, David Laight wrote:
> Putting '} else {' on one line is important when reading code.

I used not to like that due to "else if ()" being less readable and less
easy to spot, but the arguments you gave regarding the end of screen are
valid and are similar to my hate of GNU's broken "while ()" on its own
line especially after a "do { }" block where it immediately looks like
an accidental infinite loop.

However:

> But none of this is related to the location of attributes unless
> you need to split long lines and put the attribute before the
> function name where you may need.
> 
> static struct frobulate *
> __inline ....
> find_frobulate(....)

This is exactly the case where I hate to dig into code looking like
that: you build, it fails to find symbol "find_frobulate()", you run
"git grep -w find_frobulate" to figure what file provides it, or even
"grep ^find_frobulate" if you want. And you find it in frobulate.c. You
double-check, you find that frobulate.o was built and linked into your
executable. Despite this it fails to find the symbol. Finally you open
the file to discover this painful "static" two lines above, which made
you waste 3 minutes of your time digging at the wrong place.

*Just* for this reason I'm much more careful to always put the type and
name on the same line nowadays.

> Especially if you need #if around the attributes.

This is the only exception I still have to the rule above. But #if by
definition require multi-line processing anyway and they're not welcome
in the middle of control flows.

Willy

^ permalink raw reply	[flat|nested] 49+ messages in thread

end of thread, other threads:[~2021-09-27  9:23 UTC | newest]

Thread overview: 49+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-10  3:09 incoming Andrew Morton
2021-09-10  3:10 ` [patch 1/9] mm: move kvmalloc-related functions to slab.h Andrew Morton
2021-09-10  3:10 ` [patch 2/9] rapidio: avoid bogus __alloc_size warning Andrew Morton
2021-09-10  3:10 ` [patch 3/9] Compiler Attributes: add __alloc_size() for better bounds checking Andrew Morton
2021-09-10  3:10 ` [patch 4/9] checkpatch: add __alloc_size() to known $Attribute Andrew Morton
2021-09-10  3:10 ` [patch 5/9] slab: clean up function declarations Andrew Morton
2021-09-10  3:10 ` [patch 6/9] slab: add __alloc_size attributes for better bounds checking Andrew Morton
2021-09-10  3:10 ` [patch 7/9] mm/page_alloc: " Andrew Morton
2021-09-10  3:10 ` [patch 8/9] percpu: " Andrew Morton
2021-09-10  3:10 ` [patch 9/9] mm/vmalloc: " Andrew Morton
2021-09-10 17:23   ` Linus Torvalds
2021-09-10 17:23     ` Linus Torvalds
2021-09-10 18:43     ` Kees Cook
2021-09-10 19:17       ` Linus Torvalds
2021-09-10 19:17         ` Linus Torvalds
2021-09-10 19:32         ` Kees Cook
2021-09-10 19:49     ` Nick Desaulniers
2021-09-10 19:49       ` Nick Desaulniers
2021-09-10 20:16       ` Linus Torvalds
2021-09-10 20:16         ` Linus Torvalds
2021-09-10 20:47         ` Kees Cook
2021-09-10 20:58           ` Nick Desaulniers
2021-09-10 20:58             ` Nick Desaulniers
2021-09-10 21:07             ` Kees Cook
2021-09-11  5:29     ` Joe Perches
2021-09-11  5:29       ` Joe Perches
2021-09-21 23:37     ` Kees Cook
2021-09-21 23:45       ` Joe Perches
2021-09-21 23:45         ` Joe Perches
2021-09-22  2:25         ` function prototype element ordering Kees Cook
2021-09-22  4:24           ` Joe Perches
2021-09-22  4:24             ` Joe Perches
2021-09-24 19:43             ` Kees Cook
2021-09-22  7:24           ` Alexey Dobriyan
2021-09-22  8:51             ` Joe Perches
2021-09-22  8:51               ` Joe Perches
2021-09-22 10:45               ` Alexey Dobriyan
2021-09-22 11:19             ` Jani Nikula
2021-09-22 21:15             ` Linus Torvalds
2021-09-22 21:15               ` Linus Torvalds
2021-09-23  5:10               ` Joe Perches
2021-09-23  5:10                 ` Joe Perches
2021-09-25 19:40               ` David Laight
2021-09-26 21:03                 ` Linus Torvalds
2021-09-26 21:03                   ` Linus Torvalds
2021-09-27  8:21                   ` David Laight
2021-09-27  9:22                     ` Willy Tarreau
2021-09-10 17:11 ` incoming Kees Cook
2021-09-10 20:13   ` incoming Kees Cook

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.