rcu: Convert timers to use timer_setup()

PATCH In review Kees Cook 2 participants 6 messages Oct 24, 2017 – Oct 30, 2017

Subsystems (2)

RCU Torture-test modules

lore

Share & tools

lore

Kees Cook

author

2017-10-24 09:32 UTC #0 lore raw

PATCH 1/1 rcu: Convert timers to use timer_setup()

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

---
 kernel/rcu/rcutorture.c  | 4 ++--
 kernel/rcu/tree_plugin.h | 9 +++++----
 2 files changed, 7 insertions(+), 6 deletions(-)

kernel/rcu/rcutorture.c +2 -2

index e1d3fa534ac0..b6fbbeb5a7da 100644
@@ -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) {

kernel/rcu/tree_plugin.h +5 -4

index 8a5a3f9b1250..5fb7beee76aa 100644
@@ -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);
+
+	do_nocb_deferred_wakeup_common(x);
 }
 
 /*

@@ -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);
 }
 
 /*

Signature · 6 lines

2.7.4


-- 
Kees Cook
Pixel Security

Kees Cook

author

2017-10-25 14:17 UTC #1 lore raw

Sorry, I sent this and forgot that timer_setup_on_stack() is only in -next. If this patch is okay, I can carry it in the timers tree.

Thanks!

-Kees

Quoted 72 lines · Kees Cook

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

---
 kernel/rcu/rcutorture.c  | 4 ++--
 kernel/rcu/tree_plugin.h | 9 +++++----
 2 files changed, 7 insertions(+), 6 deletions(-)

kernel/rcu/rcutorture.c +2 -2

index e1d3fa534ac0..b6fbbeb5a7da 100644
@@ -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) {

kernel/rcu/tree_plugin.h +5 -5

index 8a5a3f9b1250..5fb7beee76aa 100644
@@ -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);
+
+       do_nocb_deferred_wakeup_common(x);
 }
 
 /*

@@ -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);
 }
 
 /*
--

2.7.4

--
Kees Cook
Pixel Security

Signature · 2 lines

Kees Cook
Pixel Security

Paul E. McKenney

2017-10-30 18:04 UTC #3 lore raw

In reply to Kees Cook

Quoted 10 lines · Kees Cook

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

Quoted 32 lines

---
 kernel/rcu/rcutorture.c  | 4 ++--
 kernel/rcu/tree_plugin.h | 9 +++++----
 2 files changed, 7 insertions(+), 6 deletions(-)

kernel/rcu/rcutorture.c +2 -2

index e1d3fa534ac0..b6fbbeb5a7da 100644
@@ -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) {

kernel/rcu/tree_plugin.h +0 -0

index 8a5a3f9b1250..5fb7beee76aa 100644

kernel/rcu/tree_plugin.h static void do_nocb_deferred_wakeup_common(struct rcu_data *rdp) · line 2270

Quoted 8 lines

kernel/rcu/tree_plugin.h +2 -2

@@ -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);

kernel/rcu/tree_plugin.h static void do_nocb_deferred_wakeup_common(struct rcu_data *rdp) · line 2272

Quoted 2 lines

kernel/rcu/tree_plugin.h +2 -0
+
+	do_nocb_deferred_wakeup_common(x);

And of course here:

	do_nocb_deferred_wakeup_common(rdp);

kernel/rcu/tree_plugin.h static void __init rcu_boot_init_nocb_percpu_data(struct rcu_data *rdp) · line 2336

Quoted 10 lines

kernel/rcu/tree_plugin.h +1 -2

 }
 
 /*

@@ -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.

Quoted 11 lines

kernel/rcu/tree_plugin.h +0 -0
 }
 
 /*
Signature · 6 lines
2.7.4


-- 
Kees Cook
Pixel Security

Paul E. McKenney

2017-10-30 18:05 UTC #2 lore raw

In reply to Kees Cook

Quoted 2 lines · Kees Cook

Sorry, I sent this and forgot that timer_setup_on_stack() is only in -next. If this patch is okay, I can carry it in the timers tree.

Given the fixes I called out, I am fine with your carrying it separately.

Thanx, Paul

Quoted 84 lines

Thanks!

-Kees

Quoted 72 lines · Kees Cook

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

---
 kernel/rcu/rcutorture.c  | 4 ++--
 kernel/rcu/tree_plugin.h | 9 +++++----
 2 files changed, 7 insertions(+), 6 deletions(-)

kernel/rcu/rcutorture.c +2 -2

index e1d3fa534ac0..b6fbbeb5a7da 100644
@@ -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) {

kernel/rcu/tree_plugin.h +5 -5

index 8a5a3f9b1250..5fb7beee76aa 100644
@@ -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);
+
+       do_nocb_deferred_wakeup_common(x);
 }
 
 /*

@@ -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);
 }
 
 /*
--

2.7.4

--
Kees Cook
Pixel Security

Signature · 2 lines

Kees Cook
Pixel Security

Kees Cook

author

2017-10-30 21:13 UTC #4 lore raw

In reply to Paul E. McKenney

Quoted 50 lines · Paul E. McKenney

Quoted 10 lines · Kees Cook

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

Quoted 34 lines

---
 kernel/rcu/rcutorture.c  | 4 ++--
 kernel/rcu/tree_plugin.h | 9 +++++----
 2 files changed, 7 insertions(+), 6 deletions(-)

kernel/rcu/rcutorture.c +2 -2

index e1d3fa534ac0..b6fbbeb5a7da 100644
@@ -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) {

kernel/rcu/tree_plugin.h +0 -0

index 8a5a3f9b1250..5fb7beee76aa 100644
@@ -2265,9 +2265,11 @@ static void do_nocb_deferred_wakeup_common(struct rcu_data *rdp)
 }

Quoted 9 lines

kernel/rcu/tree_plugin.h static void do_nocb_deferred_wakeup_common(struct rcu_data *rdp) · line 2270
Quoted 6 lines
kernel/rcu/tree_plugin.h +2 -2
 /* 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.

Quoted 23 lines

        struct rcu_data *rdp = from_timer(rdp, t, nocb_timer);
kernel/rcu/tree_plugin.h static void do_nocb_deferred_wakeup_common(struct rcu_data *rdp) · line 2272
Quoted 2 lines
kernel/rcu/tree_plugin.h +2 -0
+
+     do_nocb_deferred_wakeup_common(x);
And of course here:
        do_nocb_deferred_wakeup_common(rdp);
kernel/rcu/tree_plugin.h static void __init rcu_boot_init_nocb_percpu_data(struct rcu_data *rdp) · line 2336
Quoted 10 lines
kernel/rcu/tree_plugin.h +1 -2
 }
 
 /*
@@ -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?

Quoted 1 line

        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.

Quoted 2 lines

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

Quoted 1 line

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

Signature · 2 lines

Kees Cook
Pixel Security

Paul E. McKenney

2017-10-30 21:22 UTC #5 lore raw

Quoted 93 lines · Kees Cook

Quoted 50 lines · Paul E. McKenney
Quoted 10 lines · Kees Cook
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
Quoted 34 lines
---
 kernel/rcu/rcutorture.c  | 4 ++--
 kernel/rcu/tree_plugin.h | 9 +++++----
 2 files changed, 7 insertions(+), 6 deletions(-)
kernel/rcu/rcutorture.c +2 -2
index e1d3fa534ac0..b6fbbeb5a7da 100644
@@ -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) {
kernel/rcu/tree_plugin.h +0 -0
index 8a5a3f9b1250..5fb7beee76aa 100644
@@ -2265,9 +2265,11 @@ static void do_nocb_deferred_wakeup_common(struct rcu_data *rdp)
 }
 
Quoted 9 lines
kernel/rcu/tree_plugin.h static void do_nocb_deferred_wakeup_common(struct rcu_data *rdp) · line 2270
Quoted 6 lines
kernel/rcu/tree_plugin.h +2 -2
 /* 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.
Quoted 23 lines
        struct rcu_data *rdp = from_timer(rdp, t, nocb_timer);
kernel/rcu/tree_plugin.h static void do_nocb_deferred_wakeup_common(struct rcu_data *rdp) · line 2272
Quoted 2 lines
kernel/rcu/tree_plugin.h +2 -0
+
+     do_nocb_deferred_wakeup_common(x);
And of course here:
        do_nocb_deferred_wakeup_common(rdp);
kernel/rcu/tree_plugin.h static void __init rcu_boot_init_nocb_percpu_data(struct rcu_data *rdp) · line 2336
Quoted 10 lines
kernel/rcu/tree_plugin.h +1 -2
 }
 
 /*
@@ -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?
Quoted 1 line
        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.

Quoted 6 lines

Quoted 2 lines
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.

Quoted 3 lines

Quoted 1 line
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