Re: Dangerous code in cpumask_of_cpu?

From: Mike Travis
Date: Wed Jul 09 2008 - 10:43:12 EST


Rusty Russell wrote:
> On Wednesday 09 July 2008 01:29:34 Mike Travis wrote:
>> Johannes Weiner wrote:
>>> Johannes Weiner <hannes@xxxxxxxxxxxx> writes:
>>>> Hi,
>>>>
>>>> Johannes Weiner <hannes@xxxxxxxxxxxx> writes:
>>>>> Hi,
>>>>>
>>>>> Rusty Russell <rusty@xxxxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/