linux-kernel - Re: [PATCH] mm/slub: fix lockups on PREEMPT && !SMP kernels

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150313181650.GA11058@leverpostej>
Date:	Fri, 13 Mar 2015 18:16:50 +0000
From:	Mark Rutland <mark.rutland@....com>
To:	Christoph Lameter <cl@...ux.com>
Cc:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Catalin Marinas <Catalin.Marinas@....com>,
	David Rientjes <rientjes@...gle.com>,
	Jesper Dangaard Brouer <brouer@...hat.com>,
	Joonsoo Kim <iamjoonsoo.kim@....com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Pekka Enberg <penberg@...nel.org>,
	Steve Capper <steve.capper@...aro.org>
Subject: Re: [PATCH] mm/slub: fix lockups on PREEMPT && !SMP kernels

On Fri, Mar 13, 2015 at 04:29:23PM +0000, Christoph Lameter wrote:
> On Fri, 13 Mar 2015, Mark Rutland wrote:
> 
> >  	 */
> > -	do {
> > -		tid = this_cpu_read(s->cpu_slab->tid);
> > -		c = raw_cpu_ptr(s->cpu_slab);
> > -	} while (IS_ENABLED(CONFIG_PREEMPT) && unlikely(tid != c->tid));
> > +	c = raw_cpu_ptr(s->cpu_slab);
> > +	tid = READ_ONCE(c->tid);
> >
> 
> Ok that works for the !SMP case. What about SMP and PREEMPT now?

>From testing on boards I have access to, things seem fine so far with
SMP && PREEMPT. If we have any allocator stress tests I'm more than
happy to give them a go.

As I mentioned, it's not clear to me that the the READ_ONCE(c->tid) is
safe (i.e. it is atomic and non-destructive). If READ_ONCE(c->tid) is
not safe then the code added in 9aabf810a67cd97e is similarly broken
given the access in the loop condition, in addition to the hoisting done
by the compiler.

> And yes code like this was deemed safe for years and the race condition is
> very subtle and difficult to trigger (also given that PREEMPT is rarely
> used these days).

The this_cpu_cmpxchg_double is the saving grace here: if c->tid is read
from a different CPU it will fail and we'll retry the whole thing.
That's exactly what the original patch relied on in the case a
preemption occured after the loop.

w.r.t. CONFIG_PREEMPT, git grep tells me otherwise:

[mark@...erpostej:~/src/linux]% git grep -w 'CONFIG_PREEMPT' -- arch/*/configs/* | wc -l
109
[mark@...erpostej:~/src/linux]% git grep -w 'CONFIG_PREEMPT is not set' -- arch/*/configs/* | wc -l 
2
[mark@...erpostej:~/src/linux]% git grep -w 'CONFIG_PREEMPT=y' -- arch/*/configs/* | wc -l
107
[mark@...erpostej:~/src/linux]% git grep -w 'CONFIG_PREEMPT=n' -- arch/*/configs/* | wc -l
0

Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/