linux-kernel - Re: [RFC][PATCH v2] slub: Keep page and object in sync in slab_alloc

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]

Date:	Fri, 18 Jan 2013 13:52:09 -0500
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Christoph Lameter <cl@...ux.com>
Cc:	LKML <linux-kernel@...r.kernel.org>, linux-mm <linux-mm@...ck.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Pekka Enberg <penberg@...nel.org>,
	Matt Mackall <mpm@...enic.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	RT <linux-rt-users@...r.kernel.org>,
	Clark Williams <clark@...hat.com>,
	John Kacur <jkacur@...il.com>,
	"Luis Claudio R. Goncalves" <lgoncalv@...hat.com>
Subject: Re: [RFC][PATCH v2] slub: Keep page and object in sync in
 slab_alloc_node()

On Fri, 2013-01-18 at 18:29 +0000, Christoph Lameter wrote:
> On Fri, 18 Jan 2013, Steven Rostedt wrote:
> 
> > On Fri, 2013-01-18 at 10:04 -0500, Steven Rostedt wrote:
> >
> > Just to be more complete:
> >
> > > 	CPU0			CPU1
> > > 	----			----
> > 			c = __this_cpu_ptr(s->cpu_slab);
> > 			<migrates to CPU0>
> >
> > > <cpu fetches c->page>
> > 			<another task>
> >
> > > 			updates c->tid
> 
> We can avoid the above scenario by doing a cpu local fetch.
> 
> i.e.
> `	tid = this_cpu_read(s->cpu_slab->tid);

I'm curious to why not just add the preempt disable? It's rather quick
and avoids all this complex trickery, which is just prone to bugs. It
would make it much easier for others to review as well, and also keeps
the setting of page, objects and cpu_slab consistent with everything
else (which is assigned under preempt(irq)_disable).


> 
> 
> > > 			updates c->page
> > > 			updates c->freelist
> > > <cpu fetches c->tid>
> > > <cpu fetches c->freelist>
> > >
> > >   node_match() succeeds even though
> > >     current c->page wont
> > >
> >
> >  <migrates back to CPU 1>
> >
> > >  this_cpu_cmpxchg_double() only tests
> > >    the object (freelist) and tid, both which
> > >    will match, but the page that was tested
> > >    isn't the right one.
> > >
> >
> > Yes, it's very unlikely, but we are in the business of dealing with the
> > very unlikely. That's because in our business, the very unlikely is very
> > likely. Damn, I need to buy a lotto ticket!
> 
> Well, the consequence would be that an object from another node than
> desired will be allocated. Not that severe of an issue.

Yes, it's not that severe of an issue, but it is still incorrect code.
Why not just allocate on whatever node you want then? Why bother with
the check at all?

-- Steve


--
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/