linux-kernel - Re: [GIT] Networking

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Message-ID: <8a58c60f-dfca-6dba-6ea5-0684b1c5e900@gmail.com>
Date:   Sun, 5 Apr 2020 01:55:46 +0200
From:   Alejandro Colomar <colomar.6.4.3@...il.com>
To:     linux-kernel@...r.kernel.org
Subject: Re: [GIT] Networking

On Thu, Sep 3, 2015 at 11:31 AM, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:

 >> [-Wsizeof-array-argument]

 > Ahh. Google shows that it's an old clang warning that gcc has recently
 > picked up.

 > But even clang doesn't seem to have any way for a project to say
 > "please warn about arrays in function argument declaration". It *is*
 > very traditional idiomatic C, it's just that I personally think it's
 > one of those bad traditional C things exactly because it's so
 > misleading about what actually goes on. But I guess that in practice,
 > the only thing that it actually *affects* is "sizeof" (and assignment
 > to the variable name - something that would be invalid for a real
 > array, but works on argument arrays because they are really just
 > pointers).

 > The "array as function argument" syntax is occasionally useful
 > (particularly for the multi-dimensional array case), so I very much
 > understand why it exists, I just think that in the kernel we'd be
 > better off with the rule that it's against our coding practices.

 >                   Linus


Hi Linus,

First of all, this is my first message to this mailing list, and I'm
trying to reply to a very old thread, so sorry if I don't know how I
should do it.

I have a different approach in my code to avoid that whole class of bugs
relating sizeof and false arrays in function argument declarations.
I do like the sintactic sugar that they provide, so I decided to ban
"sizeof(array)" completely off my code.

I have developed the following macro:

#define ARRAY_BYTES(arr)	(sizeof((arr)[0]) * ARRAY_SIZE(arr))

which compiles to a simple "sizeof(arr)" by undoing the division in
"ARRAY_SIZE()", but with the added benefit that it checks that the
argument is an array (due to "ARRAY_SIZE()"), and if not, compilation
breaks which means that the array is not an array but a pointer.

My rules are:

  - Size of an array (number of elements):
	ARRAY_SIZE(arr)
  - Signed size of an array (normally for loops where I compare against a
  signed variable):
	ARRAY_SSIZE(arr)	defined as: ((ptrdiff_t)ARRAY_SIZE(arr))
  - Size of an array in bytes (normally for buffers):
	ARRAY_BYTES(arr)

No use of "sizeof" is allowed for arrays, which completely rules
out bugs of that class, because I never pass an array to "sizeof", which
is the core of the problem.  I've been using those macros in my code for
8 months, and they work really nice.

I propose to include the macro "ARRAY_BYTES()" in <linux/kernel.h> just
after "ARRAY_SIZE()" and replace every appearance of "sizeof(array)" in
Linux by "ARRAY_BYTES(array)", and modify the coding style guide to ban
"sizeof(array)" completely off the kernel.

Below is a patch with two commits:  one that adds the macro to
<linux/kernel.h>, and another one that serves as an example of usage
for the macro (that one is just as an example).


		Alex.

Please CC me <colomar.6.4.3@...il.com> in any response to this thread.

 From b5b674d39b28e703300698fa63e4ab4be646df8f Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <colomar.6.4.3@...il.com>
Date: Sun, 5 Apr 2020 01:45:35 +0200
Subject: [PATCH 1/2] linux/kernel.h: add ARRAY_BYTES() macro

---
  include/linux/kernel.h | 6 ++++++
  1 file changed, 6 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 9b7a8d74a9d6..dc806e2a7799 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -46,6 +46,12 @@
   */
  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + 
__must_be_array(arr))

+/**
+ * ARRAY_BYTES - get the number of bytes in array @arr
+ * @arr: array to be sized
+ */
+#define ARRAY_BYTES(arr)	(sizeof((arr)[0]) * ARRAY_SIZE(arr))
+
  #define u64_to_user_ptr(x) (		\
  {					\
  	typecheck(u64, (x));		\
-- 
2.25.1


 From 3e7bcf70b708b51a7807c336c5d1b01403989d3b Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <colomar.6.4.3@...il.com>
Date: Sun, 5 Apr 2020 01:48:17 +0200
Subject: [PATCH 2/2] block, bfq: Use ARRAY_BYTES() for arrays instead of
  sizeof()

---
  block/bfq-cgroup.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
index 68882b9b8f11..51ba9b9a8855 100644
--- a/block/bfq-cgroup.c
+++ b/block/bfq-cgroup.c
@@ -7,6 +7,7 @@
  #include <linux/blkdev.h>
  #include <linux/cgroup.h>
  #include <linux/elevator.h>
+#include <linux/kernel.h>
  #include <linux/ktime.h>
  #include <linux/rbtree.h>
  #include <linux/ioprio.h>
@@ -794,7 +795,8 @@ void bfq_bic_update_cgroup(struct bfq_io_cq *bic, 
struct bio *bio)
  	 * refcounter for bfqg, to let it disappear only after no
  	 * bfq_queue refers to it any longer.
  	 */
-	blkg_path(bfqg_to_blkg(bfqg), bfqg->blkg_path, sizeof(bfqg->blkg_path));
+	blkg_path(bfqg_to_blkg(bfqg), bfqg->blkg_path,
+						ARRAY_BYTES(bfqg->blkg_path));
  	bic->blkcg_serial_nr = serial_nr;
  out:
  	rcu_read_unlock();
-- 
2.25.1