Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756079AbYGHIy7 (ORCPT ); Tue, 8 Jul 2008 04:54:59 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1750823AbYGHIyu (ORCPT ); Tue, 8 Jul 2008 04:54:50 -0400 Received: from saeurebad.de ([85.214.36.134]:51415 "EHLO saeurebad.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750810AbYGHIyt (ORCPT ); Tue, 8 Jul 2008 04:54:49 -0400 From: Johannes Weiner To: Rusty Russell Cc: Mike Travis , linux-kernel@vger.kernel.org, "H. Anvin" , Christoph Lameter , Ingo Molnar Subject: Re: Dangerous code in cpumask_of_cpu? References: <200807081816.40623.rusty@rustcorp.com.au> <87myksn587.fsf@saeurebad.de> Date: Tue, 08 Jul 2008 10:54:38 +0200 In-Reply-To: <87myksn587.fsf@saeurebad.de> (Johannes Weiner's message of "Tue, 08 Jul 2008 10:35:20 +0200") Message-ID: <87iqvgn4c1.fsf@saeurebad.de> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.0.60 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.1.3 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2721 Lines: 97 Hi, Johannes Weiner writes: > Hi, > > Rusty Russell writes: > >> Hi Christoph/Mike, >> >> Looked at cpumask_of_cpu as introduced in >> 9f0e8d0400d925c3acd5f4e01dbeb736e4011882 (x86: convert cpumask_of_cpu macro >> to allocated array), and I don't think it's safe: >> >> #define cpumask_of_cpu(cpu) \ >> (*({ \ >> typeof(_unused_cpumask_arg_) m; \ >> if (sizeof(m) == sizeof(unsigned long)) { \ >> m.bits[0] = 1UL<<(cpu); \ >> } else { \ >> cpus_clear(m); \ >> cpu_set((cpu), m); \ >> } \ >> &m; \ >> })) >> >> Referring to &m once out of scope is invalid, and I can't find any evidence >> that it's legal here. In particular, the change >> b53e921ba1cff8453dc9a87a84052fa12d5b30bd (generic: reduce stack pressure in >> sched_affinity) which passes &m to other functions seems highly risky. >> >> I'm surprised this hasn't already hit us, but perhaps gcc isn't as clever as >> it could be? > You don't refer to &m outside scope. Look at the character below the > first e of #define :) Oh, well you do access it outside scope, sorry. Me sleepy. I guess because we dereference it immediately again, the location is not clobbered yet. At least in my test case, gcc assembled it to code that puts the address in eax and derefences it immediately, before eax is reused: static int *foo(void) { int x = 42; return &x; } int main(void) { return *foo(); } > But then, this code should probably just evaluate to m without this > obscure *(&m) construct. This, however is still possible, no? --- Subject: cpumask: don't dereference an invalidated pointer m is auto storage, don't use its address outside its scope. Just return m directly instead of that *({type m; &m}) construct. --- diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h index c24875b..19802cb 100644 --- a/include/linux/cpumask.h +++ b/include/linux/cpumask.h @@ -232,7 +232,7 @@ extern cpumask_t *cpumask_of_cpu_map; #else #define cpumask_of_cpu(cpu) \ -(*({ \ +({ \ typeof(_unused_cpumask_arg_) m; \ if (sizeof(m) == sizeof(unsigned long)) { \ m.bits[0] = 1UL<<(cpu); \ @@ -240,8 +240,8 @@ extern cpumask_t *cpumask_of_cpu_map; cpus_clear(m); \ cpu_set((cpu), m); \ } \ - &m; \ -})) + m; \ +}) #endif #define CPU_MASK_LAST_WORD BITMAP_LAST_WORD_MASK(NR_CPUS) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/