: fix CONFIG_NET_CLS_ACT skb leaks in HFSC/CBQ

# This is a BitKeeper generated diff -Nru style patch. #
# ChangeSet
# 2005/01/19 05:15:16+01:00 [email protected]
# [PKT_SCHED]: fix CONFIG_NET_CLS_ACT skb leaks in HFSC/CBQ
#

#   Both HFSC and CBQ leak unclassified skbs with CONFIG_NET_CLS_ACT.
#   Move freeing to enqueue where it belongs. Same change for in HTB/prio,
#   they just don't leak because they don't have unclassified packets.
#   
#   Signed-off-by: Patrick McHardy <[email protected]>
# 
# net/sched/sch_prio.c
#   2005/01/19 05:15:07+01:00 [email protected] +30 -44
#   [PKT_SCHED]: fix CONFIG_NET_CLS_ACT skb leaks in HFSC/CBQ
#   
#   Both HFSC and CBQ leak unclassified skbs with CONFIG_NET_CLS_ACT.
#   Move freeing to enqueue where it belongs. Same change for in HTB/prio,
#   they just don't leak because they don't have unclassified packets.
#   
#   Signed-off-by: Patrick McHardy <[email protected]>
# 
# net/sched/sch_htb.c
#   2005/01/19 05:15:07+01:00 [email protected] +16 -44
#   [PKT_SCHED]: fix CONFIG_NET_CLS_ACT skb leaks in HFSC/CBQ
#   
#   Both HFSC and CBQ leak unclassified skbs with CONFIG_NET_CLS_ACT.
#   Move freeing to enqueue where it belongs. Same change for in HTB/prio,
#   they just don't leak because they don't have unclassified packets.
#   
#   Signed-off-by: Patrick McHardy <[email protected]>
# 
# net/sched/sch_hfsc.c
#   2005/01/19 05:15:07+01:00 [email protected] +11 -34
#   [PKT_SCHED]: fix CONFIG_NET_CLS_ACT skb leaks in HFSC/CBQ
#   
#   Both HFSC and CBQ leak unclassified skbs with CONFIG_NET_CLS_ACT.
#   Move freeing to enqueue where it belongs. Same change for in HTB/prio,
#   they just don't leak because they don't have unclassified packets.
#   
#   Signed-off-by: Patrick McHardy <[email protected]>
# 
# net/sched/sch_cbq.c
#   2005/01/19 05:15:07+01:00 [email protected] +26 -53
#   [PKT_SCHED]: fix CONFIG_NET_CLS_ACT skb leaks in HFSC/CBQ
#   
#   Both HFSC and CBQ leak unclassified skbs with CONFIG_NET_CLS_ACT.
#   Move freeing to enqueue where it belongs. Same change for in HTB/prio,
#   they just don't leak because they classify all packets.
#   
#   Signed-off-by: Patrick McHardy <[email protected]>
# 
diff -Nru a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
--- a/net/sched/sch_cbq.c	2005-01-19 05:30:01 +01:00
+++ b/net/sched/sch_cbq.c	2005-01-19 05:30:01 +01:00
@@ -241,7 +241,7 @@
  */

static struct cbq_class *
-cbq_classify(struct sk_buff *skb, struct Qdisc *sch, int *qres) +cbq_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)
{
struct cbq_sched_data *q = qdisc_priv(sch); struct cbq_class *head = &q->link;
@@ -255,13 +255,11 @@
*/
if (TC_H_MAJ(prio^sch->handle) == 0 &&
(cl = cbq_class_lookup(q, prio)) != NULL)
- return cl;
+ return cl;

+ *qerr = NET_XMIT_DROP;
for (;;) {
int result = 0;
-#ifdef CONFIG_NET_CLS_ACT
- int terminal = 0;
-#endif
defmap = head->defaults;

/*
@@ -282,27 +280,13 @@

#ifdef CONFIG_NET_CLS_ACT
switch (result) {
- case TC_ACT_SHOT: /* Stop and kfree */
- *qres = NET_XMIT_DROP;
- terminal = 1;
- break;
case TC_ACT_QUEUED:
case TC_ACT_STOLEN:
- terminal = 1;
- break;
- case TC_ACT_RECLASSIFY: /* Things look good */
- case TC_ACT_OK:
- case TC_ACT_UNSPEC:
- default:
- break;
- }
-
- if (terminal) {
- kfree_skb(skb);
+ *qerr = NET_XMIT_SUCCESS;
+ case TC_ACT_SHOT:
return NULL;
}
-#else
-#ifdef CONFIG_NET_CLS_POLICE
+#elif defined(CONFIG_NET_CLS_POLICE)
switch (result) {
case TC_POLICE_RECLASSIFY:
return cbq_reclassify(skb, cl);
@@ -312,7 +296,6 @@
break;
}
#endif
-#endif
if (cl->level == 0)
return cl;

@@ -423,45 +406,35 @@
{
struct cbq_sched_data *q = qdisc_priv(sch); int len = skb->len;
- int ret = NET_XMIT_SUCCESS;
- struct cbq_class *cl = cbq_classify(skb, sch,&ret);
+ int ret;
+ struct cbq_class *cl = cbq_classify(skb, sch, &ret);

#ifdef CONFIG_NET_CLS_POLICE
q->rx_class = cl;
#endif
- if (cl) {
-#ifdef CONFIG_NET_CLS_POLICE
- cl->q->__parent = sch;
-#endif
- if ((ret = cl->q->enqueue(skb, cl->q)) == NET_XMIT_SUCCESS) {
- sch->q.qlen++;
- sch->bstats.packets++;
- sch->bstats.bytes+=len;
- cbq_mark_toplevel(q, cl);
- if (!cl->next_alive)
- cbq_activate_class(cl);
- return ret;
- }
- }
-
-#ifndef CONFIG_NET_CLS_ACT
- sch->qstats.drops++;
- if (cl == NULL)
+ if (cl == NULL) {
+ if (ret == NET_XMIT_DROP)
+ sch->qstats.drops++;
kfree_skb(skb);
- else {
- cbq_mark_toplevel(q, cl);
- cl->qstats.drops++;
- }
-#else
- if ( NET_XMIT_DROP == ret) {
- sch->qstats.drops++;
+ return ret;
}

- if (cl != NULL) {
+#ifdef CONFIG_NET_CLS_POLICE
+ cl->q->__parent = sch;
+#endif
+ if ((ret = cl->q->enqueue(skb, cl->q)) == NET_XMIT_SUCCESS) {
+ sch->q.qlen++;
+ sch->bstats.packets++;
+ sch->bstats.bytes+=len;
cbq_mark_toplevel(q, cl);
- cl->qstats.drops++;
+ if (!cl->next_alive)
+ cbq_activate_class(cl);
+ return ret;
}
-#endif
+
+ sch->qstats.drops++;
+ cbq_mark_toplevel(q, cl);
+ cl->qstats.drops++;
return ret;
}

diff -Nru a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
@@ -1214,7 +1214,7 @@
 }
 
 static struct hfsc_class *
-hfsc_classify(struct sk_buff *skb, struct Qdisc *sch, int *qres)
+hfsc_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)
 {
 	struct hfsc_sched *q = qdisc_priv(sch);
 	struct hfsc_class *cl;

@@ -1227,36 +1227,21 @@
 		if (cl->level == 0)
 			return cl;
 
+	*qerr = NET_XMIT_DROP;
 	tcf = q->root.filter_list;
 	while (tcf && (result = tc_classify(skb, tcf, &res)) >= 0) {
 #ifdef CONFIG_NET_CLS_ACT
-		int terminal = 0;
 		switch (result) {
-		case TC_ACT_SHOT: 
-			*qres = NET_XMIT_DROP;
-			terminal = 1;
-			break;
 		case TC_ACT_QUEUED:
 		case TC_ACT_STOLEN: 
-			terminal = 1;
-			break;
-		case TC_ACT_RECLASSIFY: 
-		case TC_ACT_OK:
-		case TC_ACT_UNSPEC:
-		default:
-		break;
-		}
-
-		if (terminal) {
-			kfree_skb(skb);
+			*qerr = NET_XMIT_SUCCESS;
+		case TC_ACT_SHOT: 
 			return NULL;
 		}
-#else
-#ifdef CONFIG_NET_CLS_POLICE
+#elif defined(CONFIG_NET_CLS_POLICE)
 		if (result == TC_POLICE_SHOT)
 			return NULL;
 #endif
-#endif
 		if ((cl = (struct hfsc_class *)res.class) == NULL) {
 			if ((cl = hfsc_find_class(res.classid, sch)) == NULL)
 				break; /* filter selected invalid classid */

@@ -1652,27 +1637,19 @@
 static int
 hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 {
-	int ret = NET_XMIT_SUCCESS;
-	struct hfsc_class *cl = hfsc_classify(skb, sch, &ret);
-	unsigned int len = skb->len;
+	struct hfsc_class *cl;
+	unsigned int len;
 	int err;
 
-
-#ifdef CONFIG_NET_CLS_ACT
+	cl = hfsc_classify(skb, sch, &err);
 	if (cl == NULL) {
-		if (NET_XMIT_DROP == ret) {
+		if (err == NET_XMIT_DROP)
 			sch->qstats.drops++;
-		}
-		return ret;
-	}
-#else
-	if (cl == NULL) {
 		kfree_skb(skb);
-		sch->qstats.drops++;
-		return NET_XMIT_DROP;
+		return err;
 	}
-#endif
 
+	len = skb->len;
 	err = cl->qdisc->enqueue(skb, cl->qdisc);
 	if (unlikely(err != NET_XMIT_SUCCESS)) {
 		cl->qstats.drops++;

I dont have time, but please double check that the following still applies:

1) A qdisc receiving a STOLEN/QUEUED/SHOT signal from the classification result MUST free the packet and immediately stop processing that packet. The infrastructure code will clone packets if they want to steal or queue it.
2) Return code of qdisc from enqueue function need to be dealt with care. For example if the packet is localy generated and it is a TCP packet you could confuse the stack by telling it the packet was dropped because it will retransmit (and some things will happen with the window adjustments).
3) packets that are dropped because of a full Q should continue to return a XMIT_DROP (you want TCP for example to know about this)

cheers,
jamal

It does now. Before there were things like "unsigned int len = skb->len" after the call to tc_classify.

TC_ACT_SHOT => NET_XMIT_DROP
TC_ACT_STOLEN | TC_ACT_QUEUED => NET_XMIT_SUCCESS

They do.

Ok, Patrick - then i have no issues; i will look closely when it shows up in bk.