From patchwork Wed Dec 14 10:55:07 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Rutland X-Patchwork-Id: 88002 Delivered-To: patch@linaro.org Received: by 10.140.20.101 with SMTP id 92csp129533qgi; Wed, 14 Dec 2016 02:56:28 -0800 (PST) X-Received: by 10.84.197.129 with SMTP id n1mr205648510pld.30.1481712988476; Wed, 14 Dec 2016 02:56:28 -0800 (PST) Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id z13si52274224plh.244.2016.12.14.02.56.28; Wed, 14 Dec 2016 02:56:28 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932427AbcLNK4G (ORCPT + 25 others); Wed, 14 Dec 2016 05:56:06 -0500 Received: from foss.arm.com ([217.140.101.70]:41730 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932401AbcLNK4D (ORCPT ); Wed, 14 Dec 2016 05:56:03 -0500 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 560C1152D; Wed, 14 Dec 2016 02:56:02 -0800 (PST) Received: from leverpostej (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E118C3F445; Wed, 14 Dec 2016 02:55:59 -0800 (PST) Date: Wed, 14 Dec 2016 10:55:07 +0000 From: Mark Rutland To: Boqun Feng Cc: "Paul E. McKenney" , Mathieu Desnoyers , Josh Triplett , Colin King , Lai Jiangshan , Steven Rostedt , linux-kernel@vger.kernel.org Subject: Re: [PATCH] rcu: shift by 1UL rather than 1 to fix sign extension error Message-ID: <20161214105506.GA17982@leverpostej> References: <20161213105646.9598-1-colin.king@canonical.com> <20161213171632.GA32535@leverpostej> <20161213183647.GD3924@linux.vnet.ibm.com> <20161214004755.GG9728@tardis.cn.ibm.com> <20161214014002.GI9728@tardis.cn.ibm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20161214014002.GI9728@tardis.cn.ibm.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Dec 14, 2016 at 09:40:02AM +0800, Boqun Feng wrote: > On Wed, Dec 14, 2016 at 08:47:55AM +0800, Boqun Feng wrote: > > On Tue, Dec 13, 2016 at 10:36:47AM -0800, Paul E. McKenney wrote: > > > On Wed, Dec 14, 2016 at 02:09:27AM +0800, Boqun Feng wrote: > > > > #define for_each_leaf_node_cpu(rnp, mask, cpu) \ > > > > for((cpu) = (rnp)->grplo + find _first_bit(mask, MASK_BITS(mask)); \ > > > > (cpu) >= (rnp)->grplo && (cpu) <= (rnp)->grphi; \ > > > > (cpu) = (rnp)->grplo + find _next_bit(mask, ..., > > > > leaf_node_cpu_bit(rnp, cpu) + 1))) \ > > > > if (!cpu_possible(cpu)) \ > > > > continue; \ > > > > else > > > > > > What is the purpose of the cpu_possible() check? > > > > > > > To filter out CPUs in range [grplo, grphi] but not in cpu_possible_mask. > > Hmm.. if rcu_cpu_starting(cpu) is never called with "impossible" cpu, > IOW, ->qsmask and ->expmask never mask "impossible" cpus, then this is > just an over-care check. > > I think I probably will remove this check eventually, let me audit the > code a little more for safety ;-) FWIW, back when I wrote bc75e99983df1efd ("rcu: Correctly handle sparse possible cpus"), the only places I saw accesses to (percpu) data for !possible cpus were the places I fixed up. IIRC I tested with a version of the patch below. That won't catch erroneous non-percpu accesses, but it covers part of the problem, at least. ;) Thanks, Mark. ---->8---- >From fcabcee9ce080073496c736c49e2378a0907c656 Mon Sep 17 00:00:00 2001 From: Mark Rutland Date: Mon, 16 May 2016 16:08:29 +0100 Subject: [PATCH] percpu: add possible cpu validation Recently, the RCU tree code was seen to access per-cpu data for CPUs not in cpu_possible_mask, for which a per-cpu region (and offset) had not been allocated. Often this went unnoticed because valid (but erroneous) pointers were generated, and the accesses hit some other data. This patch adds a new CONFIG_DEBUG_PER_CPU. When selected, per_cpu_ptr will verify that the provided CPU id is possible, and therefore there is a backing percpu area. When the CPU is not possible, we WARN, though the access proceeds are normal otherwise, matching the !CONFIG_DEBUG_PER_CPU behaviour. As the option can adversely affect performance, it is disabled by default. Signed-off-by: Mark Rutland --- include/linux/percpu-defs.h | 16 ++++++++++++++-- lib/Kconfig.debug | 10 ++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) -- 1.9.1 diff --git a/include/linux/percpu-defs.h b/include/linux/percpu-defs.h index 8f16299..1525352 100644 --- a/include/linux/percpu-defs.h +++ b/include/linux/percpu-defs.h @@ -207,6 +207,16 @@ (void)__vpp_verify; \ } while (0) +/* + * __verify_pcpu_cpu() verifies that @cpu is possible, and hence has a valid + * percpu region. + */ +#ifdef CONFIG_DEBUG_PER_CPU +#define __verify_pcpu_cpu(cpu) WARN_ON_ONCE(!cpu_possible(cpu)) +#else +#define __verify_pcpu_cpu(cpu) ({ (void)(cpu); }) +#endif + #ifdef CONFIG_SMP /* @@ -219,8 +229,10 @@ #define per_cpu_ptr(ptr, cpu) \ ({ \ + int ____cpu = (cpu); \ + __verify_pcpu_cpu(____cpu); \ __verify_pcpu_ptr(ptr); \ - SHIFT_PERCPU_PTR((ptr), per_cpu_offset((cpu))); \ + SHIFT_PERCPU_PTR((ptr), per_cpu_offset((____cpu))); \ }) #define raw_cpu_ptr(ptr) \ @@ -247,7 +259,7 @@ (typeof(*(__p)) __kernel __force *)(__p); \ }) -#define per_cpu_ptr(ptr, cpu) ({ (void)(cpu); VERIFY_PERCPU_PTR(ptr); }) +#define per_cpu_ptr(ptr, cpu) ({ __verify_pcpu_cpu(cpu); VERIFY_PERCPU_PTR(ptr); }) #define raw_cpu_ptr(ptr) per_cpu_ptr(ptr, 0) #define this_cpu_ptr(ptr) raw_cpu_ptr(ptr) diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index a6c8db1..14678d2 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -665,6 +665,16 @@ config DEBUG_PER_CPU_MAPS Say N if unsure. +config DEBUG_PER_CPU + bool "Debug access to percpu objects" + depends on DEBUG_KERNEL + help + Say Y to verify that addresses are only generated for valid percpu + objects (i.e. for a possible CPU). This adds additional code and + decreases performance. + + Sey N if unsure. + config DEBUG_HIGHMEM bool "Highmem debugging" depends on DEBUG_KERNEL && HIGHMEM