From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932904AbdJ3VWf (ORCPT ); Mon, 30 Oct 2017 17:22:35 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:56256 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932608AbdJ3VWe (ORCPT ); Mon, 30 Oct 2017 17:22:34 -0400 Date: Mon, 30 Oct 2017 14:22:31 -0700 From: "Paul E. McKenney" To: Kees Cook Cc: Josh Triplett , Steven Rostedt , Mathieu Desnoyers , Lai Jiangshan , LKML Subject: Re: [PATCH] rcu: Convert timers to use timer_setup() Reply-To: paulmck@linux.vnet.ibm.com References: <20171024093204.GA47418@beast> <20171030180446.GY3659@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-GCONF: 00 x-cbid: 17103021-0036-0000-0000-00000283FA7D X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00007982; HX=3.00000241; KW=3.00000007; PH=3.00000004; SC=3.00000239; SDB=6.00938796; UDB=6.00473247; IPR=6.00719057; BA=6.00005663; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00017796; XFM=3.00000015; UTC=2017-10-30 21:22:32 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17103021-0037-0000-0000-0000423CA07D Message-Id: <20171030212231.GC3659@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-10-30_07:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000 definitions=main-1710300283 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Oct 30, 2017 at 02:13:03PM -0700, Kees Cook wrote: > On Mon, Oct 30, 2017 at 11:04 AM, Paul E. McKenney > wrote: > > On Tue, Oct 24, 2017 at 02:32:04AM -0700, Kees Cook wrote: > >> In preparation for unconditionally passing the struct timer_list pointer to > >> all timer callbacks, switch to using the new timer_setup() and from_timer() > >> to pass the timer pointer explicitly. > >> > >> Cc: "Paul E. McKenney" > >> Cc: Josh Triplett > >> Cc: Steven Rostedt > >> Cc: Mathieu Desnoyers > >> Cc: Lai Jiangshan > >> Signed-off-by: Kees Cook > > > > One question below. > > > > Thanx, Paul > > > >> --- > >> kernel/rcu/rcutorture.c | 4 ++-- > >> kernel/rcu/tree_plugin.h | 9 +++++---- > >> 2 files changed, 7 insertions(+), 6 deletions(-) > >> > >> diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c > >> index e1d3fa534ac0..b6fbbeb5a7da 100644 > >> --- a/kernel/rcu/rcutorture.c > >> +++ b/kernel/rcu/rcutorture.c > >> @@ -1078,7 +1078,7 @@ static void rcu_torture_timer_cb(struct rcu_head *rhp) > >> * counter in the element should never be greater than 1, otherwise, the > >> * RCU implementation is broken. > >> */ > >> -static void rcu_torture_timer(unsigned long unused) > >> +static void rcu_torture_timer(struct timer_list *unused) > >> { > >> int idx; > >> unsigned long started; > >> @@ -1165,7 +1165,7 @@ rcu_torture_reader(void *arg) > >> VERBOSE_TOROUT_STRING("rcu_torture_reader task started"); > >> set_user_nice(current, MAX_NICE); > >> if (irqreader && cur_ops->irq_capable) > >> - setup_timer_on_stack(&t, rcu_torture_timer, 0); > >> + timer_setup_on_stack(&t, rcu_torture_timer, 0); > >> > >> do { > >> if (irqreader && cur_ops->irq_capable) { > >> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h > >> index 8a5a3f9b1250..5fb7beee76aa 100644 > >> --- a/kernel/rcu/tree_plugin.h > >> +++ b/kernel/rcu/tree_plugin.h > >> @@ -2265,9 +2265,11 @@ static void do_nocb_deferred_wakeup_common(struct rcu_data *rdp) > >> } > >> > >> /* Do a deferred wakeup of rcu_nocb_kthread() from a timer handler. */ > >> -static void do_nocb_deferred_wakeup_timer(unsigned long x) > >> +static void do_nocb_deferred_wakeup_timer(struct timer_list *t) > >> { > >> - do_nocb_deferred_wakeup_common((struct rcu_data *)x); > >> + struct rcu_data *x = from_timer(x, t, nocb_timer); > > > > As long as we are creating a real typed variable for this could we > > please call it "rdp" in order to follow the usual RCU conventions? > > Sure thing! My scripts had tried to minimize variable name churn, but > this makes much more sense for what's happening in this function. > > > > > struct rcu_data *rdp = from_timer(rdp, t, nocb_timer); > > > >> + > >> + do_nocb_deferred_wakeup_common(x); > > > > And of course here: > > > > do_nocb_deferred_wakeup_common(rdp); > > > >> } > >> > >> /* > >> @@ -2331,8 +2333,7 @@ static void __init rcu_boot_init_nocb_percpu_data(struct rcu_data *rdp) > >> init_swait_queue_head(&rdp->nocb_wq); > >> rdp->nocb_follower_tail = &rdp->nocb_follower_head; > >> raw_spin_lock_init(&rdp->nocb_lock); > >> - setup_timer(&rdp->nocb_timer, do_nocb_deferred_wakeup_timer, > >> - (unsigned long)rdp); > >> + timer_setup(&rdp->nocb_timer, do_nocb_deferred_wakeup_timer, 0); > > > > Shouldn't this instead be something like this, give or take casts? > > > > timer_setup(&rdp->nocb_timer, do_nocb_deferred_wakeup_timer, rdp); > > Nope, the new timer API will unconditionally pass the timer pointer > (&rdp->nocb_timer) to the callback. The 3rd argument is flags. > > > Otherwise, I don't see how do_nocb_deferred_wakeup_common() avoids a > > NULL-pointer dereference. > > You can see the intermediate step to the API here: > > https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=timers/core&id=686fef928bba6be13cabe639f154af7d72b63120 Ah, got it, from_timer() does the needed conversion. > > Given the fixes I called out, I am fine with your carrying it separately. > > I'll adjust the variable name and carry it in the timer tree. Thanks! Sounds good! Thanx, Paul