[PATCH] x86: Fix compilation bug in kprobes' twobyte_is_boostable

[PATCH] x86: Fix compilation bug in kprobes' twobyte_is_boostable

[Josh Stone]

When compiling an i386_defconfig kernel with gcc-4.6.1-9.fc15.i686, I
noticed a warning about the asm operand for test_bit in kprobes'
can_boost.  I discovered that this caused only the first long of
twobyte_is_boostable[] to be output.

Jakub filed and fixed gcc PR50571 to correct the warning and this output
issue.  But to solve it for less current gcc, we can make kprobes'
twobyte_is_boostable[] volatile, and it won't be optimized out.

Before:

    CC      arch/x86/kernel/kprobes.o
  In file included from include/linux/bitops.h:22:0,
                   from include/linux/kernel.h:17,
                   from [...]/arch/x86/include/asm/percpu.h:44,
                   from [...]/arch/x86/include/asm/current.h:5,
                   from [...]/arch/x86/include/asm/processor.h:15,
                   from [...]/arch/x86/include/asm/atomic.h:6,
                   from include/linux/atomic.h:4,
                   from include/linux/mutex.h:18,
                   from include/linux/notifier.h:13,
                   from include/linux/kprobes.h:34,
                   from arch/x86/kernel/kprobes.c:43:
  [...]/arch/x86/include/asm/bitops.h: In function ‘can_boost.part.1’:
  [...]/arch/x86/include/asm/bitops.h:319:2: warning: use of memory input
        without lvalue in asm operand 1 is deprecated [enabled by default]

  $ objdump -rd arch/x86/kernel/kprobes.o | grep -A1 -w bt
       551:	0f a3 05 00 00 00 00 	bt     %eax,0x0
                          554: R_386_32	.rodata.cst4

  $ objdump -s -j .rodata.cst4 -j .data arch/x86/kernel/kprobes.o

  arch/x86/kernel/kprobes.o:     file format elf32-i386

  Contents of section .data:
   0000 48000000 00000000 00000000 00000000  H...............
  Contents of section .rodata.cst4:
   0000 4c030000                             L...

Only a single long of twobyte_is_boostable[] is in the object file.

After, with volatile:

  $ objdump -rd arch/x86/kernel/kprobes.o | grep -A1 -w bt
       551:	0f a3 05 20 00 00 00 	bt     %eax,0x20
                          554: R_386_32	.data

  $ objdump -s -j .rodata.cst4 -j .data arch/x86/kernel/kprobes.o

  arch/x86/kernel/kprobes.o:     file format elf32-i386

  Contents of section .data:
   0000 48000000 00000000 00000000 00000000  H...............
   0010 00000000 00000000 00000000 00000000  ................
   0020 4c030000 0f000200 ffff0000 ffcff0c0  L...............
   0030 0000ffff 3bbbfff8 03ff2ebb 26bb2e77  ....;.......&..w

Now all 32 bytes are output into .data instead.

Signed-off-by: Josh Stone <jistone@redhat.com>
Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Jakub Jelinek <jakub@redhat.com>

---

Note, this is a repost of https://lkml.org/lkml/2011/10/17/500

See also my more general fix, https://lkml.org/lkml/2011/10/6/412

---
 arch/x86/kernel/kprobes.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/kprobes.c b/arch/x86/kernel/kprobes.c
index f1a6244..c0ed3d9 100644
--- a/arch/x86/kernel/kprobes.c
+++ b/arch/x86/kernel/kprobes.c
@@ -75,8 +75,10 @@ DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
 	/*
 	 * Undefined/reserved opcodes, conditional jump, Opcode Extension
 	 * Groups, and some special opcodes can not boost.
+	 * This is volatile to keep gcc from statically optimizing it out, as
+	 * variable_test_bit makes gcc think only *(unsigned long*) is used.
 	 */
-static const u32 twobyte_is_boostable[256 / 32] = {
+static volatile const u32 twobyte_is_boostable[256 / 32] = {
 	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f          */
 	/*      ----------------------------------------------          */
 	W(0x00, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0, 0, 0, 0, 0, 0) | /* 00 */
-- 
1.7.6.4

Re: [PATCH] x86: Fix compilation bug in kprobes' twobyte_is_boostable

[Linus Torvalds]

On Fri, Oct 21, 2011 at 11:33 PM, Josh Stone <jistone@redhat.com> wrote:
> When compiling an i386_defconfig kernel with gcc-4.6.1-9.fc15.i686, I
> noticed a warning about the asm operand for test_bit in kprobes'
> can_boost.  I discovered that this caused only the first long of
> twobyte_is_boostable[] to be output.
>
> Jakub filed and fixed gcc PR50571 to correct the warning and this output
> issue.  But to solve it for less current gcc, we can make kprobes'
> twobyte_is_boostable[] volatile, and it won't be optimized out.

Hmm. I'd *much* rather do this in arch/x86/include/asm/bitops.h
instead, methinks.

Also, rather than your

> See also my more general fix, https://lkml.org/lkml/2011/10/6/412

wouldn't the simple fix be just to add the volatile there to the cast
we already do, ie something like the appended (cut-and-paste, so it's
whitespace-damaged, but you get the idea).

Now, I'm the first to say that I hate volatile, and I'm not entirely
happy about the cast there, but (a) we're casting a volatile pointer
to begin with, and (b) it's essentially the same thing that we do for
the inline asms that modify the bit (see the "ADDR" macro, which also
handles some gcc versioning issues).

And I don't mind volatile in code nearly as much as I mind volatile on
the data structures (it's one of my "C typing was misdesigned" pet
peeves: I think "volatile" is about the access, not about the data)

Does this fix the gcc problem too?

Historical notes:

 - We *used* to have a volatile there (and a whole base address and
"tell gcc which word changed" mess) up until commit eb2b4e682a6 ("x86:
revert commit 709f744 ("x86: bitops asm constraint fixes")"). We
reverted the base address mess, but we probably should have kept the
"volatile".

 - Long long ago, we had that "big array" approach for ADDR too. So
we've wavered between the volatile and using a block memory op. But
we've used the "volatile" for a long time now for the bit change ones,
so I don't think we should mix concepts like your patch.

Comments? Does the simple 'volatile' approach also fix the problem?

               Linus

---
 arch/x86/include/asm/bitops.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h
index 1775d6e5920e..87f000d9377e 100644
--- a/arch/x86/include/asm/bitops.h
+++ b/arch/x86/include/asm/bitops.h
@@ -319,7 +319,7 @@ static inline int variable_test_bit(int nr, volatile const u
        asm volatile("bt %2,%1\n\t"
                     "sbb %0,%0"
                     : "=r" (oldbit)
-                    : "m" (*(unsigned long *)addr), "Ir" (nr));
+                    : "m" (*(volatile unsigned long *)addr), "Ir" (nr));

        return oldbit;
 }

Re: [PATCH] x86: Fix compilation bug in kprobes' twobyte_is_boostable

[Josh Stone]

On 10/22/2011 05:25 PM, Linus Torvalds wrote:
> Hmm. I'd *much* rather do this in arch/x86/include/asm/bitops.h
> instead, methinks.

Agreed, that's why I tried that first.

> Also, rather than your
> 
>> See also my more general fix, https://lkml.org/lkml/2011/10/6/412
> 
> wouldn't the simple fix be just to add the volatile there to the cast
> we already do, ie something like the appended (cut-and-paste, so it's
> whitespace-damaged, but you get the idea).

Unfortunately, no.  Whatever const-propagation gcc is doing here
(somewhat wrongly per PR50571) is not affected by volatile on that cast.
 I also tried leaving it to the parameter type (no cast), no help.

Another historical note is that the parameter used to be void* until
commit 5136dea5, which I suppose is why the current cast exists at all.
 Reverting that still doesn't affect the immediate problem.

It may have gotten lost in the various messages I sent, but note that
this is only happening on i386, where the source u32 matches the size of
the casted long.  On x86_64, where long is of course bigger than u32,
the pointer aliasing seems to prevent the issue.  So yet another fix is
to make that asm cast something silly that will always alias, like
(*(char*)addr).  That might be even more magical/fragile though.

> And I don't mind volatile in code nearly as much as I mind volatile on
> the data structures (it's one of my "C typing was misdesigned" pet
> peeves: I think "volatile" is about the access, not about the data)

I just tried removing the const from twobyte_is_boostable[], and that
also does the trick.  Not sure why I didn't try that first -- I guess
because I saw all the volatiles in bitops.  Is that more palatable, or
would you still rather try to fix it in bitops.h directly?

>  - Long long ago, we had that "big array" approach for ADDR too. So
> we've wavered between the volatile and using a block memory op. But
> we've used the "volatile" for a long time now for the bit change ones,
> so I don't think we should mix concepts like your patch.

Do you recall why it settled on volatile?  That seems like the less
descriptive of the two approaches.  But "long long ago" appears to be
beyond recorded (git) history...

Thanks,
Josh

Re: [PATCH] x86: Fix compilation bug in kprobes' twobyte_is_boostable

[Linus Torvalds]

On Sun, Oct 23, 2011 at 7:07 PM, Josh Stone <jistone@redhat.com> wrote:
>
> Unfortunately, no.  Whatever const-propagation gcc is doing here
> (somewhat wrongly per PR50571) is not affected by volatile on that cast.
>  I also tried leaving it to the parameter type (no cast), no help.

Oh, ok.

> I just tried removing the const from twobyte_is_boostable[], and that
> also does the trick.  Not sure why I didn't try that first -- I guess
> because I saw all the volatiles in bitops.  Is that more palatable, or
> would you still rather try to fix it in bitops.h directly?

I guess there's no nice bitops.h fix, so maybe the minimal "remove
const" is the right thing to do.

>>  - Long long ago, we had that "big array" approach for ADDR too. So
>> we've wavered between the volatile and using a block memory op. But
>> we've used the "volatile" for a long time now for the bit change ones,
>> so I don't think we should mix concepts like your patch.
>
> Do you recall why it settled on volatile?  That seems like the less
> descriptive of the two approaches.  But "long long ago" appears to be
> beyond recorded (git) history...

Yeah, it's really long ago. Iirc the reason it got removed was that
there were gcc versions that were unhappy about it, we've had problems
in this area before..

             Linus

[PATCH] x86: Fix compilation bug in kprobes' twobyte_is_boostable

[Josh Stone]

When compiling an i386_defconfig kernel with gcc-4.6.1-9.fc15.i686, I
noticed a warning about the asm operand for test_bit in kprobes'
can_boost.  I discovered that this caused only the first long of
twobyte_is_boostable[] to be output.

Jakub filed and fixed gcc PR50571 to correct the warning and this output
issue.  But to solve it for less current gcc, we can make kprobes'
twobyte_is_boostable[] non-const, and it won't be optimized out.

Before:

    CC      arch/x86/kernel/kprobes.o
  In file included from include/linux/bitops.h:22:0,
                   from include/linux/kernel.h:17,
                   from [...]/arch/x86/include/asm/percpu.h:44,
                   from [...]/arch/x86/include/asm/current.h:5,
                   from [...]/arch/x86/include/asm/processor.h:15,
                   from [...]/arch/x86/include/asm/atomic.h:6,
                   from include/linux/atomic.h:4,
                   from include/linux/mutex.h:18,
                   from include/linux/notifier.h:13,
                   from include/linux/kprobes.h:34,
                   from arch/x86/kernel/kprobes.c:43:
  [...]/arch/x86/include/asm/bitops.h: In function ‘can_boost.part.1’:
  [...]/arch/x86/include/asm/bitops.h:319:2: warning: use of memory input
        without lvalue in asm operand 1 is deprecated [enabled by default]

  $ objdump -rd arch/x86/kernel/kprobes.o | grep -A1 -w bt
       551:	0f a3 05 00 00 00 00 	bt     %eax,0x0
                          554: R_386_32	.rodata.cst4

  $ objdump -s -j .rodata.cst4 -j .data arch/x86/kernel/kprobes.o

  arch/x86/kernel/kprobes.o:     file format elf32-i386

  Contents of section .data:
   0000 48000000 00000000 00000000 00000000  H...............
  Contents of section .rodata.cst4:
   0000 4c030000                             L...

Only a single long of twobyte_is_boostable[] is in the object file.

After, without the const on twobyte_is_boostable:

  $ objdump -rd arch/x86/kernel/kprobes.o | grep -A1 -w bt
       551:	0f a3 05 20 00 00 00 	bt     %eax,0x20
                          554: R_386_32	.data

  $ objdump -s -j .rodata.cst4 -j .data arch/x86/kernel/kprobes.o

  arch/x86/kernel/kprobes.o:     file format elf32-i386

  Contents of section .data:
   0000 48000000 00000000 00000000 00000000  H...............
   0010 00000000 00000000 00000000 00000000  ................
   0020 4c030000 0f000200 ffff0000 ffcff0c0  L...............
   0030 0000ffff 3bbbfff8 03ff2ebb 26bb2e77  ....;.......&..w

Now all 32 bytes are output into .data instead.

Signed-off-by: Josh Stone <jistone@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Jakub Jelinek <jakub@redhat.com>

---
 arch/x86/kernel/kprobes.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/kprobes.c b/arch/x86/kernel/kprobes.c
index f1a6244..794bc95 100644
--- a/arch/x86/kernel/kprobes.c
+++ b/arch/x86/kernel/kprobes.c
@@ -75,8 +75,10 @@ DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
 	/*
 	 * Undefined/reserved opcodes, conditional jump, Opcode Extension
 	 * Groups, and some special opcodes can not boost.
+	 * This is non-const to keep gcc from statically optimizing it out, as
+	 * variable_test_bit makes gcc think only *(unsigned long*) is used.
 	 */
-static const u32 twobyte_is_boostable[256 / 32] = {
+static u32 twobyte_is_boostable[256 / 32] = {
 	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f          */
 	/*      ----------------------------------------------          */
 	W(0x00, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0, 0, 0, 0, 0, 0) | /* 00 */
-- 
1.7.6.4