Bug in netem reordering

Bug in netem reordering

[Vijay Subramanian]

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

Re: Bug in netem reordering

[Hagen Paul Pfeifer]

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 <hagen@jauu.net>

netem: catch NULL pointer by updating the real qdisc statistic

Reported-by: Vijay Subramanian <subramanian.vijay@gmail.com>
Signed-off-by: Hagen Paul Pfeifer <hagen@jauu.net>
---
 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

Re: Bug in netem reordering

[Eric Dumazet]

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 <hagen@jauu.net>
> 
> netem: catch NULL pointer by updating the real qdisc statistic
> 
> Reported-by: Vijay Subramanian <subramanian.vijay@gmail.com>
> Signed-off-by: Hagen Paul Pfeifer <hagen@jauu.net>
> ---

Thanks for this fix !

Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

Re: Bug in netem reordering

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 05 Jan 2012 06:34:16 +0100

> 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 <hagen@jauu.net>
>> 
>> netem: catch NULL pointer by updating the real qdisc statistic
>> 
>> Reported-by: Vijay Subramanian <subramanian.vijay@gmail.com>
>> Signed-off-by: Hagen Paul Pfeifer <hagen@jauu.net>
>> ---
> 
> Thanks for this fix !
> 
> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied, thanks.