From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joel Fernandes Date: Thu, 21 Feb 2019 12:13:11 -0500 Subject: [Intel-wired-lan] [PATCH RFC 3/5] sched/cpufreq: Fix incorrect RCU API usage In-Reply-To: <20190221161144.GU32494@hirez.programming.kicks-ass.net> References: <20190221054942.132388-1-joel@joelfernandes.org> <20190221054942.132388-4-joel@joelfernandes.org> <20190221091805.GX32477@hirez.programming.kicks-ass.net> <20190221152139.GB19213@google.com> <20190221153117.GT32494@hirez.programming.kicks-ass.net> <20190221155218.GZ11787@linux.ibm.com> <20190221161144.GU32494@hirez.programming.kicks-ass.net> Message-ID: <20190221171311.GA118415@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: intel-wired-lan@osuosl.org List-ID: On Thu, Feb 21, 2019 at 05:11:44PM +0100, Peter Zijlstra wrote: > On Thu, Feb 21, 2019 at 07:52:18AM -0800, Paul E. McKenney wrote: > > On Thu, Feb 21, 2019 at 04:31:17PM +0100, Peter Zijlstra wrote: > > > On Thu, Feb 21, 2019 at 10:21:39AM -0500, Joel Fernandes wrote: > > > > On Thu, Feb 21, 2019 at 10:18:05AM +0100, Peter Zijlstra wrote: > > > > > On Thu, Feb 21, 2019 at 12:49:40AM -0500, Joel Fernandes (Google) wrote: > > > > > > @@ -34,8 +34,12 @@ void cpufreq_add_update_util_hook(int cpu, struct update_util_data *data, > > > > > > if (WARN_ON(!data || !func)) > > > > > > return; > > > > > > > > > > > > - if (WARN_ON(per_cpu(cpufreq_update_util_data, cpu))) > > > > > > + rcu_read_lock(); > > > > > > + if (WARN_ON(rcu_dereference(per_cpu(cpufreq_update_util_data, cpu)))) { > > > > > > + rcu_read_unlock(); > > > > > > return; > > > > > > + } > > > > > > + rcu_read_unlock(); > > > > > > > > > > > > data->func = func; > > > > > > rcu_assign_pointer(per_cpu(cpufreq_update_util_data, cpu), data); > > > For whatever it is worth, in that case it could use rcu_access_pointer(). > > And this primitive does not do the lockdep check for being within an RCU > > read-side critical section. As Peter says, if there is no dereferencing, > > there can be no use-after-free bug, so the RCU read-side critical is > > not needed. > > On top of that, I suspect this is under the write-side lock (we're doing > assignment after all). Yes it is under a policy->rwsem, just confirmed that. And indeed rcu_read_lock() is not needed here in this patch, thanks for pointing that out. Sometimes the word "dereference" plays some visual tricks and in this case tempted me to add an RCU reader section ;-) Assuming you guys are in agreement with usage of rcu_access_pointer() here to keep sparse happy, I'll rework the patch accordingly and resubmit with that. thanks! - Joel From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.6 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS, USER_AGENT_MUTT autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id BFC15C43381 for ; Thu, 21 Feb 2019 17:13:20 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 8DCEB2083B for ; Thu, 21 Feb 2019 17:13:20 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=joelfernandes.org header.i=@joelfernandes.org header.b="NUkDyIfC" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726232AbfBURNT (ORCPT ); Thu, 21 Feb 2019 12:13:19 -0500 Received: from mail-qk1-f194.google.com ([209.85.222.194]:36524 "EHLO mail-qk1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726371AbfBURNP (ORCPT ); Thu, 21 Feb 2019 12:13:15 -0500 Received: by mail-qk1-f194.google.com with SMTP id o125so4712131qkf.3 for ; Thu, 21 Feb 2019 09:13:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=joelfernandes.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=41DJd/jPexbjxvgsSyMtotGUuKs1Czyv7tGoT1h60KM=; b=NUkDyIfCjApsV4ZYCcxVhXbcYB4tde9vxi5EkAlTQvXRu9Gl1dkXcu7a5kLo03gyil Snw+QY1LdgTcXMaa/cluhgr+XbKsR9vJYfV/DkmZw+z3uuQq+9MufIgHLK7iDg04OZ2C yfBCIYjdUWWXF3IlPKAGbJeSGSoedJNcxluig= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=41DJd/jPexbjxvgsSyMtotGUuKs1Czyv7tGoT1h60KM=; b=UmG+ssJgkHq71FejOd/BmB8ni0lqUeu5zu8bCAJB32rzD9F3XUI6I+E17UhZHD7sEf muxpXQDH6q62gLFw1r5iPMLT+CAXSLdy/xKlPL84NII3YidOOekPRldupPT2nn7dPLVM nS9lMwIkA33kFNhb9l+PU46yjbu1g7732vV8vcPoj5ziTWO048bjoaZO++k5IPj/R0Z6 WIV9Wnkh6CkeepHyUxdesXb9yol2igRIYFlLCEmED2uHVCzahoTx55ZxJBlchWinCDhy 9oDjbocXlo6XvFoEssh9lgC52vnPRNq44/Agq20PEoVy7o+z3By5zbM2jpBPPX4YmtP/ rZfg== X-Gm-Message-State: AHQUAuag1cdn12jnzXjGGxDjDRkJXHo0GDDKWQ+6ox2luR1vCakjKe/f n8WLr9AAfaKKaWKyKA11Xp4YrQ== X-Google-Smtp-Source: AHgI3IbfzzzZ88sploh4dDYxzRY2NObMLMZPcHodt+kMbEUHkhN7VjWE2CL6nLEKzCfDDednB73/Ug== X-Received: by 2002:a37:83c6:: with SMTP id f189mr9459033qkd.196.1550769194098; Thu, 21 Feb 2019 09:13:14 -0800 (PST) Received: from localhost ([2620:0:1004:1100:cca9:fccc:8667:9bdc]) by smtp.gmail.com with ESMTPSA id 14sm323833qkf.23.2019.02.21.09.13.12 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 21 Feb 2019 09:13:12 -0800 (PST) Date: Thu, 21 Feb 2019 12:13:11 -0500 From: Joel Fernandes To: Peter Zijlstra Cc: "Paul E. McKenney" , linux-kernel@vger.kernel.org, Alexei Starovoitov , Christian Brauner , Daniel Borkmann , David Ahern , "David S. Miller" , Ido Schimmel , Ingo Molnar , "moderated list:INTEL ETHERNET DRIVERS" , Jakub Kicinski , Jeff Kirsher , Jesper Dangaard Brouer , John Fastabend , Josh Triplett , keescook@chromium.org, Lai Jiangshan , Martin KaFai Lau , Mathieu Desnoyers , netdev@vger.kernel.org, rcu@vger.kernel.org, Song Liu , Steven Rostedt , xdp-newbies@vger.kernel.org, Yonghong Song Subject: Re: [PATCH RFC 3/5] sched/cpufreq: Fix incorrect RCU API usage Message-ID: <20190221171311.GA118415@google.com> References: <20190221054942.132388-1-joel@joelfernandes.org> <20190221054942.132388-4-joel@joelfernandes.org> <20190221091805.GX32477@hirez.programming.kicks-ass.net> <20190221152139.GB19213@google.com> <20190221153117.GT32494@hirez.programming.kicks-ass.net> <20190221155218.GZ11787@linux.ibm.com> <20190221161144.GU32494@hirez.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190221161144.GU32494@hirez.programming.kicks-ass.net> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: rcu-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: rcu@vger.kernel.org On Thu, Feb 21, 2019 at 05:11:44PM +0100, Peter Zijlstra wrote: > On Thu, Feb 21, 2019 at 07:52:18AM -0800, Paul E. McKenney wrote: > > On Thu, Feb 21, 2019 at 04:31:17PM +0100, Peter Zijlstra wrote: > > > On Thu, Feb 21, 2019 at 10:21:39AM -0500, Joel Fernandes wrote: > > > > On Thu, Feb 21, 2019 at 10:18:05AM +0100, Peter Zijlstra wrote: > > > > > On Thu, Feb 21, 2019 at 12:49:40AM -0500, Joel Fernandes (Google) wrote: > > > > > > @@ -34,8 +34,12 @@ void cpufreq_add_update_util_hook(int cpu, struct update_util_data *data, > > > > > > if (WARN_ON(!data || !func)) > > > > > > return; > > > > > > > > > > > > - if (WARN_ON(per_cpu(cpufreq_update_util_data, cpu))) > > > > > > + rcu_read_lock(); > > > > > > + if (WARN_ON(rcu_dereference(per_cpu(cpufreq_update_util_data, cpu)))) { > > > > > > + rcu_read_unlock(); > > > > > > return; > > > > > > + } > > > > > > + rcu_read_unlock(); > > > > > > > > > > > > data->func = func; > > > > > > rcu_assign_pointer(per_cpu(cpufreq_update_util_data, cpu), data); > > > For whatever it is worth, in that case it could use rcu_access_pointer(). > > And this primitive does not do the lockdep check for being within an RCU > > read-side critical section. As Peter says, if there is no dereferencing, > > there can be no use-after-free bug, so the RCU read-side critical is > > not needed. > > On top of that, I suspect this is under the write-side lock (we're doing > assignment after all). Yes it is under a policy->rwsem, just confirmed that. And indeed rcu_read_lock() is not needed here in this patch, thanks for pointing that out. Sometimes the word "dereference" plays some visual tricks and in this case tempted me to add an RCU reader section ;-) Assuming you guys are in agreement with usage of rcu_access_pointer() here to keep sparse happy, I'll rework the patch accordingly and resubmit with that. thanks! - Joel