[PATCH 1/9] net classifier: dont allow filters on semi-classful qdisc

[PATCH 1/9] net classifier: dont allow filters on semi-classful qdisc

[Stephen Hemminger]

[-- Attachment #1: cls-bind-tcf.patch --]
[-- Type: text/plain, Size: 3796 bytes --]

There are several qdisc which only support a single class (sfq, mq, tbf)
and the kernel would dereference a null pointer (bind_tcf), if a user
attempted to apply a filter one of these classes.

This patch changes the tcf_bind_filter to return an error in
these cases.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


---
This needs to go in net-2.6 and stable.

 include/net/pkt_cls.h   |   12 +++++++++---
 net/sched/cls_basic.c   |    4 +++-
 net/sched/cls_fw.c      |    6 ++++--
 net/sched/cls_route.c   |    4 +++-
 net/sched/cls_tcindex.c |    4 +++-
 net/sched/cls_u32.c     |    4 +++-
 6 files changed, 25 insertions(+), 9 deletions(-)

--- a/include/net/pkt_cls.h	2010-08-06 11:51:18.903581556 -0700
+++ b/include/net/pkt_cls.h	2010-08-06 12:20:02.072241508 -0700
@@ -40,15 +40,21 @@ cls_set_class(struct tcf_proto *tp, unsi
 	return old_cl;
 }
 
-static inline void
+static inline int
 tcf_bind_filter(struct tcf_proto *tp, struct tcf_result *r, unsigned long base)
 {
+	const struct Qdisc_class_ops *cops = tp->q->ops->cl_ops;
 	unsigned long cl;
 
-	cl = tp->q->ops->cl_ops->bind_tcf(tp->q, base, r->classid);
+	if (!cops->bind_tcf)
+		return -EINVAL;
+
+	cl = cops->bind_tcf(tp->q, base, r->classid);
 	cl = cls_set_class(tp, &r->class, cl);
 	if (cl)
-		tp->q->ops->cl_ops->unbind_tcf(tp->q, cl);
+		cops->unbind_tcf(tp->q, cl);
+
+	return 0;
 }
 
 static inline void
--- a/net/sched/cls_basic.c	2010-08-06 11:51:18.923582342 -0700
+++ b/net/sched/cls_basic.c	2010-08-06 11:55:13.292553190 -0700
@@ -153,7 +153,9 @@ static inline int basic_set_parms(struct
 
 	if (tb[TCA_BASIC_CLASSID]) {
 		f->res.classid = nla_get_u32(tb[TCA_BASIC_CLASSID]);
-		tcf_bind_filter(tp, &f->res, base);
+		err = tcf_bind_filter(tp, &f->res, base);
+		if (err)
+			goto errout;
 	}
 
 	tcf_exts_change(tp, &f->exts, &e);
--- a/net/sched/cls_fw.c	2010-08-06 11:51:18.943583126 -0700
+++ b/net/sched/cls_fw.c	2010-08-06 11:55:39.085476144 -0700
@@ -206,10 +206,11 @@ fw_change_attrs(struct tcf_proto *tp, st
 	if (err < 0)
 		return err;
 
-	err = -EINVAL;
 	if (tb[TCA_FW_CLASSID]) {
 		f->res.classid = nla_get_u32(tb[TCA_FW_CLASSID]);
-		tcf_bind_filter(tp, &f->res, base);
+		err = tcf_bind_filter(tp, &f->res, base);
+		if (err)
+			goto errout;
 	}
 
 #ifdef CONFIG_NET_CLS_IND
@@ -220,6 +221,7 @@ fw_change_attrs(struct tcf_proto *tp, st
 	}
 #endif /* CONFIG_NET_CLS_IND */
 
+	err = -EINVAL;
 	if (tb[TCA_FW_MASK]) {
 		mask = nla_get_u32(tb[TCA_FW_MASK]);
 		if (mask != head->mask)
--- a/net/sched/cls_route.c	2010-08-06 11:51:18.959583757 -0700
+++ b/net/sched/cls_route.c	2010-08-06 11:55:50.077870498 -0700
@@ -412,7 +412,9 @@ static int route4_set_parms(struct tcf_p
 
 	if (tb[TCA_ROUTE4_CLASSID]) {
 		f->res.classid = nla_get_u32(tb[TCA_ROUTE4_CLASSID]);
-		tcf_bind_filter(tp, &f->res, base);
+		err = tcf_bind_filter(tp, &f->res, base);
+		if (err)
+			goto errout;
 	}
 
 	tcf_exts_change(tp, &f->exts, &e);
--- a/net/sched/cls_tcindex.c	2010-08-06 11:51:18.999585326 -0700
+++ b/net/sched/cls_tcindex.c	2010-08-06 11:56:01.486283847 -0700
@@ -295,7 +295,9 @@ tcindex_set_parms(struct tcf_proto *tp, 
 
 	if (tb[TCA_TCINDEX_CLASSID]) {
 		cr.res.classid = nla_get_u32(tb[TCA_TCINDEX_CLASSID]);
-		tcf_bind_filter(tp, &cr.res, base);
+		err = tcf_bind_filter(tp, &cr.res, base);
+		if (err)
+			goto errout;
 	}
 
 	tcf_exts_change(tp, &cr.exts, &e);
--- a/net/sched/cls_u32.c	2010-08-06 11:51:19.019586112 -0700
+++ b/net/sched/cls_u32.c	2010-08-06 11:56:12.390678703 -0700
@@ -528,7 +528,9 @@ static int u32_set_parms(struct tcf_prot
 	}
 	if (tb[TCA_U32_CLASSID]) {
 		n->res.classid = nla_get_u32(tb[TCA_U32_CLASSID]);
-		tcf_bind_filter(tp, &n->res, base);
+		err = tcf_bind_filter(tp, &n->res, base);
+		if (err)
+			goto errout;
 	}
 
 #ifdef CONFIG_NET_CLS_IND

[PATCH 2/9] net classifier: deinline bind/unbind functions

[Stephen Hemminger]

[-- Attachment #1: cls-deinline.patch --]
[-- Type: text/plain, Size: 3262 bytes --]

The filter bind/unbind functions are not worth inlining.
Not performance critical in the least.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
 include/net/pkt_cls.h |   52 ++------------------------------------------------
 net/sched/cls_api.c   |   50 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 49 deletions(-)

--- a/include/net/pkt_cls.h	2010-08-06 12:21:18.000000000 -0700
+++ b/include/net/pkt_cls.h	2010-08-06 12:21:59.736709482 -0700
@@ -16,55 +16,9 @@ struct tcf_walker {
 
 extern int register_tcf_proto_ops(struct tcf_proto_ops *ops);
 extern int unregister_tcf_proto_ops(struct tcf_proto_ops *ops);
-
-static inline unsigned long
-__cls_set_class(unsigned long *clp, unsigned long cl)
-{
-	unsigned long old_cl;
- 
-	old_cl = *clp;
-	*clp = cl;
-	return old_cl;
-}
-
-static inline unsigned long
-cls_set_class(struct tcf_proto *tp, unsigned long *clp, 
-	unsigned long cl)
-{
-	unsigned long old_cl;
-	
-	tcf_tree_lock(tp);
-	old_cl = __cls_set_class(clp, cl);
-	tcf_tree_unlock(tp);
- 
-	return old_cl;
-}
-
-static inline int
-tcf_bind_filter(struct tcf_proto *tp, struct tcf_result *r, unsigned long base)
-{
-	const struct Qdisc_class_ops *cops = tp->q->ops->cl_ops;
-	unsigned long cl;
-
-	if (!cops->bind_tcf)
-		return -EINVAL;
-
-	cl = cops->bind_tcf(tp->q, base, r->classid);
-	cl = cls_set_class(tp, &r->class, cl);
-	if (cl)
-		cops->unbind_tcf(tp->q, cl);
-
-	return 0;
-}
-
-static inline void
-tcf_unbind_filter(struct tcf_proto *tp, struct tcf_result *r)
-{
-	unsigned long cl;
-
-	if ((cl = __cls_set_class(&r->class, 0)) != 0)
-		tp->q->ops->cl_ops->unbind_tcf(tp->q, cl);
-}
+extern int tcf_bind_filter(struct tcf_proto *tp, struct tcf_result *r,
+			   unsigned long base);
+extern void tcf_unbind_filter(struct tcf_proto *tp, struct tcf_result *r);
 
 struct tcf_exts {
 #ifdef CONFIG_NET_CLS_ACT
--- a/net/sched/cls_api.c	2010-08-06 12:20:02.000000000 -0700
+++ b/net/sched/cls_api.c	2010-08-06 12:21:21.623261373 -0700
@@ -99,6 +99,56 @@ out:
 }
 EXPORT_SYMBOL(unregister_tcf_proto_ops);
 
+static unsigned long __cls_set_class(unsigned long *clp, unsigned long cl)
+{
+	unsigned long old_cl;
+
+	old_cl = *clp;
+	*clp = cl;
+	return old_cl;
+}
+
+static unsigned long cls_set_class(struct tcf_proto *tp, unsigned long *clp,
+				   unsigned long cl)
+{
+	unsigned long old_cl;
+
+	tcf_tree_lock(tp);
+	old_cl = __cls_set_class(clp, cl);
+	tcf_tree_unlock(tp);
+
+	return old_cl;
+}
+
+int tcf_bind_filter(struct tcf_proto *tp, struct tcf_result *r,
+		    unsigned long base)
+{
+	const struct Qdisc_class_ops *cops = tp->q->ops->cl_ops;
+	unsigned long cl;
+
+	if (!cops->bind_tcf)
+		return -ENOENT;
+
+	cl = cops->bind_tcf(tp->q, base, r->classid);
+	cl = cls_set_class(tp, &r->class, cl);
+	if (cl)
+		cops->unbind_tcf(tp->q, cl);
+
+	return 0;
+}
+EXPORT_SYMBOL(tcf_bind_filter);
+
+void tcf_unbind_filter(struct tcf_proto *tp, struct tcf_result *r)
+{
+	unsigned long cl;
+
+	cl = __cls_set_class(&r->class, 0);
+	if (cl)
+		tp->q->ops->cl_ops->unbind_tcf(tp->q, cl);
+}
+EXPORT_SYMBOL(tcf_unbind_filter);
+
+
 static int tfilter_notify(struct net *net, struct sk_buff *oskb,
 			  struct nlmsghdr *n, struct tcf_proto *tp,
 			  unsigned long fh, int event);

[PATCH 3/9] u32 classifier: fix sparse warnings

[Stephen Hemminger]

[-- Attachment #1: cls-u32-sparse.patch --]
[-- Type: text/plain, Size: 1454 bytes --]

The variable _data is used in asm-generic to define sections
which causes sparse warnings, so just rename the variable.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/net/sched/cls_u32.c	2010-08-06 11:41:06.974527973 -0700
+++ b/net/sched/cls_u32.c	2010-08-06 12:20:03.508295989 -0700
@@ -135,12 +135,12 @@ next_knode:
 
 		for (i = n->sel.nkeys; i>0; i--, key++) {
 			int toff = off + key->off + (off2 & key->offmask);
-			__be32 *data, _data;
+			__be32 *data, hdata;
 
 			if (skb_headroom(skb) + toff < 0)
 				goto out;
 
-			data = skb_header_pointer(skb, toff, 4, &_data);
+			data = skb_header_pointer(skb, toff, 4, &hdata);
 			if (!data)
 				goto out;
 			if ((*data ^ key->val) & key->mask) {
@@ -188,10 +188,10 @@ check_terminal:
 		ht = n->ht_down;
 		sel = 0;
 		if (ht->divisor) {
-			__be32 *data, _data;
+			__be32 *data, hdata;
 
 			data = skb_header_pointer(skb, off + n->sel.hoff, 4,
-						  &_data);
+						  &hdata);
 			if (!data)
 				goto out;
 			sel = ht->divisor & u32_hash_fold(*data, &n->sel,
@@ -203,11 +203,11 @@ check_terminal:
 		if (n->sel.flags&(TC_U32_OFFSET|TC_U32_VAROFFSET)) {
 			off2 = n->sel.off + 3;
 			if (n->sel.flags & TC_U32_VAROFFSET) {
-				__be16 *data, _data;
+				__be16 *data, hdata;
 
 				data = skb_header_pointer(skb,
 							  off + n->sel.offoff,
-							  2, &_data);
+							  2, &hdata);
 				if (!data)
 					goto out;
 				off2 += ntohs(n->sel.offmask & *data) >>

[PATCH 4/9] netem: add locking around changes

[Stephen Hemminger]

[-- Attachment #1: netem-cleanup.patch --]
[-- Type: text/plain, Size: 1480 bytes --]

Some safety improvements to netem to make it safer to change paramters
while queue is running.

Use sch_tree_lock() to block all packets during change.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/net/sched/sch_netem.c	2010-08-02 16:22:42.389335747 -0700
+++ b/net/sched/sch_netem.c	2010-08-03 08:25:15.611820853 -0700
@@ -318,7 +318,6 @@ static int get_dist_table(struct Qdisc *
 	struct netem_sched_data *q = qdisc_priv(sch);
 	unsigned long n = nla_len(attr)/sizeof(__s16);
 	const __s16 *data = nla_data(attr);
-	spinlock_t *root_lock;
 	struct disttable *d;
 	int i;
 
@@ -333,12 +332,9 @@ static int get_dist_table(struct Qdisc *
 	for (i = 0; i < n; i++)
 		d->table[i] = data[i];
 
-	root_lock = qdisc_root_sleeping_lock(sch);
-
-	spin_lock_bh(root_lock);
 	kfree(q->delay_dist);
 	q->delay_dist = d;
-	spin_unlock_bh(root_lock);
+
 	return 0;
 }
 
@@ -412,6 +408,7 @@ static int netem_change(struct Qdisc *sc
 		return ret;
 	}
 
+	sch_tree_lock(sch);
 	q->latency = qopt->latency;
 	q->jitter = qopt->jitter;
 	q->limit = qopt->limit;
@@ -432,7 +429,7 @@ static int netem_change(struct Qdisc *sc
 	if (tb[TCA_NETEM_DELAY_DIST]) {
 		ret = get_dist_table(sch, tb[TCA_NETEM_DELAY_DIST]);
 		if (ret)
-			return ret;
+			goto out;
 	}
 
 	if (tb[TCA_NETEM_REORDER])
@@ -440,6 +437,8 @@ static int netem_change(struct Qdisc *sc
 
 	if (tb[TCA_NETEM_CORRUPT])
 		get_corrupt(sch, tb[TCA_NETEM_CORRUPT]);
+ out:
+	sch_tree_unlock(sch);
 
 	return 0;
 }

[PATCH 5/9] netem: cleanup dump code

[Stephen Hemminger]

[-- Attachment #1: netem-dump.patch --]
[-- Type: text/plain, Size: 1021 bytes --]

Use nla_put_nested to update netlink attribute value.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/net/sched/sch_netem.c	2010-08-03 08:28:28.031830483 -0700
+++ b/net/sched/sch_netem.c	2010-08-03 08:29:39.546593787 -0700
@@ -565,8 +565,7 @@ static void netem_destroy(struct Qdisc *
 static int netem_dump(struct Qdisc *sch, struct sk_buff *skb)
 {
 	const struct netem_sched_data *q = qdisc_priv(sch);
-	unsigned char *b = skb_tail_pointer(skb);
-	struct nlattr *nla = (struct nlattr *) b;
+	struct nlattr *nla = (struct nlattr *) skb_tail_pointer(skb);
 	struct tc_netem_qopt qopt;
 	struct tc_netem_corr cor;
 	struct tc_netem_reorder reorder;
@@ -593,12 +592,10 @@ static int netem_dump(struct Qdisc *sch,
 	corrupt.correlation = q->corrupt_cor.rho;
 	NLA_PUT(skb, TCA_NETEM_CORRUPT, sizeof(corrupt), &corrupt);
 
-	nla->nla_len = skb_tail_pointer(skb) - b;
-
-	return skb->len;
+	return nla_nest_end(skb, nla);
 
 nla_put_failure:
-	nlmsg_trim(skb, b);
+	nlmsg_trim(skb, nla);
 	return -1;
 }
 

[PATCH 6/9] netem: distribution table changes

[Stephen Hemminger]

[-- Attachment #1: netem-dist-vmalloc.patch --]
[-- Type: text/plain, Size: 1408 bytes --]

Fix some issues with distribution table in netem:
  * Make maximum size visible to user space
  * Use vmalloc to avoid allocating large physical contiguous memory.
    (maybe this should use flex array?)

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/include/linux/pkt_sched.h	2010-08-02 16:22:42.197329627 -0700
+++ b/include/linux/pkt_sched.h	2010-08-03 08:29:43.926514463 -0700
@@ -466,6 +466,7 @@ struct tc_netem_corrupt {
 };
 
 #define NETEM_DIST_SCALE	8192
+#define NETEM_DIST_MAX		16384
 
 /* DRR */
 
--- a/net/sched/sch_netem.c	2010-08-03 08:29:39.546593787 -0700
+++ b/net/sched/sch_netem.c	2010-08-03 08:29:43.926514463 -0700
@@ -321,10 +321,10 @@ static int get_dist_table(struct Qdisc *
 	struct disttable *d;
 	int i;
 
-	if (n > 65536)
+	if (n > NETEM_DIST_MAX)
 		return -EINVAL;
 
-	d = kmalloc(sizeof(*d) + n*sizeof(d->table[0]), GFP_KERNEL);
+	d = vmalloc(sizeof(*d) + n*sizeof(d->table[0]));
 	if (!d)
 		return -ENOMEM;
 
@@ -332,7 +332,7 @@ static int get_dist_table(struct Qdisc *
 	for (i = 0; i < n; i++)
 		d->table[i] = data[i];
 
-	kfree(q->delay_dist);
+	vfree(q->delay_dist);
 	q->delay_dist = d;
 
 	return 0;
@@ -559,7 +559,7 @@ static void netem_destroy(struct Qdisc *
 
 	qdisc_watchdog_cancel(&q->watchdog);
 	qdisc_destroy(q->qdisc);
-	kfree(q->delay_dist);
+	vfree(q->delay_dist);
 }
 
 static int netem_dump(struct Qdisc *sch, struct sk_buff *skb)

[PATCH 7/9] netem: dump distribution table

[Stephen Hemminger]

[-- Attachment #1: netem-dump-dist.patch --]
[-- Type: text/plain, Size: 852 bytes --]

The existing netem API is not symmetric; the current distribution
table is not added to dump output.  Since distribution table is 
could be large, only add it if there space available (for compatiablity
with older applications).

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/net/sched/sch_netem.c	2010-08-03 08:29:43.926514463 -0700
+++ b/net/sched/sch_netem.c	2010-08-03 08:30:49.297380886 -0700
@@ -592,6 +592,13 @@ static int netem_dump(struct Qdisc *sch,
 	corrupt.correlation = q->corrupt_cor.rho;
 	NLA_PUT(skb, TCA_NETEM_CORRUPT, sizeof(corrupt), &corrupt);
 
+	/* dump table if exists and there is space available */
+	if (q->delay_dist) {
+		const struct disttable *d = q->delay_dist;
+		nla_put(skb, TCA_NETEM_DELAY_DIST,
+			d->size * sizeof(__s16), d->table);
+	}
+
 	return nla_nest_end(skb, nla);
 
 nla_put_failure:

[PATCH 8/9] netem - revised correlated loss generator

[Stephen Hemminger]

[-- Attachment #1: netem-clg.patch --]
[-- Type: text/plain, Size: 10275 bytes --]

Subject: [PATCH 8/9] netem: correlated loss genartor

This is a patch originated with Stefano Salsano and Fabio Ludovici.
It provides several alternative loss models for use with netem.
This patch adds two state machine based loss models.

To simplify the original code:
   * elminated the debugging messages and statistics
   * reformatted for clarity
   * changed API to nested attribute relating to loss
   * changed the table to always loop across bits
   * only allocate parameters needed

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
 include/linux/pkt_sched.h |   25 ++++
 net/sched/sch_netem.c     |  262 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 284 insertions(+), 3 deletions(-)

--- a/net/sched/sch_netem.c	2010-08-02 15:03:03.814479429 -0700
+++ b/net/sched/sch_netem.c	2010-08-02 15:03:04.746482821 -0700
@@ -47,6 +47,20 @@
 	 layering other disciplines.  It does not need to do bandwidth
 	 control either since that can be handled by using token
 	 bucket or other rate control.
+
+     Correlated Loss Generator models
+
+	Added generation of correlated loss according to the
+	"Gilbert-Elliot" model, a 4-state markov model.
+
+	References:
+	[1] NetemCLG Home http://netgroup.uniroma2.it/NetemCLG
+	[2] S. Salsano, F. Ludovici, A. Ordine, "Definition of a general
+	and intuitive loss model for packet networks and its implementation
+	in the Netem module in the Linux kernel", available in [1]
+
+	Authors: Stefano Salsano <stefano.salsano at uniroma2.it
+		 Fabio Ludovici <fabio.ludovici at yahoo.it>
 */
 
 struct netem_sched_data {
@@ -73,6 +87,26 @@ struct netem_sched_data {
 		u32  size;
 		s16 table[0];
 	} *delay_dist;
+
+	enum  {
+		CLG_RANDOM,
+		CLG_4_STATES,
+		CLG_GILB_ELL,
+	} loss_model;
+
+	/* Correlated Loss Generation models */
+	struct clgstate {
+		/* state of the Markov chain */
+		u8 state;
+
+		/* 4-states and Gilbert-Elliot models */
+		u32 a1;	/* p13 for 4-states or p for GE */
+		u32 a2;	/* p31 for 4-states or r for GE */
+		u32 a3;	/* p32 for 4-states or h for GE */
+		u32 a4;	/* p14 for 4-states or 1-k for GE */
+		u32 a5; /* p23 used only in 4-states */
+	} clg;
+
 };
 
 /* Time stamp put into socket buffer control block */
@@ -119,6 +153,122 @@ static u32 get_crandom(struct crndstate 
 	return answer;
 }
 
+/* loss_4state - 4-state model loss generator
+ * Generates losses according to the 4-state Markov chain adopted in
+ * the GI (General and Intuitive) loss model.
+ */
+static bool loss_4state(struct netem_sched_data *q)
+{
+	struct clgstate *clg = &q->clg;
+	u32 rnd = net_random();
+
+	/*
+	 * Makes a comparision between rnd and the transition
+	 * probabilities outgoing from the current state, then decides the
+	 * next state and if the next packet has to be transmitted or lost.
+	 * The four states correspond to:
+	 *   1 => successfully transmitted packets within a gap period
+	 *   4 => isolated losses within a gap period
+	 *   3 => lost packets within a burst period
+	 *   2 => successfully transmitted packets within a burst period
+	 */
+	switch (clg->state) {
+	case 1:
+		if (rnd < clg->a4) {
+			clg->state = 4;
+			return true;
+		} else if (clg->a4 < rnd && rnd < clg->a1) {
+			clg->state = 3;
+			return true;
+		} else if (clg->a1 < rnd)
+			clg->state = 1;
+
+		break;
+	case 2:
+		if (rnd < clg->a5) {
+			clg->state = 3;
+			return true;
+		} else
+			clg->state = 2;
+
+		break;
+	case 3:
+		if (rnd < clg->a3)
+			clg->state = 2;
+		else if (clg->a3 < rnd && rnd < clg->a2 + clg->a3) {
+			clg->state = 1;
+			return true;
+		} else if (clg->a2 + clg->a3 < rnd) {
+			clg->state = 3;
+			return true;
+		}
+		break;
+	case 4:
+		clg->state = 1;
+		break;
+	}
+
+	return false;
+}
+
+/* loss_gilb_ell - Gilbert-Elliot model loss generator
+ * Generates losses according to the Gilbert-Elliot loss model or
+ * its special cases  (Gilbert or Simple Gilbert)
+ *
+ * Makes a comparision between random number and the transition
+ * probabilities outgoing from the current state, then decides the
+ * next state. A second random number is extracted and the comparision
+ * with the loss probability of the current state decides if the next
+ * packet will be transmitted or lost.
+ */
+static bool loss_gilb_ell(struct netem_sched_data *q)
+{
+	struct clgstate *clg = &q->clg;
+
+	switch (clg->state) {
+	case 1:
+		if (net_random() < clg->a1)
+			clg->state = 2;
+		if (net_random() < clg->a4)
+			return true;
+	case 2:
+		if (net_random() < clg->a2)
+			clg->state = 1;
+		if (clg->a3 > net_random())
+			return true;
+	}
+
+	return false;
+}
+
+static bool loss_event(struct netem_sched_data *q)
+{
+	switch (q->loss_model) {
+	case CLG_RANDOM:
+		/* Random packet drop 0 => none, ~0 => all */
+		return q->loss && q->loss >= get_crandom(&q->loss_cor);
+
+	case CLG_4_STATES:
+		/* 4state loss model algorithm (used also for GI model)
+		* Extracts a value from the markov 4 state loss generator,
+		* if it is 1 drops a packet and if needed writes the event in
+		* the kernel logs
+		*/
+		return loss_4state(q);
+
+	case CLG_GILB_ELL:
+		/* Gilbert-Elliot loss model algorithm
+		* Extracts a value from the Gilbert-Elliot loss generator,
+		* if it is 1 drops a packet and if needed writes the event in
+		* the kernel logs
+		*/
+		return loss_gilb_ell(q);
+	}
+
+	return false;	/* not reached */
+}
+
+
 /* tabledist - return a pseudo-randomly distributed value with mean mu and
  * std deviation sigma.  Uses table lookup to approximate the desired
  * distribution, and a uniformly-distributed pseudo-random source.
@@ -171,8 +321,8 @@ static int netem_enqueue(struct sk_buff 
 	if (q->duplicate && q->duplicate >= get_crandom(&q->dup_cor))
 		++count;
 
-	/* Random packet drop 0 => none, ~0 => all */
-	if (q->loss && q->loss >= get_crandom(&q->loss_cor))
+	/* Drop packet? */
+	if (loss_event(q))
 		--count;
 
 	if (count == 0) {
@@ -370,10 +520,62 @@ static void get_corrupt(struct Qdisc *sc
 	init_crandom(&q->corrupt_cor, r->correlation);
 }
 
+static int get_loss_clg(struct Qdisc *sch, const struct nlattr *attr)
+{
+	struct netem_sched_data *q = qdisc_priv(sch);
+	struct nlattr *la;
+	int rem;
+
+	nla_for_each_nested(la, attr, rem) {
+		switch(nla_type(la)) {
+		case NETEM_LOSS_GI: {
+			const struct tc_netem_gimodel *gi = nla_data(la);
+
+			if (nla_len(la) != sizeof(struct tc_netem_gimodel)) {
+				pr_info("netem: incorrect gi model size\n");
+				return -EINVAL;
+			}
+
+			q->loss_model = CLG_4_STATES;
+
+			q->clg.state = 1;
+			q->clg.a1 = gi->p13;
+			q->clg.a2 = gi->p31;
+			q->clg.a3 = gi->p32;
+			q->clg.a4 = gi->p14;
+			q->clg.a5 = gi->p23;
+			break;
+		}
+		case NETEM_LOSS_GE: {
+			const struct tc_netem_gemodel *ge = nla_data(la);
+
+			if (nla_len(la) != sizeof(struct tc_netem_gemodel)) {
+				pr_info("netem: incorrect gi model size\n");
+				return -EINVAL;
+			}
+
+			q->loss_model = CLG_GILB_ELL;
+			q->clg.state = 1;
+			q->clg.a1 = ge->p;
+			q->clg.a2 = ge->r;
+			q->clg.a3 = ge->h;
+			q->clg.a4 = ge->k1;
+			break;
+		}
+		default:
+			pr_info("netem: unknown loss element %d\n",
+				nla_type(la));
+			return -EINVAL;
+		}
+	}
+	return 0;
+}
+
 static const struct nla_policy netem_policy[TCA_NETEM_MAX + 1] = {
 	[TCA_NETEM_CORR]	= { .len = sizeof(struct tc_netem_corr) },
 	[TCA_NETEM_REORDER]	= { .len = sizeof(struct tc_netem_reorder) },
 	[TCA_NETEM_CORRUPT]	= { .len = sizeof(struct tc_netem_corrupt) },
+	[TCA_NETEM_LOSS]	= { .type = NLA_NESTED },
 };
 
 static int parse_attr(struct nlattr *tb[], int maxtype, struct nlattr *nla,
@@ -441,10 +643,14 @@ static int netem_change(struct Qdisc *sc
 
 	if (tb[TCA_NETEM_CORRUPT])
 		get_corrupt(sch, tb[TCA_NETEM_CORRUPT]);
+
+	q->loss_model = CLG_RANDOM;
+	if (tb[TCA_NETEM_LOSS])
+		ret = get_loss_clg(sch, tb[TCA_NETEM_LOSS]);
  out:
 	sch_tree_unlock(sch);
 
-	return 0;
+	return ret;
 }
 
 /*
@@ -542,6 +748,7 @@ static int netem_init(struct Qdisc *sch,
 
 	qdisc_watchdog_init(&q->watchdog, sch);
 
+	q->loss_model = CLG_RANDOM;
 	q->qdisc = qdisc_create_dflt(qdisc_dev(sch), sch->dev_queue,
 				     ops, TC_H_MAKE(sch->handle, 1));
 	if (!q->qdisc) {
@@ -566,6 +773,52 @@ static void netem_destroy(struct Qdisc *
 	vfree(q->delay_dist);
 }
 
+static int dump_loss_model(const struct netem_sched_data *q, struct sk_buff *skb)
+{
+	struct nlattr *nest;
+
+	nest = nla_nest_start(skb, NETEM_LOSS_MAX);
+	if (nest == NULL)
+		goto nla_put_failure;
+
+	switch (q->loss_model) {
+	case CLG_RANDOM:
+		nla_nest_cancel(skb, nest);
+		return 0;	/* no data */
+
+	case CLG_4_STATES: {
+		struct tc_netem_gimodel gi = {
+			.p13 = q->clg.a1,
+			.p31 = q->clg.a2,
+			.p32 = q->clg.a3,
+			.p14 = q->clg.a4,
+			.p23 = q->clg.a5,
+		};
+
+		NLA_PUT(skb, NETEM_LOSS_GI, sizeof(gi), &gi);
+		break;
+	}
+	case CLG_GILB_ELL: {
+		struct tc_netem_gemodel ge = {
+			.p = q->clg.a1,
+			.r = q->clg.a2,
+			.h = q->clg.a3,
+			.k1 = q->clg.a4,
+		};
+
+		NLA_PUT(skb, NETEM_LOSS_GE, sizeof(ge), &ge);
+		break;
+	}
+	}
+
+	nla_nest_end(skb, nest);
+	return 0;
+
+nla_put_failure:
+	nla_nest_cancel(skb, nest);
+	return -1;
+}
+
 static int netem_dump(struct Qdisc *sch, struct sk_buff *skb)
 {
 	const struct netem_sched_data *q = qdisc_priv(sch);
@@ -603,6 +856,9 @@ static int netem_dump(struct Qdisc *sch,
 			d->size * sizeof(__s16), d->table);
 	}
 
+	if (dump_loss_model(q, skb) != 0)
+		goto nla_put_failure;
+
 	return nla_nest_end(skb, nla);
 
 nla_put_failure:
--- a/include/linux/pkt_sched.h	2010-08-02 15:03:03.038476487 -0700
+++ b/include/linux/pkt_sched.h	2010-08-02 15:03:04.746482821 -0700
@@ -435,6 +435,7 @@ enum {
 	TCA_NETEM_DELAY_DIST,
 	TCA_NETEM_REORDER,
 	TCA_NETEM_CORRUPT,
+	TCA_NETEM_LOSS,
 	__TCA_NETEM_MAX,
 };
 
@@ -465,6 +466,30 @@ struct tc_netem_corrupt {
 	__u32	correlation;
 };
 
+enum {
+	NETEM_LOSS_GI,		/* General Intuitive - 4 state model */
+	NETEM_LOSS_GE,		/* Gilbert Elliot models */
+	__NETEM_LOSS_MAX
+};
+#define NETEM_LOSS_MAX (__NETEM_LOSS_MAX - 1)
+
+/* State transition probablities for 4 state model */
+struct tc_netem_gimodel {
+	__u32	p13;
+	__u32	p31;
+	__u32	p32;
+	__u32	p14;
+	__u32	p23;
+};
+
+/* Gilbert-Elliot models */
+struct tc_netem_gemodel {
+	__u32 p;
+	__u32 r;
+	__u32 h;
+	__u32 k1;
+};
+
 #define NETEM_DIST_SCALE	8192
 #define NETEM_DIST_MAX		16384
 

[PATCH 9/9] netem: restore no jitter option

[Stephen Hemminger]

[-- Attachment #1: netem-queue.patch --]
[-- Type: text/plain, Size: 1366 bytes --]

Before netem was forced to be classless, it was possible to prevent
packet reordering by using a pure FIFO queue. This restores the option,
although now it has to be done as a module parameter.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/net/sched/sch_netem.c	2010-08-02 15:02:36.654303844 -0700
+++ b/net/sched/sch_netem.c	2010-08-02 15:02:59.146459862 -0700
@@ -87,6 +87,10 @@ static inline struct netem_skb_cb *netem
 	return (struct netem_skb_cb *)qdisc_skb_cb(skb)->data;
 }
 
+static int jitter = 1;
+module_param(jitter, int, 0);
+MODULE_PARM_DESC(jitter, "reorder packets based on delay");
+
 /* init_crandom - initialize correlated random number generator
  * Use entropy source for initial seed.
  */
@@ -531,6 +535,7 @@ static struct Qdisc_ops tfifo_qdisc_ops 
 static int netem_init(struct Qdisc *sch, struct nlattr *opt)
 {
 	struct netem_sched_data *q = qdisc_priv(sch);
+	struct Qdisc_ops *ops = jitter ? &tfifo_qdisc_ops : &pfifo_qdisc_ops;
 	int ret;
 
 	if (!opt)
@@ -539,8 +544,7 @@ static int netem_init(struct Qdisc *sch,
 	qdisc_watchdog_init(&q->watchdog, sch);
 
 	q->qdisc = qdisc_create_dflt(qdisc_dev(sch), sch->dev_queue,
-				     &tfifo_qdisc_ops,
-				     TC_H_MAKE(sch->handle, 1));
+				     ops, TC_H_MAKE(sch->handle, 1));
 	if (!q->qdisc) {
 		pr_debug("netem: qdisc create failed\n");
 		return -ENOMEM;

Re: [PATCH 1/9] net classifier: dont allow filters on semi-classful qdisc

[Jarek Poplawski]

Stephen Hemminger wrote, On -10.01.-28163 20:59:

> There are several qdisc which only support a single class (sfq, mq, tbf)
> and the kernel would dereference a null pointer (bind_tcf), if a user
> attempted to apply a filter one of these classes.


mq and tbf can't have this issue because they don't have
.tcf_chain class method. sfq should support it on purpose
after this patch:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7d2681a6ff4f9ab5e48d02550b4c6338f1638998
and needs tiny fix only.


Jarek P.

> 
> This patch changes the tcf_bind_filter to return an error in
> these cases.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> 
> ---
> This needs to go in net-2.6 and stable.
> 
>  include/net/pkt_cls.h   |   12 +++++++++---
>  net/sched/cls_basic.c   |    4 +++-
>  net/sched/cls_fw.c      |    6 ++++--
>  net/sched/cls_route.c   |    4 +++-
>  net/sched/cls_tcindex.c |    4 +++-
>  net/sched/cls_u32.c     |    4 +++-
>  6 files changed, 25 insertions(+), 9 deletions(-)
> 
> --- a/include/net/pkt_cls.h	2010-08-06 11:51:18.903581556 -0700
> +++ b/include/net/pkt_cls.h	2010-08-06 12:20:02.072241508 -0700
> @@ -40,15 +40,21 @@ cls_set_class(struct tcf_proto *tp, unsi
>  	return old_cl;
>  }
>  
> -static inline void
> +static inline int
>  tcf_bind_filter(struct tcf_proto *tp, struct tcf_result *r, unsigned long base)
>  {
> +	const struct Qdisc_class_ops *cops = tp->q->ops->cl_ops;
>  	unsigned long cl;
>  
> -	cl = tp->q->ops->cl_ops->bind_tcf(tp->q, base, r->classid);
> +	if (!cops->bind_tcf)
> +		return -EINVAL;
> +
> +	cl = cops->bind_tcf(tp->q, base, r->classid);
>  	cl = cls_set_class(tp, &r->class, cl);
>  	if (cl)
> -		tp->q->ops->cl_ops->unbind_tcf(tp->q, cl);
> +		cops->unbind_tcf(tp->q, cl);
> +
> +	return 0;
>  }
>  
>  static inline void
> --- a/net/sched/cls_basic.c	2010-08-06 11:51:18.923582342 -0700
> +++ b/net/sched/cls_basic.c	2010-08-06 11:55:13.292553190 -0700
> @@ -153,7 +153,9 @@ static inline int basic_set_parms(struct
>  
>  	if (tb[TCA_BASIC_CLASSID]) {
>  		f->res.classid = nla_get_u32(tb[TCA_BASIC_CLASSID]);
> -		tcf_bind_filter(tp, &f->res, base);
> +		err = tcf_bind_filter(tp, &f->res, base);
> +		if (err)
> +			goto errout;
>  	}
>  
>  	tcf_exts_change(tp, &f->exts, &e);
> --- a/net/sched/cls_fw.c	2010-08-06 11:51:18.943583126 -0700
> +++ b/net/sched/cls_fw.c	2010-08-06 11:55:39.085476144 -0700
> @@ -206,10 +206,11 @@ fw_change_attrs(struct tcf_proto *tp, st
>  	if (err < 0)
>  		return err;
>  
> -	err = -EINVAL;
>  	if (tb[TCA_FW_CLASSID]) {
>  		f->res.classid = nla_get_u32(tb[TCA_FW_CLASSID]);
> -		tcf_bind_filter(tp, &f->res, base);
> +		err = tcf_bind_filter(tp, &f->res, base);
> +		if (err)
> +			goto errout;
>  	}
>  
>  #ifdef CONFIG_NET_CLS_IND
> @@ -220,6 +221,7 @@ fw_change_attrs(struct tcf_proto *tp, st
>  	}
>  #endif /* CONFIG_NET_CLS_IND */
>  
> +	err = -EINVAL;
>  	if (tb[TCA_FW_MASK]) {
>  		mask = nla_get_u32(tb[TCA_FW_MASK]);
>  		if (mask != head->mask)
> --- a/net/sched/cls_route.c	2010-08-06 11:51:18.959583757 -0700
> +++ b/net/sched/cls_route.c	2010-08-06 11:55:50.077870498 -0700
> @@ -412,7 +412,9 @@ static int route4_set_parms(struct tcf_p
>  
>  	if (tb[TCA_ROUTE4_CLASSID]) {
>  		f->res.classid = nla_get_u32(tb[TCA_ROUTE4_CLASSID]);
> -		tcf_bind_filter(tp, &f->res, base);
> +		err = tcf_bind_filter(tp, &f->res, base);
> +		if (err)
> +			goto errout;
>  	}
>  
>  	tcf_exts_change(tp, &f->exts, &e);
> --- a/net/sched/cls_tcindex.c	2010-08-06 11:51:18.999585326 -0700
> +++ b/net/sched/cls_tcindex.c	2010-08-06 11:56:01.486283847 -0700
> @@ -295,7 +295,9 @@ tcindex_set_parms(struct tcf_proto *tp, 
>  
>  	if (tb[TCA_TCINDEX_CLASSID]) {
>  		cr.res.classid = nla_get_u32(tb[TCA_TCINDEX_CLASSID]);
> -		tcf_bind_filter(tp, &cr.res, base);
> +		err = tcf_bind_filter(tp, &cr.res, base);
> +		if (err)
> +			goto errout;
>  	}
>  
>  	tcf_exts_change(tp, &cr.exts, &e);
> --- a/net/sched/cls_u32.c	2010-08-06 11:51:19.019586112 -0700
> +++ b/net/sched/cls_u32.c	2010-08-06 11:56:12.390678703 -0700
> @@ -528,7 +528,9 @@ static int u32_set_parms(struct tcf_prot
>  	}
>  	if (tb[TCA_U32_CLASSID]) {
>  		n->res.classid = nla_get_u32(tb[TCA_U32_CLASSID]);
> -		tcf_bind_filter(tp, &n->res, base);
> +		err = tcf_bind_filter(tp, &n->res, base);
> +		if (err)
> +			goto errout;
>  	}
>  
>  #ifdef CONFIG_NET_CLS_IND
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/9] net classifier: dont allow filters on semi-classful qdisc

[Stephen Hemminger]

On Fri, 06 Aug 2010 23:24:47 +0200
Jarek Poplawski <jarkao2@gmail.com> wrote:

> Stephen Hemminger wrote, On -10.01.-28163 20:59:
> 
> > There are several qdisc which only support a single class (sfq, mq, tbf)
> > and the kernel would dereference a null pointer (bind_tcf), if a user
> > attempted to apply a filter one of these classes.
> 
> 
> mq and tbf can't have this issue because they don't have
> .tcf_chain class method. sfq should support it on purpose
> after this patch:
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7d2681a6ff4f9ab5e48d02550b4c6338f1638998
> and needs tiny fix only.

Probably best to fix both ways.  Fix sfq to allow filters to
be chained, and fix API to prevent refuse to allow qdisc to
register with tcf_chain && !bind_tcf

[PATCH] sfq: add dummy bind/unbind handles

[Stephen Hemminger]

Applying a filter to an SFQ qdisc would cause null dereference
in tcf_bind_filter because although SFQ is classful it didn't
have all the necessary equipment.

Better alternative to changing tcf_bind API is to just fix
SFQ.  This should go to net-2.6 and stable.


Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/net/sched/sch_sfq.c	2010-08-06 15:07:26.552820159 -0700
+++ b/net/sched/sch_sfq.c	2010-08-06 15:14:24.458287452 -0700
@@ -502,6 +502,10 @@ static unsigned long sfq_get(struct Qdis
 	return 0;
 }
 
+static void sfq_put(struct Qdisc *q, unsigned long cl)
+{
+}
+
 static struct tcf_proto **sfq_find_tcf(struct Qdisc *sch, unsigned long cl)
 {
 	struct sfq_sched_data *q = qdisc_priv(sch);
@@ -511,6 +515,12 @@ static struct tcf_proto **sfq_find_tcf(s
 	return &q->filter_list;
 }
 
+static unsigned long sfq_bind_tcf(struct Qdisc *sch, unsigned long parent,
+				  u32 cl)
+{
+	return 0;
+}
+
 static int sfq_dump_class(struct Qdisc *sch, unsigned long cl,
 			  struct sk_buff *skb, struct tcmsg *tcm)
 {
@@ -556,6 +566,8 @@ static void sfq_walk(struct Qdisc *sch, 
 static const struct Qdisc_class_ops sfq_class_ops = {
 	.get		=	sfq_get,
 	.tcf_chain	=	sfq_find_tcf,
+	.bind_tcf	=	sfq_bind_tcf,
+	.unbind_tcf	=	sfq_put,
 	.dump		=	sfq_dump_class,
 	.dump_stats	=	sfq_dump_class_stats,

Re: [PATCH 1/9] net classifier: dont allow filters on semi-classful qdisc

[Jarek Poplawski]

Stephen Hemminger wrote, On 06.08.2010 23:58:

> On Fri, 06 Aug 2010 23:24:47 +0200
> Jarek Poplawski <jarkao2@gmail.com> wrote:
> 
>> Stephen Hemminger wrote, On -10.01.-28163 20:59:
>>
>>> There are several qdisc which only support a single class (sfq, mq, tbf)
>>> and the kernel would dereference a null pointer (bind_tcf), if a user
>>> attempted to apply a filter one of these classes.
>>
>>
>> mq and tbf can't have this issue because they don't have
>> .tcf_chain class method. sfq should support it on purpose
>> after this patch:
>>
>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7d2681a6ff4f9ab5e48d02550b4c6338f1638998
>> and needs tiny fix only.
> 
> Probably best to fix both ways.  Fix sfq to allow filters to
> be chained, and fix API to prevent refuse to allow qdisc to
> register with tcf_chain && !bind_tcf
> 

Yes, but your patch needs a different changelog and I'm not sure
it's necessary for stable (there should be no other such cases
for now).

Jarek P.

Re: [PATCH] sfq: add dummy bind/unbind handles

[Jarek Poplawski]

Stephen Hemminger wrote, On 07.08.2010 00:23:

> Applying a filter to an SFQ qdisc would cause null dereference
> in tcf_bind_filter because although SFQ is classful it didn't
> have all the necessary equipment.
> 
> Better alternative to changing tcf_bind API is to just fix
> SFQ.  This should go to net-2.6 and stable.
> 

Hmm... FYI, actually I've sent already a similar patch to the
original bug report thread (except .unbind_tcf method which
doesn't matter for fixing this bug, so should be rather
implemented in a separate patch, if needed at all in this
case).

Jarek P.

> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> --- a/net/sched/sch_sfq.c	2010-08-06 15:07:26.552820159 -0700
> +++ b/net/sched/sch_sfq.c	2010-08-06 15:14:24.458287452 -0700
> @@ -502,6 +502,10 @@ static unsigned long sfq_get(struct Qdis
>  	return 0;
>  }
>  
> +static void sfq_put(struct Qdisc *q, unsigned long cl)
> +{
> +}
> +
>  static struct tcf_proto **sfq_find_tcf(struct Qdisc *sch, unsigned long cl)
>  {
>  	struct sfq_sched_data *q = qdisc_priv(sch);
> @@ -511,6 +515,12 @@ static struct tcf_proto **sfq_find_tcf(s
>  	return &q->filter_list;
>  }
>  
> +static unsigned long sfq_bind_tcf(struct Qdisc *sch, unsigned long parent,
> +				  u32 cl)
> +{
> +	return 0;
> +}
> +
>  static int sfq_dump_class(struct Qdisc *sch, unsigned long cl,
>  			  struct sk_buff *skb, struct tcmsg *tcm)
>  {
> @@ -556,6 +566,8 @@ static void sfq_walk(struct Qdisc *sch, 
>  static const struct Qdisc_class_ops sfq_class_ops = {
>  	.get		=	sfq_get,
>  	.tcf_chain	=	sfq_find_tcf,
> +	.bind_tcf	=	sfq_bind_tcf,
> +	.unbind_tcf	=	sfq_put,
>  	.dump		=	sfq_dump_class,
>  	.dump_stats	=	sfq_dump_class_stats,
> 

Re: [PATCH] sfq: add dummy bind/unbind handles

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Sat, 07 Aug 2010 01:17:07 +0200

> Stephen Hemminger wrote, On 07.08.2010 00:23:
> 
>> Applying a filter to an SFQ qdisc would cause null dereference
>> in tcf_bind_filter because although SFQ is classful it didn't
>> have all the necessary equipment.
>> 
>> Better alternative to changing tcf_bind API is to just fix
>> SFQ.  This should go to net-2.6 and stable.
>> 
> 
> Hmm... FYI, actually I've sent already a similar patch to the
> original bug report thread (except .unbind_tcf method which
> doesn't matter for fixing this bug, so should be rather
> implemented in a separate patch, if needed at all in this
> case).

Agreed, I can't see a way that unbind can ever be invoked
if the bind call always returns zero.

Therefore I'll apply Jarek's patch, thanks everyone.

Re: [PATCH 1/9] net classifier: dont allow filters on semi-classful qdisc

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Sat, 07 Aug 2010 00:26:17 +0200

> Stephen Hemminger wrote, On 06.08.2010 23:58:
> 
>> On Fri, 06 Aug 2010 23:24:47 +0200
>> Jarek Poplawski <jarkao2@gmail.com> wrote:
>> 
>>> Stephen Hemminger wrote, On -10.01.-28163 20:59:
>>>
>>>> There are several qdisc which only support a single class (sfq, mq, tbf)
>>>> and the kernel would dereference a null pointer (bind_tcf), if a user
>>>> attempted to apply a filter one of these classes.
>>>
>>>
>>> mq and tbf can't have this issue because they don't have
>>> .tcf_chain class method. sfq should support it on purpose
>>> after this patch:
>>>
>>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7d2681a6ff4f9ab5e48d02550b4c6338f1638998
>>> and needs tiny fix only.
>> 
>> Probably best to fix both ways.  Fix sfq to allow filters to
>> be chained, and fix API to prevent refuse to allow qdisc to
>> register with tcf_chain && !bind_tcf
>> 
> 
> Yes, but your patch needs a different changelog and I'm not sure
> it's necessary for stable (there should be no other such cases
> for now).

Right.

I'm tossing this series since there are dependencies upon this first
patch for which we've arrived at a different solution (Jarek's
patch).

Re: [PATCH] sfq: add dummy bind/unbind handles

[Jarek Poplawski]

David Miller wrote, On 08.08.2010 07:45:

> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Sat, 07 Aug 2010 01:17:07 +0200
> 
>> Stephen Hemminger wrote, On 07.08.2010 00:23:
>>
>>> Applying a filter to an SFQ qdisc would cause null dereference
>>> in tcf_bind_filter because although SFQ is classful it didn't
>>> have all the necessary equipment.
>>>
>>> Better alternative to changing tcf_bind API is to just fix
>>> SFQ.  This should go to net-2.6 and stable.
>>>
>>
>> Hmm... FYI, actually I've sent already a similar patch to the
>> original bug report thread (except .unbind_tcf method which
>> doesn't matter for fixing this bug, so should be rather
>> implemented in a separate patch, if needed at all in this
>> case).
> 
> Agreed, I can't see a way that unbind can ever be invoked
> if the bind call always returns zero.

To tell the truth, I think unbind should be implemented anyway,
just for consistency, safety, and easier verification. But, looking
at a similar case of .get and .put in the same driver, Patrick
seemed to do it purposely, so I expected some discussion about the
rules yet, and made it minimal to ease merging to older kernels.
 
Thanks,
Jarek P.

Re: Re: [PATCH] sfq: add dummy bind/unbind handles

[Franchoze Eric]

08.08.10, 11:04, "Jarek Poplawski" <jarkao2@gmail.com>:

> David Miller wrote, On 08.08.2010 07:45:
>  
>  > From: Jarek Poplawski 
>  > Date: Sat, 07 Aug 2010 01:17:07 +0200
>  > 
>  >> Stephen Hemminger wrote, On 07.08.2010 00:23:
>  >>
>  >>> Applying a filter to an SFQ qdisc would cause null dereference
>  >>> in tcf_bind_filter because although SFQ is classful it didn't
>  >>> have all the necessary equipment.
>  >>>
>  >>> Better alternative to changing tcf_bind API is to just fix
>  >>> SFQ.  This should go to net-2.6 and stable.
>  >>>
>  >>
>  >> Hmm... FYI, actually I've sent already a similar patch to the
>  >> original bug report thread (except .unbind_tcf method which
>  >> doesn't matter for fixing this bug, so should be rather
>  >> implemented in a separate patch, if needed at all in this
>  >> case).
>  > 
>  > Agreed, I can't see a way that unbind can ever be invoked
>  > if the bind call always returns zero.
>  
>  To tell the truth, I think unbind should be implemented anyway,
>  just for consistency, safety, and easier verification. But, looking
>  at a similar case of .get and .put in the same driver, Patrick
>  seemed to do it purposely, so I expected some discussion about the
>  rules yet, and made it minimal to ease merging to older kernels.
>   
>  Thanks,
>  Jarek P.
>  
>  

As for me it's better to add unbind now that get unexpected null derefance in future with API changing...