BUG: HTB?

BUG: HTB?

[Asim Shankar]

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

Re: BUG: HTB?

[Thomas Graf]

Hi,

* Asim Shankar <7bca1cb50504211221655fd54c@mail.gmail.com> 2005-04-21 14:21
> 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.

Yes, that's a bug.

> 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? 

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

[Asim Shankar]

[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]

> > 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.
> 
> > 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?
> 
Patched pasted and attached:

--- linux-2.6.11.7/net/sched/sch_htb.c	2005-04-07 13:57:45.000000000 -0500
+++ linux-2.6.11.7-new/net/sched/sch_htb.c	2005-04-21 14:17:36.272065816 -0500
@@ -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) {

[-- Attachment #2: patch-htb-2.6.11.7 --]
[-- Type: text/plain, Size: 436 bytes --]

--- linux-2.6.11.7/net/sched/sch_htb.c	2005-04-07 13:57:45.000000000 -0500
+++ linux-2.6.11.7-new/net/sched/sch_htb.c	2005-04-21 14:17:36.272065816 -0500
@@ -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) {

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

[Thomas Graf]

[CCed Martin Devera]

* Asim Shankar <7bca1cb505042113417a5d9f59@mail.gmail.com> 2005-04-21 15:41
> 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 <tgraf@suug.ch>

> --- linux-2.6.11.7/net/sched/sch_htb.c	2005-04-07 13:57:45.000000000 -0500
> +++ linux-2.6.11.7-new/net/sched/sch_htb.c	2005-04-21 14:17:36.272065816 -0500
> @@ -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?

> +	    sch->qstats.drops++;
> +	    return NET_XMIT_DROP;
>  	}
>  #ifdef CONFIG_NET_CLS_ACT
>      } else if (!cl) {

Thanks.

[PATCH 2.6.11.7] sch_htb: Drop packet when direct queue is full

[Asim Shankar]

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 <asimshankar@gmail.com>

--- linux-2.6.11.7/net/sched/sch_htb.c.orig 2005-04-21 17:40:05.305709014 -0500
+++ linux-2.6.11.7/net/sched/sch_htb.c 2005-04-21 17:35:27.872624173 -0500
@@ -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) {

[PATCH 2.6.11.7] sch_htb: Drop packet when direct queue is full

[Asim Shankar]

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 patch: 
Erroneously returned NET_XMIT_SUCCESS even though packet was not enqueued)

Signed-off-by: Asim Shankar <asimshankar@gmail.com>

--- linux-2.6.11.7/net/sched/sch_htb.c.orig	2005-04-21 17:40:05.305709014 -0500
+++ linux-2.6.11.7/net/sched/sch_htb.c	2005-04-21 17:35:27.872624173 -0500
@@ -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) {

Re: [PATCH 2.6.11.7] sch_htb: Drop packet when direct queue is full

[David S. Miller]

On Mon, 25 Apr 2005 14:15:05 -0500 (CDT)
Asim Shankar <asimshankar@gmail.com> wrote:

> 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 patch: 
> Erroneously returned NET_XMIT_SUCCESS even though packet was not enqueued)
> 
> Signed-off-by: Asim Shankar <asimshankar@gmail.com>

Your patch is mangled by your email client.  The non-changing lines
in the patch have two leading spaces, which is wrong.  So the patch
will not apply.

Re: [PATCH 2.6.11.7] sch_htb: Drop packet when direct queue is full

[Asim Shankar]

> Your patch is mangled by your email client.  The non-changing lines
> in the patch have two leading spaces, which is wrong.  So the patch
> will not apply.
> 

Resending as a gzipped attachment. Hope this works.

Thanks,

-- Asim