[PATCH net] net: sch_red: Fix race between timer and red_destroy()

[PATCH net] net: sch_red: Fix race between timer and red_destroy()

[Vijay Subramanian]

Make sure timer does not fire once qdisc is destroyed.

Signed-off-by: Vijay Subramanian <subramanian.vijay@gmail.com>
---
 net/sched/sch_red.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
index 633e32d..380507e 100644
--- a/net/sched/sch_red.c
+++ b/net/sched/sch_red.c
@@ -39,6 +39,7 @@
 struct red_sched_data {
 	u32			limit;		/* HARD maximal queue length */
 	unsigned char		flags;
+	bool			timeron;	/* to prevent race on destroy*/
 	struct timer_list	adapt_timer;
 	struct red_parms	parms;
 	struct red_vars		vars;
@@ -166,6 +167,7 @@ static void red_destroy(struct Qdisc *sch)
 {
 	struct red_sched_data *q = qdisc_priv(sch);
 
+	q->timeron = false;
 	del_timer_sync(&q->adapt_timer);
 	qdisc_destroy(q->qdisc);
 }
@@ -241,7 +243,8 @@ static inline void red_adaptative_timer(unsigned long arg)
 
 	spin_lock(root_lock);
 	red_adaptative_algo(&q->parms, &q->vars);
-	mod_timer(&q->adapt_timer, jiffies + HZ/2);
+	if (q->timeron)
+		mod_timer(&q->adapt_timer, jiffies + HZ/2);
 	spin_unlock(root_lock);
 }
 
@@ -250,6 +253,7 @@ static int red_init(struct Qdisc *sch, struct nlattr *opt)
 	struct red_sched_data *q = qdisc_priv(sch);
 
 	q->qdisc = &noop_qdisc;
+	q->timeron = true;
 	setup_timer(&q->adapt_timer, red_adaptative_timer, (unsigned long)sch);
 	return red_change(sch, opt);
 }
-- 
1.7.9.5

Re: [PATCH net] net: sch_red: Fix race between timer and red_destroy()

[David Miller]

From: Vijay Subramanian <subramanian.vijay@gmail.com>
Date: Mon,  4 Nov 2013 18:08:16 -0800

> Make sure timer does not fire once qdisc is destroyed.
> 
> Signed-off-by: Vijay Subramanian <subramanian.vijay@gmail.com>

I do not understand how this problem can trigger.

del_timer_sync() waits for all instances of the timer handler which
are running to complete, and then it deletes the timer from any of
the scheduled timer lists.

Your patch is making the mod_timer() in the timer handler conditional,
and this really shouldn't be necessary.

The whole point of del_timer_sync() is to address these kinds of
situations.

Re: [PATCH net] net: sch_red: Fix race between timer and red_destroy()

[Vijay Subramanian]

> del_timer_sync() waits for all instances of the timer handler which
> are running to complete, and then it deletes the timer from any of
> the scheduled timer lists.
>
> Your patch is making the mod_timer() in the timer handler conditional,
> and this really shouldn't be necessary.
>
> The whole point of del_timer_sync() is to address these kinds of
> situations.


Thanks for the explanation Dave. I am trying to reconcile your
explanation with the text above del_timer_sync()
vim +1013 kernel/timer.c

" * This function only differs from del_timer() on SMP: besides deactivating
 * the timer it also makes sure the handler has finished executing on other
 * CPUs.
 *
 * Synchronization rules: Callers must prevent restarting of the timer,
 * otherwise this function is meaningless.  ..."

This seems to suggest simply calling del_timer_sync() is not enough.
In fact, for the same reason  I was recently pointed towards
commit 980c478ddbb72 (sch_sfq: use del_timer_sync() in sfq_destroy() )
and I based the patch on how q->perturb_period is reset in
sfq_destroy() and used in sfq_perturbation().

Based on your explanation , I think we do not need (in sched/sch_sfq.c)
q->perturb_period = 0;  in sfq_destroy() and just del_timer_sync()
should be enough.

Thanks,
Vijay