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

[Kees Cook]

On Mon, Oct 30, 2017 at 11:04 AM, Paul E. McKenney
<paulmck@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
>> Cc: Josh Triplett <josh@xxxxxxxxxxxxxxxx>
>> Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
>> Cc: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>
>> Cc: Lai Jiangshan <jiangshanlai@xxxxxxxxx>
>> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
>
> 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

> 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!

-Kees

-- 
Kees Cook
Pixel Security