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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 23 Oct 2011 10:07:41 -0700
From:	Josh Stone <jistone@...hat.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
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 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
--
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/