Bug in netem reordering

Hi,

I am getting a crash when I test the reordering feature of netem. This
happens every time I run the following tc  command and then pass some
traffic through the
interface. I am using the latest net-next kernel.

#tc qdisc add dev eth0 root netem delay 10ms reorder 25% 50%

Then wait for several packets to go through.

The problem seems to be in netem_enqueue(). Part of the code for
reordering is as follows:

 } else {
                /*
                 * Do re-ordering by putting one out of N packets at the front
                 * of the queue.
                 */
                cb->time_to_send = psched_get_time();
                q->counter = 0;

                __skb_queue_head(&sch->q, skb);
                q->qdisc->qstats.backlog += qdisc_pkt_len(skb);
                q->qdisc->qstats.requeues++;
                ret = NET_XMIT_SUCCESS;
        }


The issue is that q->qdisc is initialized to NULL (by netem_init? ).
When the else branch is executed as above after a few packets,
q->qdisc is NULL and we get a crash.
Can anyone else reproduce the problem?

Regards,
Vijay Subramanian

Not now, but it looks you are correct. q->qdisc is NULL until another
additional qdisc is attached (beside tfifo). See 50612537e9ab2969312.
The following patch should work.

From: Hagen Paul Pfeifer <redacted>

netem: catch NULL pointer by updating the real qdisc statistic

Reported-by: Vijay Subramanian <redacted>
Signed-off-by: Hagen Paul Pfeifer <redacted>
---
 net/sched/sch_netem.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 06a5ceb..cc29d18 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c

@@ -458,8 +458,8 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 		q->counter = 0;
 
 		__skb_queue_head(&sch->q, skb);
-		q->qdisc->qstats.backlog += qdisc_pkt_len(skb);
-		q->qdisc->qstats.requeues++;
+		sch->qstats.backlog += qdisc_pkt_len(skb);
+		sch->qstats.requeues++;
 		ret = NET_XMIT_SUCCESS;
 	}
 

-- 
1.7.7.3

Le jeudi 05 janvier 2012 à 04:35 +0100, Hagen Paul Pfeifer a écrit :

Not now, but it looks you are correct. q->qdisc is NULL until another
additional qdisc is attached (beside tfifo). See 50612537e9ab2969312.
The following patch should work.

From: Hagen Paul Pfeifer <redacted>

netem: catch NULL pointer by updating the real qdisc statistic

Reported-by: Vijay Subramanian <redacted>
Signed-off-by: Hagen Paul Pfeifer <redacted>
---

Thanks for this fix !

Acked-by: Eric Dumazet <redacted>

From: Eric Dumazet <redacted>
Date: Thu, 05 Jan 2012 06:34:16 +0100

Le jeudi 05 janvier 2012 à 04:35 +0100, Hagen Paul Pfeifer a écrit :

quoted

Not now, but it looks you are correct. q->qdisc is NULL until another
additional qdisc is attached (beside tfifo). See 50612537e9ab2969312.
The following patch should work.

From: Hagen Paul Pfeifer <redacted>

netem: catch NULL pointer by updating the real qdisc statistic

Reported-by: Vijay Subramanian <redacted>
Signed-off-by: Hagen Paul Pfeifer <redacted>
---

Thanks for this fix !

Acked-by: Eric Dumazet <redacted>

Applied, thanks.