Dangerous code in cpumask_of_cpu?

Dangerous code in cpumask_of_cpu?

[Rusty Russell]

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.

Re: Dangerous code in cpumask_of_cpu?

[Johannes Weiner]

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 :)

But then, this code should probably just evaluate to m without this
obscure *(&m) construct.

	Hannes

Re: Dangerous code in cpumask_of_cpu?

[Johannes Weiner]

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:

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)

Re: Dangerous code in cpumask_of_cpu?

[Johannes Weiner]

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

Re: Dangerous code in cpumask_of_cpu?

[Johannes Weiner]

[ fixed christoph's address in cc]

Johannes Weiner <hannes@saeurebad.de> 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

Re: Dangerous code in cpumask_of_cpu?

[Rusty Russell]

On Tuesday 08 July 2008 18:54:38 Johannes Weiner wrote:
> Johannes Weiner <hannes@saeurebad.de> 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.

Re: Dangerous code in cpumask_of_cpu?

[Andreas Schwab]

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 :)

The scope of m ends with the outmost braces, and the dereference is done
outside of it.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
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."

Re: Dangerous code in cpumask_of_cpu?

[Mike Travis]

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.]

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

Re: Dangerous code in cpumask_of_cpu?

[Rusty Russell]

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.

Re: Dangerous code in cpumask_of_cpu?

[Mike Travis]

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