Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1431141 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] firmware: declare __{start,end}_builtin_fw as pointers Vegard Nossum <vegard.nossum@oracle.com> - 2016-06-25 17:10 +0200
    Re: [PATCH] firmware: declare __{start,end}_builtin_fw as pointers Vegard Nossum <vegard.nossum@gmail.com> - 2016-06-25 23:10 +0200
      Re: [PATCH] firmware: declare __{start,end}_builtin_fw as pointers Vegard Nossum <vegard.nossum@gmail.com> - 2016-06-26 11:30 +0200
        Re: [PATCH] firmware: declare __{start,end}_builtin_fw as pointers Linus Torvalds <torvalds@linux-foundation.org> - 2016-06-26 19:20 +0200

#1431141 — [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.

By changing these variables from arrays to pointers, gcc can no longer
assume that these are separate arrays.

Cc: stable@vger.kernel.org
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 drivers/base/firmware_class.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 773fc30..4dddf7f 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -43,8 +43,8 @@ MODULE_LICENSE("GPL");
 
 #ifdef CONFIG_FW_LOADER
 
-extern struct builtin_fw __start_builtin_fw[];
-extern struct builtin_fw __end_builtin_fw[];
+extern struct builtin_fw *__start_builtin_fw;
+extern struct builtin_fw *__end_builtin_fw;
 
 static bool fw_get_builtin_firmware(struct firmware *fw, const char *name)
 {
-- 
1.9.1

[toc] | [next] | [standalone]

#1431216

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.
>
> By changing these variables from arrays to pointers, gcc can no longer
> assume that these are separate arrays.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>

Actually, the analysis seems right (by inspection of the assembly
code), but the patch is wrong and causes another crash as the
variables are not really pointers but true arrays (i.e. the linker
script provides the address of the variable, not its value).

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.


Vegard

[toc] | [prev] | [next] | [standalone]

#1431391

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1431549

On Sun, Jun 26, 2016 at 2:24 AM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
>
> 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.

Ugh. I worry about the other places where we do things like this,
depending on the linker just assigning the addresses and us being able
to compare them.

If there is a compiler option to disable this optimization, I would
almost prefer that.. Because we really do have a whole slew of these
things.

                 Linus

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web