linux-kernel - Re: [RFC][PATCH 2/5] mm: Switch mod_state() to __this_cpu

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: <1316531505.29966.57.camel@gandalf.stny.rr.com>
Date:	Tue, 20 Sep 2011 11:11:45 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Christoph Lameter <cl@...two.org>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	LKML <linux-kernel@...r.kernel.org>, Ingo Molnar <mingo@...e.hu>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Christoph Lameter <cl@...ux.com>, Tejun Heo <htejun@...il.com>
Subject: Re: [RFC][PATCH 2/5] mm: Switch mod_state() to __this_cpu_read()

On Tue, 2011-09-20 at 09:51 -0500, Christoph Lameter wrote:

> I see that __this_cpu_xx operations may not work as intended in
> preemptable contexts and there we could have more changes.

Then why do you use it in slub.c?

in mm/slab.c slab_alloc():

redo:

	/*
	 * Must read kmem_cache cpu data via this cpu ptr. Preemption is
	 * enabled. We may switch back and forth between cpus while
	 * reading from one cpu area. That does not matter as long
	 * as we end up on the original cpu again when doing the cmpxchg.
	 */
	c = __this_cpu_ptr(s->cpu_slab);

The __this_cpu_*() is in preempt enabled location. In fact, the three
this_cpu_write()'s in acquire_slab() is done within a spinlock and thus
with preemption disabled.

> 
> this_cpu ops are single instructions that do not guarantee any consistency
> with other this_cpu_ops. If you want consistency (same per cpu area data)
> then preemption needs to be disabled.

The point is, people get it wrong all the time. In fact, we should
really require that ALL USES of this_cpu_*() must be with preemption
disabled. Regardless. Because anytime you touch a per cpu variable,
there's usually a reason that it is on the cpu you are on. If you don't
have preemption disabled (and I don't count the silly:

#define _this_cpu_generic_read(pcp)					\
({	typeof(pcp) ret__;						\
	preempt_disable();						\
	ret__ = *this_cpu_ptr(&(pcp));					\
	preempt_enable();						\
	ret__;								\
})

Which is totally useless), there's probably a bug somewhere in there.
Just like the code in memcg.

Monkeying around with per cpu data is tricky. If you start doing it in
preempt enabled code, you are most certainly about to get it wrong. Why
have this super optimization. A preempt_disable() is a single operation
that touches cache hot data.

I say nuke all users of preempt_enabled per_cpu touching, and be safe.

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