linux-kernel - Re: [PATCH] x86: Fix compilation bug in kprobes' twobyte_is

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 23 Oct 2011 03:25:19 +0300
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Josh Stone <jistone@...hat.com>
Cc:	linux-kernel@...r.kernel.org,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
	Jakub Jelinek <jakub@...hat.com>
Subject: Re: [PATCH] x86: Fix compilation bug in kprobes' twobyte_is_boostable

On Fri, Oct 21, 2011 at 11:33 PM, Josh Stone <jistone@...hat.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;
 }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/