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" <paul...@linux.vnet.ibm.com>
> Cc: Josh Triplett <j...@joshtriplett.org>
> Cc: Steven Rostedt <rost...@goodmis.org>
> Cc: Mathieu Desnoyers <mathieu.desnoy...@efficios.com>
> Cc: Lai Jiangshan <jiangshan...@gmail.com>
> Signed-off-by: Kees Cook <keesc...@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?
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);
Otherwise, I don't see how do_nocb_deferred_wakeup_common() avoids a
NULL-pointer dereference.
> }
>
> /*
> --
> 2.7.4
>
>
> --
> Kees Cook
> Pixel Security
>
