Skip to contentSkip to conversation

BUG: HTB?

PATCH In review Asim Shankar 2 participants 5 messages Apr 21, 2005

Subsystems (3)
Networking Network drivers Traffic control (tc)
lore
Share & tools
lore

Asim Shankar

author
#0 lore raw

Hi,

I think there is a bug in htb_enqueue() (net/sched/sch_htb.c)

Specifically, in the lines:

if (cl == HTB_DIRECT) {
        /* enqueue to helper queue */
        if (q->direct_queue.qlen < q->direct_qlen) {
                __skb_queue_tail(&q->direct_queue, skb);
                q->direct_pkts++;
        }
}

If a packet is classified as HTB_DIRECT but the direct_queue is already full, then the packet doesn't get enqueued but sch->q.qlen++ will happen a few lines later. Overflowing of the direct_queue probably rarely happens in practice, but I was playing around and noticed it happen in some corner cases of my testing.

Should the packet be dropped instead? Like:

if (cl == HTB_DIRECT) {
        /* enqueue to helper queue */
        if (q->direct_queue.qlen < q->direct_qlen) {
                __skb_queue_tail(&q->direct_queue, skb);
                q->direct_pkts++;
        } else {
               sch->qstats.drops++;
                kfree_skb(skb);
                return NET_XMIT_DROP;
       }
}

Thanks,

-- Asim

#1 lore raw

Hi,

* Asim Shankar <[email protected]> 2005-04-21 14:21

Quoted 9 lines

Specifically, in the lines:

if (cl == HTB_DIRECT) {
        /* enqueue to helper queue */
        if (q->direct_queue.qlen < q->direct_qlen) {
                __skb_queue_tail(&q->direct_queue, skb);
                q->direct_pkts++;
        }
}
Quoted 5 lines

If a packet is classified as HTB_DIRECT but the direct_queue is already full, then the packet doesn't get enqueued but sch->q.qlen++ will happen a few lines later. Overflowing of the direct_queue probably rarely happens in practice, but I was playing around and noticed it happen in some corner cases of my testing.

Yes, that's a bug.

Quoted 13 lines

Should the packet be dropped instead? Like:

if (cl == HTB_DIRECT) {
        /* enqueue to helper queue */
        if (q->direct_queue.qlen < q->direct_qlen) {
                __skb_queue_tail(&q->direct_queue, skb);
                q->direct_pkts++;
        } else {
               sch->qstats.drops++;
                kfree_skb(skb);
                return NET_XMIT_DROP;
       }
}

Can you send a patch?

author
#2 lore raw

PATCH 1/1 - sch_htb: Drop packet when direct queue overflows

Quoted 22 lines
Quoted 5 lines

If a packet is classified as HTB_DIRECT but the direct_queue is already full, then the packet doesn't get enqueued but sch->q.qlen++ will happen a few lines later. Overflowing of the direct_queue probably rarely happens in practice, but I was playing around and noticed it happen in some corner cases of my testing.

Yes, that's a bug.

Quoted 13 lines

Should the packet be dropped instead? Like:

if (cl == HTB_DIRECT) {
        /* enqueue to helper queue */
        if (q->direct_queue.qlen < q->direct_qlen) {
                __skb_queue_tail(&q->direct_queue, skb);
                q->direct_pkts++;
        } else {
               sch->qstats.drops++;
                kfree_skb(skb);
                return NET_XMIT_DROP;
       }
}
Quoted 2 lines

Can you send a patch?

Patched pasted and attached:

net/sched/sch_htb.c +4 -0
@@ -717,6 +717,10 @@
 	if (q->direct_queue.qlen < q->direct_qlen) {
 	    __skb_queue_tail(&q->direct_queue, skb);
 	    q->direct_pkts++;
+	} else {
+	    kfree_skb (skb);
+	    sch->qstats.drops++;
+	    return NET_XMIT_DROP;
 	}
 #ifdef CONFIG_NET_CLS_ACT
     } else if (!cl) {
net/sched/sch_htb.c +4 -0
@@ -717,6 +717,10 @@
 	if (q->direct_queue.qlen < q->direct_qlen) {
 	    __skb_queue_tail(&q->direct_queue, skb);
 	    q->direct_pkts++;
+	} else {
+	    kfree_skb (skb);
+	    sch->qstats.drops++;
+	    return NET_XMIT_DROP;
 	}
 #ifdef CONFIG_NET_CLS_ACT
     } else if (!cl) {
#3 lore raw

Re: [PATCH] - sch_htb: Drop packet when direct queue overflows

[CCed Martin Devera]

* Asim Shankar <[email protected]> 2005-04-21 15:41

Quoted 1 line

Patched pasted and attached:

For patch formatting details see http://linux.yyz.us/patch-format.html
A short description of the patch would be helpful.

Besides of that...

Signed-off-by Thomas Graf <[email protected]>

net/sched/sch_htb.c line 721

Quoted 8 lines
net/sched/sch_htb.c +2 -0
@@ -717,6 +717,10 @@
 	if (q->direct_queue.qlen < q->direct_qlen) {
 	    __skb_queue_tail(&q->direct_queue, skb);
 	    q->direct_pkts++;
+	} else {
+	    kfree_skb (skb);

Could you write this without the whitespace?

net/sched/sch_htb.c line 726

Quoted 5 lines
net/sched/sch_htb.c +2 -0
+	    sch->qstats.drops++;
+	    return NET_XMIT_DROP;
 	}
 #ifdef CONFIG_NET_CLS_ACT
     } else if (!cl) {

Thanks.

author
#4 lore raw

PATCH 1/1 sch_htb: Drop packet when direct queue is full

htb_enqueue(): Free skb and return NET_XMIT_DROP if a packet is destined for the direct_queue but the direct_queue is full. (Before
this: erroneously returned NET_XMIT_SUCCESS even though the packet was not enqueued)

Signed-off-by Asim Shankar <[email protected]>
net/sched/sch_htb.c 2005-04-21 17:35:27.872624173 -0500 renamed from net/sched/sch_htb.c.orig 2005-04-21 17:40:05.305709014 -0500 +0 -0
@@ -717,6 +717,10 @@ static int htb_enqueue(struct sk_buff *s

if (q->direct_queue.qlen < q->direct_qlen) {
    __skb_queue_tail(&q->direct_queue, skb);
    q->direct_pkts++;
+ } else {
+     kfree_skb(skb);
+     sch->qstats.drops++;
+     return NET_XMIT_DROP;
}
#ifdef CONFIG_NET_CLS_ACT
     } else if (!cl) {