Re: [PATCH] rcu: Convert timers to use timer_setup()

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

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
> <paulmck@linux.vnet.ibm.com> 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" <paulmck@linux.vnet.ibm.com>
> >> Cc: Josh Triplett <josh@joshtriplett.org>
> >> Cc: Steven Rostedt <rostedt@goodmis.org>
> >> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> >> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> >> Signed-off-by: Kees Cook <keescook@chromium.org>
> >
> > 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