Re: [block IO crash] Re: 2.6.39-rc5-git2 boot crashs

[Thomas Gleixner <tglx@linutronix.de>]

On Wed, 4 May 2011, Tejun Heo wrote:
> > > And that code runs with preemption enabled. So when the task gets
> > > preempted _BEFORE_ it has actuallty written back the data, then the
> > > race window is wide open.
> 
> Hmmm... if it's a race caused by preemtion enabled where it shouldn't
> be, it's most likely the wrong type of this_cpu_cmpxchg_double() being
> used in SLUB?  ie. __this_cpu_cmpxchg_double() where it should have
> been this_cpu_cmpxchg_double()?  Christoph?

No, the problem is that ELAN prevents the cmpxchg8b, but keeps
CONFIG_CMPXCHG_LOCAL=y which then results in the unprotected code for
the following reason:

this_cpu_cmpxchg_double() 

-> __pcpu_double_call_return_bool 

-> this_cpu_cmpxchg_double_4

Which on x86 expands to 

-> percpu_cmpxchg8b_double() when CONFIG_X86_CMPXCHG64=y

With CONFIG_X86_CMPXCHG64=n it expands to the default:

_this_cpu_generic_cmpxchg_double() in linux/percpu.h

#define _this_cpu_generic_cmpxchg_double(pcp1, pcp2, oval1, oval2, nval1, nval2)        \
({                                                                      \
        int ret__;                                                      \
        preempt_disable();                                              \
        ret__ = __this_cpu_generic_cmpxchg_double(pcp1, pcp2,           \
                        oval1, oval2, nval1, nval2);                    \
        preempt_enable();                                               \
        ret__;                                                          \
})

And:

#define __this_cpu_generic_cmpxchg_double(pcp1, pcp2, oval1, oval2, nval1, nval2)       \
({                                                                      \
        int __ret = 0;                                                  \
        if (__this_cpu_read(pcp1) == (oval1) &&                         \
                         __this_cpu_read(pcp2)  == (oval2)) {           \
                __this_cpu_write(pcp1, (nval1));                        \
                __this_cpu_write(pcp2, (nval2));                        \
                __ret = 1;                                              \
        }                                                               \
        (__ret);                                                        \
})

So now that failing config has CONFIG_PREEMPT=n which makes
preempt_disable / enable a nop. 

So preemption is not the problem, but what about interrupts and
softirqs ?

So the question is whether CMPXCHG_LOCAL for x86 wants to depend on
X86_CMPXCHG64.

The other solution is to use irqsafe_cpu_cmpxchg_double() instead of
this_cpu_cmpxchg_double() in slub.c. 

This will not hurt the X86_CMPXCHG64=y case, but keep the expansion to
the above __this_cpu_generic_cmpxchg_double working. 

Which makes me even wonder some more whether we need that whole
CMPXCHG_LOCAL #ifdeffery in slub.c at all.

Thanks,

	tglx