linux-kernel - Re: [PATCH 5/6] net: __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: <20131016102557.GV10651@twins.programming.kicks-ass.net>
Date:	Wed, 16 Oct 2013 12:25:57 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Christoph Lameter <cl@...ux.com>, Tejun Heo <tj@...nel.org>,
	akpm@...uxfoundation.org, rostedt@...dmis.org,
	linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH 5/6] net: __this_cpu_inc in route.c

On Wed, Oct 16, 2013 at 02:22:49AM -0700, Eric Dumazet wrote:
> On Wed, 2013-10-16 at 10:46 +0200, Peter Zijlstra wrote:
> 
> > Are we sure all !x86 implementations will DTRT in that it will increment
> > some CPU and not get horribly confused? I suppose it would; but is
> > that a guarantee given someplace?
> 
> I think we should be fine, these are only stats exposed
> on /proc/net/stat/rt_cache

My concern was really that an unprotected __this_cpu op would complete
coherent and not corrupt state. If we cannot guarantee this it should
always be a full BUG to use it without proper protection.

For x86 its fairly easy to see its correct this way; but for load-store
archs this isn't immediately obvious to me.

Suppose; r1 contains our per-cpu pointer:

	LOAD  r2, per-cpu-base
	ADD   r1, r2
	LOAD  r2, $(r1) # load value
	INC   r2
	STORE $(r1), r2 # store value

If such a thing is done without preempt disable; we could be
preempted/migrated at any place along that chain. In that case the STORE
could be to another CPUs memory (we get migrated near the INC) and could
conflict with a per-cpu operation on that CPU corrupting state.

If I look at percpu.h; the generic __this_cpu versions look like they
generate the above for such archs.

In that case; I don't see how even for statistics (where we really don't
care what cpu the op happens on, as long as it happens to a cpu,
coherently) it is correct to use the raw_this_cpu stuff without
preemption protection.

In fact; I think the comment near __this_cpu_read actually alludes to
this.
--
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/