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
> <[email protected]> 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" <[email protected]>
> >> Cc: Josh Triplett <[email protected]>
> >> Cc: Steven Rostedt <[email protected]>
> >> Cc: Mathieu Desnoyers <[email protected]>
> >> Cc: Lai Jiangshan <[email protected]>
> >> Signed-off-by: Kees Cook <[email protected]>
> >
> > 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
