« Return to Thread: [patch 0/3] net: Sanitizing hrtimer usage in net/sched/sch_cbq.c

Re: [patch 1/3] net: serialize hrtimer callback in sched_cbq

by Patrick McHardy :: Rate this Message:

David Miller wrote:

> From: Thomas Gleixner <tglx@...>
> Date: Thu, 09 Jul 2009 21:59:22 -0000
>
>> The hrtimer callback cbq_undelay() is not serialized against
>> cbq_ovl_delay(). That affects at least q->pmask and q->delay_timer.
>>
>> Lock it proper.
>>
>> Signed-off-by: Thomas Gleixner <tglx@...>
>
> The problems here are even much deeper than it appears.
>
> First of all, I am to understand that hrtimers run from hardware
> interrupt context, right? If so, all of these datastructures are
> softirq safe only.
>
> And it is not merely the immediate things you see being modified in
> this hrtimer, such as ->pmask etc., it is also the q->active[]
> pointers, the list state for the classes, just about everything in the
> qdisc state is referenced in this hrtimer code path.
>
> I wonder how many queer unexplainable bugs we see because of this.
>
> What should probably happen is that the hrtimer merely fires off work
> at software interrupt context (perhaps a tasklet or similar), and that
> software interrupt code take the qdisc's root lock throughout it's
> execution.

That's my understanding what HRTIMER_SOFTIRQ is used for. I think
simply grabbing the root lock in cbq_undelay() should be fine.

Compile-tested only.

commit a790fb8873f1cbe8b9cb48cb368851e30d3ec172
Author: Patrick McHardy <kaber@...>
Date: Tue Jul 14 10:19:47 2009 +0200

net-sched: sch_cbq: fix locking in cbq_undelay()

The hrtimer callback cbq_undelay() is not serialized against
cbq_ovl_delay(). That affects at least q->pmask and q->delay_timer.

Lock it proper.

Based on patch by Thomas Gleixner <tglx@...>

Signed-off-by: Patrick McHardy <kaber@...>

diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
index 23a1676..7c659c6 100644
--- a/net/sched/sch_cbq.c
+++ b/net/sched/sch_cbq.c
@@ -593,12 +593,16 @@ static enum hrtimer_restart cbq_undelay(struct hrtimer *timer)
struct cbq_sched_data *q = container_of(timer, struct cbq_sched_data,
delay_timer);
struct Qdisc *sch = q->watchdog.qdisc;
+ spinlock_t *root_lock;
psched_time_t now;
psched_tdiff_t delay = 0;
unsigned pmask;

now = psched_get_time();

+ root_lock = qdisc_lock(qdisc_root(sch));
+ spin_lock(root_lock);
+
pmask = q->pmask;
q->pmask = 0;

@@ -615,6 +619,7 @@ static enum hrtimer_restart cbq_undelay(struct hrtimer *timer)
delay = tmp;
}
}
+ spin_unlock(root_lock);

if (delay) {
ktime_t time;

« Return to Thread: [patch 0/3] net: Sanitizing hrtimer usage in net/sched/sch_cbq.c