linux-kernel - Re: [PATCH] x86: fix BUG: using smp_processor_id() in preemptible [00000000] code: oprofiled/3319

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]

Message-ID: <alpine.LFD.2.00.0904051243310.4023@localhost.localdomain>
Date:	Sun, 5 Apr 2009 12:51:11 -0700 (PDT)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Jaswinder Singh Rajput <jaswinder@...nel.org>
cc:	John Levon <levon@...ementarian.org>, Ingo Molnar <mingo@...e.hu>,
	x86 maintainers <x86@...nel.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86: fix BUG: using smp_processor_id() in preemptible
 [00000000] code: oprofiled/3319

On Mon, 6 Apr 2009, Jaswinder Singh Rajput wrote:
>
> Fixed this bug on P4 HT machine:

I don't think this really is correct.

Anything that does that "get_stagger()" thing should already either be 
tied to a specific CPU (because it's going to actually touch the variables 
for "that CPU"), or it should likely set up the data structures for _all_ 
cases.

Just as an example, look at something like "p4_setup_ctrs()": it will use 
that stagger to determine what MSR to read. But if we are preemptable, the 
CPU we actually do the MSR read on may be a _different_ CPU than the CPU 
that we did the "get_stagger()" on!

So I really think that protecting against preemption inside of 
"get_stagger()" is fundamentally buggy. Because if you need it, then by 
the time you return the value from the function, the value has now lost 
all meaning because you might have preempted to another CPU after doing 
the put_cpu().

So I think the "get_cpu()/put_cpu()" should be done around the whole 
sequence (ie from before "get_stagger()" to after the stagger has been 
used to initialize some data structures or do wrmsr/rdmsr calls.

Of course, this is all ancient code, so whatever. But I really think this 
patch is actively bad - it just hides the issue rather than fixing 
anything.

		Linus
--
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/