From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932241AbcLNNEF (ORCPT ); Wed, 14 Dec 2016 08:04:05 -0500 Received: from mail-pg0-f67.google.com ([74.125.83.67]:34343 "EHLO mail-pg0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755768AbcLNNEE (ORCPT ); Wed, 14 Dec 2016 08:04:04 -0500 Date: Wed, 14 Dec 2016 21:03:42 +0800 From: Boqun Feng To: Mark Rutland 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: <20161214130342.GK9728@tardis.cn.ibm.com> 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> <20161214105506.GA17982@leverpostej> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="tBhgiDt8dP1efIIJ" Content-Disposition: inline In-Reply-To: <20161214105506.GA17982@leverpostej> User-Agent: Mutt/1.7.1 (2016-10-04) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --tBhgiDt8dP1efIIJ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Dec 14, 2016 at 10:55:07AM +0000, Mark Rutland wrote: > 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) =3D (rnp)->grplo + find _first_bit(mask, MASK_B= ITS(mask)); \ > > > > > (cpu) >=3D (rnp)->grplo && (cpu) <=3D (rnp)->grphi; \ > > > > > (cpu) =3D (rnp)->grplo + find _next_bit(mask, ..., > > > > > leaf_node_cpu_bit(rnp, cpu) + 1))) \ > > > > > if (!cpu_possible(cpu)) \ > > > > > continue; \ > > > > > else > > > >=20 > > > > What is the purpose of the cpu_possible() check? > > > >=20 > > >=20 > > > To filter out CPUs in range [grplo, grphi] but not in cpu_possible_ma= sk. > >=20 > > 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. > >=20 > > I think I probably will remove this check eventually, let me audit the > > code a little more for safety ;-) >=20 > 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. >=20 > That won't catch erroneous non-percpu accesses, but it covers part of > the problem, at least. ;) >=20 Good point ;-) I will also add a WARN_ON_ONCE() in macro for_each_leaf_node_cpu() and help me watch if anyone try to play an interesting game on those masks. Regards, Boqun > Thanks, > Mark. >=20 > ---->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 >=20 > 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. >=20 > 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. >=20 > As the option can adversely affect performance, it is disabled by > default. >=20 > Signed-off-by: Mark Rutland > --- > include/linux/percpu-defs.h | 16 ++++++++++++++-- > lib/Kconfig.debug | 10 ++++++++++ > 2 files changed, 24 insertions(+), 2 deletions(-) >=20 > 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) > =20 > +/* > + * __verify_pcpu_cpu() verifies that @cpu is possible, and hence has a v= alid > + * 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 > =20 > /* > @@ -219,8 +229,10 @@ > =20 > #define per_cpu_ptr(ptr, cpu) \ > ({ \ > + int ____cpu =3D (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))); \ > }) > =20 > #define raw_cpu_ptr(ptr) \ > @@ -247,7 +259,7 @@ > (typeof(*(__p)) __kernel __force *)(__p); \ > }) > =20 > -#define per_cpu_ptr(ptr, cpu) ({ (void)(cpu); VERIFY_PERCPU_PTR(ptr); }) > +#define per_cpu_ptr(ptr, cpu) ({ __verify_pcpu_cpu(cpu); VERIFY_PERCPU_P= TR(ptr); }) > #define raw_cpu_ptr(ptr) per_cpu_ptr(ptr, 0) > #define this_cpu_ptr(ptr) raw_cpu_ptr(ptr) > =20 > 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 > =20 > Say N if unsure. > =20 > +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 > --=20 > 1.9.1 >=20 --tBhgiDt8dP1efIIJ Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEcBAABCAAGBQJYUUMrAAoJEEl56MO1B/q4tUkH/1KhTBhWU5pT4Qqz8kGRX+Fy LKccWZYxRRCB7kbVykp9jURYHxB5Ig+FvuX5WsTJUCnGz3jQ3Xlg7cZXZq1CegL/ qgOo4XYSeXdojSfdq/EBnB1abEXCM9HdLJK+MB6LoBxNxs4M4VmhOzwofcXRPTDK pViH+XLESF1poYK3KtjdNLBbASqdsZR/teHUzyynm7ePTjuFs1ZrYxMaWeHBfxVq kWZ36ca2jn/jMk6BfWh0zN/F/oNu5SoQIYDKMolPtD9eM/Mz1JWnfA8XiJF6fN4i zlqTPFBj07FzrEw5E20JG030U8TTj+vPbYzKGyfPC7JobEQj627j355sMeAvbFY= =Tiy5 -----END PGP SIGNATURE----- --tBhgiDt8dP1efIIJ--