Re: Dangerous code in cpumask_of_cpu?

[Mike Travis <travis@sgi.com>]

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