lkml.org 
[lkml]   [2008]   [Jul]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: Dangerous code in cpumask_of_cpu?
    Date
    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.


    \
     
     \ /
      Last update: 2008-07-09 06:55    [W:4.574 / U:0.460 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site