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?
I don't know what the right answer is, but we might need to go to a pool of
cpumask_ts, a get_cpumask_of_cpu() which can sleep and a put_cpumask_of_cpu?
Or maybe a gcc guru can refute this?
Rusty.
Hi,
Rusty Russell <[email protected]> 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 :)
But then, this code should probably just evaluate to m without this
obscure *(&m) construct.
Hannes
Hi,
Johannes Weiner <[email protected]> writes:
> Hi,
>
> Rusty Russell <[email protected]> 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)
Johannes Weiner <[email protected]> writes:
> Hi,
>
> Johannes Weiner <[email protected]> writes:
>
>> Hi,
>>
>> Rusty Russell <[email protected]> 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
[ fixed christoph's address in cc]
Johannes Weiner <[email protected]> writes:
>> 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.
My theory was half-right. Since the code is a macro, there is no call
and hence no stack clean-up. And although it is UB, it works correctly
as the value is not yet clobbered when we access it again. Converting
foo to a macro yields this:
movl $42, -8(%ebp)
leal -8(%ebp), %eax
movl (%eax), %eax
...
ret
gcc only emits a warning if the scope we leak a local address from is
that of a function.
Hannes
On Tuesday 08 July 2008 18:54:38 Johannes Weiner wrote:
> Johannes Weiner <[email protected]> writes:
> > But then, this code should probably just evaluate to m without this
> > obscure *(&m) construct.
>
> This, however is still possible, no?
Unfortunately this change was specifically made in the changeset I refered to,
so that the &cpumask_of_cpu() could be passed :(
Cheers,
Rusty.
Johannes Weiner <[email protected]> writes:
> Hi,
>
> Rusty Russell <[email protected]> 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 :)
The scope of m ends with the outmost braces, and the dereference is done
outside of it.
Andreas.
--
Andreas Schwab, SuSE Labs, [email protected]
SuSE Linux Products GmbH, Maxfeldstra?e 5, 90409 N?rnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Johannes Weiner wrote:
> Johannes Weiner <[email protected]> writes:
>
>> Hi,
>>
>> Johannes Weiner <[email protected]> writes:
>>
>>> Hi,
>>>
>>> Rusty Russell <[email protected]> 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.]
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.
Thanks,
Mike
On Wednesday 09 July 2008 01:29:34 Mike Travis wrote:
> Johannes Weiner wrote:
> > Johannes Weiner <[email protected]> writes:
> >> Hi,
> >>
> >> Johannes Weiner <[email protected]> writes:
> >>> Hi,
> >>>
> >>> Rusty Russell <[email protected]> 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.
Rusty Russell wrote:
> On Wednesday 09 July 2008 01:29:34 Mike Travis wrote:
>> Johannes Weiner wrote:
>>> Johannes Weiner <[email protected]> writes:
>>>> Hi,
>>>>
>>>> Johannes Weiner <[email protected]> writes:
>>>>> Hi,
>>>>>
>>>>> Rusty Russell <[email protected]> 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