* [PATCH] rcu: Convert timers to use timer_setup() @ 2017-10-24 9:32 Kees Cook 2017-10-25 14:17 ` Kees Cook 2017-10-30 18:04 ` Paul E. McKenney 0 siblings, 2 replies; 6+ messages in thread From: Kees Cook @ 2017-10-24 9:32 UTC (permalink / raw) To: Paul E. McKenney Cc: Josh Triplett, Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, linux-kernel 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> --- 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); + + 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] rcu: Convert timers to use timer_setup() 2017-10-24 9:32 [PATCH] rcu: Convert timers to use timer_setup() Kees Cook @ 2017-10-25 14:17 ` Kees Cook 2017-10-30 18:05 ` Paul E. McKenney 2017-10-30 18:04 ` Paul E. McKenney 1 sibling, 1 reply; 6+ messages in thread From: Kees Cook @ 2017-10-25 14:17 UTC (permalink / raw) To: Paul E. McKenney Cc: Josh Triplett, Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, LKML 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 On Tue, Oct 24, 2017 at 11:32 AM, Kees Cook <keescook@chromium.org> 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> > --- > 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); > + > + 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 -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rcu: Convert timers to use timer_setup() 2017-10-25 14:17 ` Kees Cook @ 2017-10-30 18:05 ` Paul E. McKenney 0 siblings, 0 replies; 6+ messages in thread From: Paul E. McKenney @ 2017-10-30 18:05 UTC (permalink / raw) To: Kees Cook Cc: Josh Triplett, Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, LKML On Wed, Oct 25, 2017 at 04:17:32PM +0200, Kees Cook wrote: > 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 > Thanks! > > -Kees > > On Tue, Oct 24, 2017 at 11:32 AM, Kees Cook <keescook@chromium.org> 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> > > --- > > 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); > > + > > + 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 > > > > -- > Kees Cook > Pixel Security > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rcu: Convert timers to use timer_setup() 2017-10-24 9:32 [PATCH] rcu: Convert timers to use timer_setup() Kees Cook 2017-10-25 14:17 ` Kees Cook @ 2017-10-30 18:04 ` Paul E. McKenney 2017-10-30 21:13 ` Kees Cook 1 sibling, 1 reply; 6+ messages in thread From: Paul E. McKenney @ 2017-10-30 18:04 UTC (permalink / raw) To: Kees Cook Cc: Josh Triplett, Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, linux-kernel 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? 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rcu: Convert timers to use timer_setup() 2017-10-30 18:04 ` Paul E. McKenney @ 2017-10-30 21:13 ` Kees Cook 2017-10-30 21:22 ` Paul E. McKenney 0 siblings, 1 reply; 6+ messages in thread From: Kees Cook @ 2017-10-30 21:13 UTC (permalink / raw) To: Paul E. McKenney Cc: Josh Triplett, Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, LKML 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 > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rcu: Convert timers to use timer_setup() 2017-10-30 21:13 ` Kees Cook @ 2017-10-30 21:22 ` Paul E. McKenney 0 siblings, 0 replies; 6+ messages in thread From: Paul E. McKenney @ 2017-10-30 21:22 UTC (permalink / raw) To: Kees Cook Cc: Josh Triplett, Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, LKML 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-10-30 21:22 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-10-24 9:32 [PATCH] rcu: Convert timers to use timer_setup() Kees Cook 2017-10-25 14:17 ` Kees Cook 2017-10-30 18:05 ` Paul E. McKenney 2017-10-30 18:04 ` Paul E. McKenney 2017-10-30 21:13 ` Kees Cook 2017-10-30 21:22 ` Paul E. McKenney
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox