linux-kernel - Re: [gcv v3 27/35] arm: Replace __get_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: <20130830100105.GF25628@mudshark.cambridge.arm.com>
Date:	Fri, 30 Aug 2013 11:01:05 +0100
From:	Will Deacon <will.deacon@....com>
To:	Christoph Lameter <cl@...ux.com>
Cc:	Tejun Heo <tj@...nel.org>,
	"akpm@...uxfoundation.org" <akpm@...uxfoundation.org>,
	Russell King <linux@....linux.org.uk>,
	Catalin Marinas <Catalin.Marinas@....com>,
	"linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
	Steven Rostedt <srostedt@...hat.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [gcv v3 27/35] arm: Replace __get_cpu_var uses

Hi Christoph,

Sorry for the delay in looking at this, I've been on holiday for a week.
Comments inline.

On Wed, Aug 28, 2013 at 08:48:23PM +0100, Christoph Lameter wrote:
> Transformations done to __get_cpu_var()
> 
> 
> 1. Determine the address of the percpu instance of the current processor.
> 
>         DEFINE_PER_CPU(int, y);
>         int *x = &__get_cpu_var(y);
> 
>     Converts to
> 
>         int *x = this_cpu_ptr(&y);
> 
> 
> 2. Same as #1 but this time an array structure is involved.
> 
>         DEFINE_PER_CPU(int, y[20]);
>         int *x = __get_cpu_var(y);
> 
>     Converts to
> 
>         int *x = this_cpu_ptr(y);

This is the flavour we have for ARM's hw_breakpoint code, where we have an
array of perf_event * instead of int...

> Index: linux/arch/arm/kernel/hw_breakpoint.c
> ===================================================================
> --- linux.orig/arch/arm/kernel/hw_breakpoint.c  2013-08-26 13:48:40.956794980 -0500
> +++ linux/arch/arm/kernel/hw_breakpoint.c       2013-08-26 13:48:40.952795024 -0500
> @@ -344,13 +344,13 @@ int arch_install_hw_breakpoint(struct pe
>                 /* Breakpoint */
>                 ctrl_base = ARM_BASE_BCR;
>                 val_base = ARM_BASE_BVR;
> -               slots = (struct perf_event **)__get_cpu_var(bp_on_reg);
> +               slots = (struct perf_event **)__this_cpu_read(bp_on_reg);

...so I don't think this is quite right, and indeed, we get a bunch of errors
from GCC:

arch/arm/kernel/hw_breakpoint.c: In function ‘arch_install_hw_breakpoint’:
arch/arm/kernel/hw_breakpoint.c:347:33: error: incompatible types when assigning to type ‘struct perf_event *[16]’ from type ‘struct perf_event **’
arch/arm/kernel/hw_breakpoint.c:347:1: error: incompatible types when assigning to type ‘struct perf_event *[16]’ from type ‘struct perf_event **’
arch/arm/kernel/hw_breakpoint.c:347:1: error: incompatible types when assigning to type ‘struct perf_event *[16]’ from type ‘struct perf_event **’
arch/arm/kernel/hw_breakpoint.c:347:1: error: incompatible types when assigning to type ‘struct perf_event *[16]’ from type ‘struct perf_event **’

changing to match your recipe still doesn't work, however:

arch/arm/kernel/hw_breakpoint.c: In function ‘arch_install_hw_breakpoint’:
arch/arm/kernel/hw_breakpoint.c:347:33: error: cast specifies array type

> Index: linux/arch/arm64/kernel/debug-monitors.c
> ===================================================================
> --- linux.orig/arch/arm64/kernel/debug-monitors.c       2013-08-26 13:48:40.956794980 -0500
> +++ linux/arch/arm64/kernel/debug-monitors.c    2013-08-26 13:48:40.952795024 -0500
> @@ -98,11 +98,11 @@ void enable_debug_monitors(enum debug_el
> 
>         WARN_ON(preemptible());
> 
> -       if (local_inc_return(&__get_cpu_var(mde_ref_count)) == 1)
> +       if (this_cpu_inc_return(mde_ref_count) == 1)
>                 enable = DBG_MDSCR_MDE;

I'm not sure that this is safe. We rely on local_inc_return to be atomic
with respect to the current CPU, which will end up being a wrapper around
atomic64_inc_return. However, this_cpu_inc_return simply uses a lock, so
other people accessing the count in a different manner (local_dec_and_test
below) may break local atomicity unless we start disabling interrupts or
something horrible like that.
 
>         if (el == DBG_ACTIVE_EL1 &&
> -           local_inc_return(&__get_cpu_var(kde_ref_count)) == 1)
> +           this_cpu_inc_return(kde_ref_count) == 1)
>                 enable |= DBG_MDSCR_KDE;
> 
>         if (enable && debug_enabled) {
> @@ -118,11 +118,11 @@ void disable_debug_monitors(enum debug_e
> 
>         WARN_ON(preemptible());
> 
> -       if (local_dec_and_test(&__get_cpu_var(mde_ref_count)))
> +       if (local_dec_and_test(this_cpu_ptr(&mde_ref_count)))
>                 disable = ~DBG_MDSCR_MDE;
> 
>         if (el == DBG_ACTIVE_EL1 &&
> -           local_dec_and_test(&__get_cpu_var(kde_ref_count)))
> +           local_dec_and_test(this_cpu_ptr(&kde_ref_count)))
>                 disable &= ~DBG_MDSCR_KDE;
> 
>         if (disable) {

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