Messages in this thread | | | Date | Wed, 09 Jul 2008 07:42:55 -0700 | From | Mike Travis <> | Subject | Re: Dangerous code in cpumask_of_cpu? |
| |
Rusty Russell wrote: > On Wednesday 09 July 2008 01:29:34 Mike Travis wrote: >> Johannes Weiner wrote: >>> Johannes Weiner <hannes@saeurebad.de> writes: >>>> Hi, >>>> >>>> Johannes Weiner <hannes@saeurebad.de> writes: >>>>> Hi, >>>>> >>>>> Rusty Russell <rusty@rustcorp.com.au> 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: >>> Gee, just ignore this bs. The address is in eax, not the value. >>> >>>> static int *foo(void) >>>> { >>>> int x = 42; >>>> return &x; >>>> } >>>> >>>> int main(void) >>>> { >>>> return *foo(); >>>> } >>> However, this code seems to produce valid assembly with -O2. gcc just >>> warns and fixes it up. >>> >>> Hannes >> IIRC, the problem was I needed an lvalue and it seems that the *(&m) was >> the way I was able to coerce gcc into producing it. That's not to say >> there may be a better way however... ;-) [Btw, I picked up this technique >> in the (original) per_cpu() macro.] > > Yes, but I could do that because it wasn't referring to a temporary variable. > >> Note the lvalue isn't used for changing the cpumask value, but for sending >> it to functions like set_cpus_allowed_ptr() to avoid pushing the 512 bytes >> of a 4096 cpus cpumask onto the stack. So it becomes &(*(&m))) ... ;-) >> But I thought I checked the assembly for different config options and it >> looked ok. > > Yeah, the problem is that a future gcc will cause horrible and subtle > breakage. > > I think we are going to want a get_cpumask()/put_cpumask() pattern for this. > > Rusty.
Yes, looking at it more closely I can see the problem. Thanks btw for spotting this! I'll look at replacing it with safer code.
Cheers, Mike
| |