Re: [PATCH] firmware: declare __{start,end}_builtin_fw as pointers

[Vegard Nossum <vegard.nossum@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1480 bytes --]

On 25 June 2016 at 23:06, Vegard Nossum <vegard.nossum@gmail.com> wrote:
> On 25 June 2016 at 17:04, Vegard Nossum <vegard.nossum@oracle.com> wrote:
>> The test in this loop:
>>
>>   for (b_fw = __start_builtin_fw; b_fw != __end_builtin_fw; b_fw++) {
>>
>> was getting completely compiled out by my gcc, 7.0.0 20160520. The result
>> was that the loop was going beyond the end of the builtin_fw array and
>> giving me a page fault when trying to dereference b_fw->name inside
>> strcmp().
>>
>> I strongly suspect it's because __start_builtin_fw and __end_builtin_fw
>> are both declared as (separate) arrays, and so gcc conludes that b_fw can
>> never point to __end_builtin_fw.
>>
> I see the __start_foo[]/__end_foo[] idiom is used a lot in the kernel
> so this could potentially be a problem in other places as well. The
> best solution may be a compiler flag (if it exists). I'll play a bit
> more with it to see if I can come up with something.

This is the best I could come up with: assuming gcc is not allowed to
reason about what's inside the asm(), this is the only way I could
think of to lose the array information without incurring unnecessary
overheads. It should also be relatively safe as there is no way to
accidentally use the underlying arrays without explicitly declaring
them.

I've not run-tested the final version of the patch yet (as I have to
run), but I did successfully boot an earlier version which was only
cosmetically different (I think).


Vegard

[-- Attachment #2: 0001-firmware-declare-__-start-end-_builtin_fw-as-pointer.patch --]
[-- Type: text/x-patch, Size: 2734 bytes --]

From 7efe386eea88344a0b99a587d759d8df0e937207 Mon Sep 17 00:00:00 2001
From: Vegard Nossum <vegard.nossum@oracle.com>
Date: Sat, 25 Jun 2016 16:30:14 +0200
Subject: [PATCH] firmware: declare __{start,end}_builtin_fw as pointers

The test in this loop:

  for (b_fw = __start_builtin_fw; b_fw != __end_builtin_fw; b_fw++) {

was getting completely compiled out by my gcc, 7.0.0 20160520. The result
was that the loop was going beyond the end of the builtin_fw array and
giving me a page fault when trying to dereference b_fw->name inside
strcmp().

I strongly suspect it's because __start_builtin_fw and __end_builtin_fw
are both declared as (separate) arrays, and so gcc conludes that b_fw can
never point to __end_builtin_fw.

We can lose the array information about a pointer by using an empty
assembly stub since gcc can't reason about the assembly code.

I've used a helper macro so that we can hide the real array definition
and not use it by accident.

Cc: stable@vger.kernel.org
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 drivers/base/firmware_class.c  |  5 +++--
 include/linux/external_array.h | 18 ++++++++++++++++++
 2 files changed, 21 insertions(+), 2 deletions(-)
 create mode 100644 include/linux/external_array.h

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 773fc30..595ec55 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -9,6 +9,7 @@
 
 #include <linux/capability.h>
 #include <linux/device.h>
+#include <linux/external_array.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/timer.h>
@@ -43,8 +44,8 @@ MODULE_LICENSE("GPL");
 
 #ifdef CONFIG_FW_LOADER
 
-extern struct builtin_fw __start_builtin_fw[];
-extern struct builtin_fw __end_builtin_fw[];
+#define __start_builtin_fw external_array(struct builtin_fw, __start_builtin_fw)
+#define __end_builtin_fw external_array(struct builtin_fw, __end_builtin_fw)
 
 static bool fw_get_builtin_firmware(struct firmware *fw, const char *name)
 {
diff --git a/include/linux/external_array.h b/include/linux/external_array.h
new file mode 100644
index 0000000..dfca996
--- /dev/null
+++ b/include/linux/external_array.h
@@ -0,0 +1,18 @@
+#ifndef LINUX_EXTERNAL_ARRAY_H
+#define LINUX_EXTERNAL_ARRAY_H
+
+/*
+ * Lose any array information on the argument. Useful when we define an array
+ * in a linker script using "start" and "end" variables and gcc incorrectly
+ * assumes that these are separate arrays (and aggressively optimizes away
+ * loop termination conditions, for example).
+ */
+#define external_array(type, name) \
+	({ \
+		extern type name[]; \
+		type *name_ptr = name; \
+		asm ("" : "+r" (name_ptr)); \
+		name_ptr; \
+	})
+
+#endif
-- 
1.9.1