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]

Date:	Tue, 20 Sep 2011 10:01:38 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	Christoph Lameter <cl@...two.org>,
	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 15:49 +0200, Thomas Gleixner wrote:

> The best thing is to remove the this_cpu_* hackery alltogether and
> just keep the __this_cpu_* versions (along with proper debugging) and
> git rid of the silly underscores.

I think renaming all the __this_cpu_*() to this_cpu_*() and then
removing all the current this_cpu_*() usages is the best thing.

That is, make it mandatory that all this_cpu_*() users are with preempt
disabled.

> 
> > I think it would make more sence if __this_cpu_read() could be made to
> > trigger a warning if used in context where preemption could be off.
> 
> It should have had such a warning in the first place and the warning
> needs to yell about preemptible (i.e. unprotected) context and not the
> other way round.
> 
> But instead of just slapping smp_processor_id() checks into those
> functions we should add a more sensible debug interface like:
> 
>   debug_check_percpu_access()

Sure, I'd do something like that in the this_cpu_*() code. But since
smp_processor_id() already had the bug triggering. I just did the easy
approach as more of a proof of concept (hence the RFC in the patch set).

> 
> and the per cpu sections which require protection over a series (1
> .. N) of this_cpu_* operations want to have
> 
>   this_cpu_start()
>   this_cpu_end()
> 
> or similar annotations around them.

I agree about the concept but hate the naming.

On IRC, Peter suggested a local_lock_t type. Give a named protection
area. I like this approach as we can add debugging infrastructure to
this concept.

	local_lock_t my_vars;

	local_lock(my_vars);
	var = this_cpu_read();
	/* play with var */
	local_unlock(my_vars);

Peter even suggested for those locations that disable interrupts for
this work we could document that with:

	local_lock_irqsave(my_vars, flags);
	[...]
	local_unlock_irqrestore(my_vars, flags);

This documents nicely the area of the protected variables. The
local_lock() could simply turn into a preempt_disable(), but we could
add hooks into lockdep or something to perform other checks. Even have
sparse or coccinelle find usages of var outside of local_lock(), and
report them.

As the kernel gets more complex, especially with the focus on scaling to
huge number of CPUs, and moving towards per_cpu data handling, having
debugging capabilities of this sort is critical.

> 
> This allows us to do proper analysis of this_cpu usage and makes the
> code understandable.

Totally agree!

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