[Patch net-next 0/5] net_sched: introduce static flags for qdisc's

[Patch net-next 0/5] net_sched: introduce static flags for qdisc's

[Cong Wang]

The main goal of this patchset is to improve the behavior of setting
the default qdisc. Current behavior has no error check, no check for
ingress and _can_ crash the kernel with some buggy implementation.

We only have flags for each instance of qdisc's, for flags like
if a qdisc is a fifo qdisc, they can simply be moved into qdisc->ops,
as shown by patch 1, 2, 5. Patch 4 just uses this for error checking
when setting default qdisc.

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

---

Cong Wang (5):
  net_sched: move some qdisc flag into qdisc ops
  net_sched: move TCQ_F_MQROOT into qdisc ops
  net_sched: use a flag to indicate fifo qdiscs instead of the name
  net_sched: forbid setting default qdisc to inappropriate ones
  net_sched: move ingress flag into qdisc ops

 include/net/sch_generic.h |  9 ++++++---
 net/sched/sch_api.c       | 40 +++++++++++++++++++++++++++-------------
 net/sched/sch_fifo.c      |  6 ++++--
 net/sched/sch_fq.c        |  1 +
 net/sched/sch_fq_codel.c  |  1 +
 net/sched/sch_generic.c   | 11 ++++++-----
 net/sched/sch_ingress.c   |  1 +
 net/sched/sch_mq.c        |  2 +-
 net/sched/sch_mqprio.c    |  2 +-
 net/sched/sch_sfq.c       |  1 +
 10 files changed, 49 insertions(+), 25 deletions(-)

-- 
1.8.3.1

[Patch net-next 1/5] net_sched: move some qdisc flag into qdisc ops

[Cong Wang]

For those static flags, that is never changed dynamically,
we could just move them into qdisc->ops. This will be used
by the following patches.

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/sch_generic.h |  3 ++-
 net/sched/sch_api.c       |  4 ++--
 net/sched/sch_generic.c   | 10 +++++-----
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 2eab08c..fe835e1 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -48,7 +48,6 @@ struct Qdisc {
 	int 			(*enqueue)(struct sk_buff *skb, struct Qdisc *dev);
 	struct sk_buff *	(*dequeue)(struct Qdisc *dev);
 	unsigned int		flags;
-#define TCQ_F_BUILTIN		1
 #define TCQ_F_INGRESS		2
 #define TCQ_F_CAN_BYPASS	4
 #define TCQ_F_MQROOT		8
@@ -181,6 +180,8 @@ struct Qdisc_ops {
 	const struct Qdisc_class_ops	*cl_ops;
 	char			id[IFNAMSIZ];
 	int			priv_size;
+#define QDISC_F_BUILTIN		1
+	unsigned int		flags;
 
 	int 			(*enqueue)(struct sk_buff *, struct Qdisc *);
 	struct sk_buff *	(*dequeue)(struct Qdisc *);
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index f06aa01..f2b194b 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -260,7 +260,7 @@ static struct Qdisc *qdisc_match_from_root(struct Qdisc *root, u32 handle)
 {
 	struct Qdisc *q;
 
-	if (!(root->flags & TCQ_F_BUILTIN) &&
+	if (!(root->ops->flags & QDISC_F_BUILTIN) &&
 	    root->handle == handle)
 		return root;
 
@@ -1384,7 +1384,7 @@ static int tc_fill_qdisc(struct sk_buff *skb, struct Qdisc *q, u32 clid,
 
 static bool tc_qdisc_dump_ignore(struct Qdisc *q)
 {
-	return (q->flags & TCQ_F_BUILTIN) ? true : false;
+	return (q->ops->flags & QDISC_F_BUILTIN) ? true : false;
 }
 
 static int qdisc_notify(struct net *net, struct sk_buff *oskb,
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 942fea8..460388a 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -392,6 +392,7 @@ static struct sk_buff *noop_dequeue(struct Qdisc *qdisc)
 
 struct Qdisc_ops noop_qdisc_ops __read_mostly = {
 	.id		=	"noop",
+	.flags		=	QDISC_F_BUILTIN,
 	.priv_size	=	0,
 	.enqueue	=	noop_enqueue,
 	.dequeue	=	noop_dequeue,
@@ -407,7 +408,6 @@ static struct netdev_queue noop_netdev_queue = {
 struct Qdisc noop_qdisc = {
 	.enqueue	=	noop_enqueue,
 	.dequeue	=	noop_dequeue,
-	.flags		=	TCQ_F_BUILTIN,
 	.ops		=	&noop_qdisc_ops,
 	.list		=	LIST_HEAD_INIT(noop_qdisc.list),
 	.q.lock		=	__SPIN_LOCK_UNLOCKED(noop_qdisc.q.lock),
@@ -418,6 +418,7 @@ EXPORT_SYMBOL(noop_qdisc);
 
 static struct Qdisc_ops noqueue_qdisc_ops __read_mostly = {
 	.id		=	"noqueue",
+	.flags		=	QDISC_F_BUILTIN,
 	.priv_size	=	0,
 	.enqueue	=	noop_enqueue,
 	.dequeue	=	noop_dequeue,
@@ -434,7 +435,6 @@ static struct netdev_queue noqueue_netdev_queue = {
 static struct Qdisc noqueue_qdisc = {
 	.enqueue	=	NULL,
 	.dequeue	=	noop_dequeue,
-	.flags		=	TCQ_F_BUILTIN,
 	.ops		=	&noqueue_qdisc_ops,
 	.list		=	LIST_HEAD_INIT(noqueue_qdisc.list),
 	.q.lock		=	__SPIN_LOCK_UNLOCKED(noqueue_qdisc.q.lock),
@@ -676,7 +676,7 @@ void qdisc_destroy(struct Qdisc *qdisc)
 {
 	const struct Qdisc_ops  *ops = qdisc->ops;
 
-	if (qdisc->flags & TCQ_F_BUILTIN ||
+	if (ops->flags & QDISC_F_BUILTIN ||
 	    !atomic_dec_and_test(&qdisc->refcnt))
 		return;
 
@@ -777,7 +777,7 @@ static void transition_one_qdisc(struct net_device *dev,
 	struct Qdisc *new_qdisc = dev_queue->qdisc_sleeping;
 	int *need_watchdog_p = _need_watchdog;
 
-	if (!(new_qdisc->flags & TCQ_F_BUILTIN))
+	if (!(new_qdisc->ops->flags & QDISC_F_BUILTIN))
 		clear_bit(__QDISC_STATE_DEACTIVATED, &new_qdisc->state);
 
 	rcu_assign_pointer(dev_queue->qdisc, new_qdisc);
@@ -826,7 +826,7 @@ static void dev_deactivate_queue(struct net_device *dev,
 	if (qdisc) {
 		spin_lock_bh(qdisc_lock(qdisc));
 
-		if (!(qdisc->flags & TCQ_F_BUILTIN))
+		if (!(qdisc->ops->flags & QDISC_F_BUILTIN))
 			set_bit(__QDISC_STATE_DEACTIVATED, &qdisc->state);
 
 		rcu_assign_pointer(dev_queue->qdisc, qdisc_default);
-- 
1.8.3.1

[Patch net-next 2/5] net_sched: move TCQ_F_MQROOT into qdisc ops

[Cong Wang]

It is just another static flag which can be moved.

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/sch_generic.h | 2 +-
 net/sched/sch_api.c       | 6 +++---
 net/sched/sch_mq.c        | 2 +-
 net/sched/sch_mqprio.c    | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index fe835e1..943736a 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -50,7 +50,6 @@ struct Qdisc {
 	unsigned int		flags;
 #define TCQ_F_INGRESS		2
 #define TCQ_F_CAN_BYPASS	4
-#define TCQ_F_MQROOT		8
 #define TCQ_F_ONETXQUEUE	0x10 /* dequeue_skb() can assume all skbs are for
 				      * q->dev_queue : It can test
 				      * netif_xmit_frozen_or_stopped() before
@@ -181,6 +180,7 @@ struct Qdisc_ops {
 	char			id[IFNAMSIZ];
 	int			priv_size;
 #define QDISC_F_BUILTIN		1
+#define QDISC_F_MQ		2
 	unsigned int		flags;
 
 	int 			(*enqueue)(struct sk_buff *, struct Qdisc *);
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index f2b194b..90a4cf9 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -970,12 +970,12 @@ qdisc_create(struct net_device *dev, struct netdev_queue *dev_queue,
 			spinlock_t *root_lock;
 
 			err = -EOPNOTSUPP;
-			if (sch->flags & TCQ_F_MQROOT)
+			if (sch->ops->flags & QDISC_F_MQ)
 				goto err_out4;
 
 			if ((sch->parent != TC_H_ROOT) &&
 			    !(sch->flags & TCQ_F_INGRESS) &&
-			    (!p || !(p->flags & TCQ_F_MQROOT)))
+			    (!p || !(p->ops->flags & QDISC_F_MQ)))
 				root_lock = qdisc_root_sleeping_lock(sch);
 			else
 				root_lock = qdisc_lock(sch);
@@ -1041,7 +1041,7 @@ static int qdisc_change(struct Qdisc *sch, struct nlattr **tca)
 	if (tca[TCA_RATE]) {
 		/* NB: ignores errors from replace_estimator
 		   because change can't be undone. */
-		if (sch->flags & TCQ_F_MQROOT)
+		if (sch->ops->flags & QDISC_F_MQ)
 			goto out;
 		gen_replace_estimator(&sch->bstats,
 				      sch->cpu_bstats,
diff --git a/net/sched/sch_mq.c b/net/sched/sch_mq.c
index f3cbaec..cab9fc2 100644
--- a/net/sched/sch_mq.c
+++ b/net/sched/sch_mq.c
@@ -66,7 +66,6 @@ static int mq_init(struct Qdisc *sch, struct nlattr *opt)
 		qdisc->flags |= TCQ_F_ONETXQUEUE;
 	}
 
-	sch->flags |= TCQ_F_MQROOT;
 	return 0;
 
 err:
@@ -237,6 +236,7 @@ static const struct Qdisc_class_ops mq_class_ops = {
 struct Qdisc_ops mq_qdisc_ops __read_mostly = {
 	.cl_ops		= &mq_class_ops,
 	.id		= "mq",
+	.flags		= QDISC_F_MQ,
 	.priv_size	= sizeof(struct mq_sched),
 	.init		= mq_init,
 	.destroy	= mq_destroy,
diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
index 3811a74..dc208c2 100644
--- a/net/sched/sch_mqprio.c
+++ b/net/sched/sch_mqprio.c
@@ -155,7 +155,6 @@ static int mqprio_init(struct Qdisc *sch, struct nlattr *opt)
 	for (i = 0; i < TC_BITMASK + 1; i++)
 		netdev_set_prio_tc_map(dev, i, qopt->prio_tc_map[i]);
 
-	sch->flags |= TCQ_F_MQROOT;
 	return 0;
 
 err:
@@ -404,6 +403,7 @@ static const struct Qdisc_class_ops mqprio_class_ops = {
 static struct Qdisc_ops mqprio_qdisc_ops __read_mostly = {
 	.cl_ops		= &mqprio_class_ops,
 	.id		= "mqprio",
+	.flags		= QDISC_F_MQ,
 	.priv_size	= sizeof(struct mqprio_sched),
 	.init		= mqprio_init,
 	.destroy	= mqprio_destroy,
-- 
1.8.3.1

[Patch net-next 3/5] net_sched: use a flag to indicate fifo qdiscs instead of the name

[Cong Wang]

Relying on its name is a bad practice.

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/sch_generic.h | 1 +
 net/sched/sch_fifo.c      | 6 ++++--
 net/sched/sch_generic.c   | 1 +
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 943736a..f7ad38a 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -181,6 +181,7 @@ struct Qdisc_ops {
 	int			priv_size;
 #define QDISC_F_BUILTIN		1
 #define QDISC_F_MQ		2
+#define QDISC_F_FIFO		4
 	unsigned int		flags;
 
 	int 			(*enqueue)(struct sk_buff *, struct Qdisc *);
diff --git a/net/sched/sch_fifo.c b/net/sched/sch_fifo.c
index 2177eac..e51d786 100644
--- a/net/sched/sch_fifo.c
+++ b/net/sched/sch_fifo.c
@@ -96,6 +96,7 @@ static int fifo_dump(struct Qdisc *sch, struct sk_buff *skb)
 struct Qdisc_ops pfifo_qdisc_ops __read_mostly = {
 	.id		=	"pfifo",
 	.priv_size	=	0,
+	.flags 		=	QDISC_F_FIFO,
 	.enqueue	=	pfifo_enqueue,
 	.dequeue	=	qdisc_dequeue_head,
 	.peek		=	qdisc_peek_head,
@@ -111,6 +112,7 @@ EXPORT_SYMBOL(pfifo_qdisc_ops);
 struct Qdisc_ops bfifo_qdisc_ops __read_mostly = {
 	.id		=	"bfifo",
 	.priv_size	=	0,
+	.flags 		=	QDISC_F_FIFO,
 	.enqueue	=	bfifo_enqueue,
 	.dequeue	=	qdisc_dequeue_head,
 	.peek		=	qdisc_peek_head,
@@ -126,6 +128,7 @@ EXPORT_SYMBOL(bfifo_qdisc_ops);
 struct Qdisc_ops pfifo_head_drop_qdisc_ops __read_mostly = {
 	.id		=	"pfifo_head_drop",
 	.priv_size	=	0,
+	.flags 		=	QDISC_F_FIFO,
 	.enqueue	=	pfifo_tail_enqueue,
 	.dequeue	=	qdisc_dequeue_head,
 	.peek		=	qdisc_peek_head,
@@ -143,8 +146,7 @@ int fifo_set_limit(struct Qdisc *q, unsigned int limit)
 	struct nlattr *nla;
 	int ret = -ENOMEM;
 
-	/* Hack to avoid sending change message to non-FIFO */
-	if (strncmp(q->ops->id + 1, "fifo", 4) != 0)
+	if (!(q->ops->flags & QDISC_F_FIFO))
 		return 0;
 
 	nla = kmalloc(nla_attr_size(sizeof(struct tc_fifo_qopt)), GFP_KERNEL);
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 460388a..70b7713 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -567,6 +567,7 @@ static int pfifo_fast_init(struct Qdisc *qdisc, struct nlattr *opt)
 struct Qdisc_ops pfifo_fast_ops __read_mostly = {
 	.id		=	"pfifo_fast",
 	.priv_size	=	sizeof(struct pfifo_fast_priv),
+	.flags		=	QDISC_F_FIFO,
 	.enqueue	=	pfifo_fast_enqueue,
 	.dequeue	=	pfifo_fast_dequeue,
 	.peek		=	pfifo_fast_peek,
-- 
1.8.3.1

[Patch net-next 4/5] net_sched: forbid setting default qdisc to inappropriate ones

[Cong Wang]

Currently there is no check for if a qdisc is appropriate
to be used as the default qdisc. This causes we get no
error even we set the default qdisc to an inappropriate one
but an error will be shown up later. This is not good.

Also, for qdisc's like HTB, kernel will just crash when
we use it as default qdisc, because some data structures are
not even initialized yet before checking opt == NULL, the cleanup
doing ->reset() or ->destroy() on them will just crash.

Let's fail as early as we can.

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/sch_generic.h |  1 +
 net/sched/sch_api.c       | 12 ++++++++++--
 net/sched/sch_fifo.c      |  6 +++---
 net/sched/sch_fq.c        |  1 +
 net/sched/sch_fq_codel.c  |  1 +
 net/sched/sch_generic.c   |  2 +-
 net/sched/sch_mq.c        |  2 +-
 net/sched/sch_sfq.c       |  1 +
 8 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index f7ad38a..2e6748e 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -182,6 +182,7 @@ struct Qdisc_ops {
 #define QDISC_F_BUILTIN		1
 #define QDISC_F_MQ		2
 #define QDISC_F_FIFO		4
+#define QDISC_F_DEFAULTABLE	8
 	unsigned int		flags;
 
 	int 			(*enqueue)(struct sk_buff *, struct Qdisc *);
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 90a4cf9..e501e9d 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -227,6 +227,7 @@ static struct Qdisc_ops *qdisc_lookup_default(const char *name)
 int qdisc_set_default(const char *name)
 {
 	const struct Qdisc_ops *ops;
+	int err = 0;
 
 	if (!capable(CAP_NET_ADMIN))
 		return -EPERM;
@@ -243,13 +244,20 @@ int qdisc_set_default(const char *name)
 	}
 
 	if (ops) {
+		if (!(ops->flags & QDISC_F_DEFAULTABLE)) {
+			err = -EINVAL;
+			goto unlock;
+		}
 		/* Set new default */
 		module_put(default_qdisc_ops->owner);
 		default_qdisc_ops = ops;
+	} else {
+		err = -ENOENT;
 	}
-	write_unlock(&qdisc_mod_lock);
 
-	return ops ? 0 : -ENOENT;
+unlock:
+	write_unlock(&qdisc_mod_lock);
+	return err;
 }
 
 /* We know handle. Find qdisc among all qdisc's attached to device
diff --git a/net/sched/sch_fifo.c b/net/sched/sch_fifo.c
index e51d786..83947f6 100644
--- a/net/sched/sch_fifo.c
+++ b/net/sched/sch_fifo.c
@@ -96,7 +96,7 @@ static int fifo_dump(struct Qdisc *sch, struct sk_buff *skb)
 struct Qdisc_ops pfifo_qdisc_ops __read_mostly = {
 	.id		=	"pfifo",
 	.priv_size	=	0,
-	.flags 		=	QDISC_F_FIFO,
+	.flags 		=	QDISC_F_FIFO | QDISC_F_DEFAULTABLE,
 	.enqueue	=	pfifo_enqueue,
 	.dequeue	=	qdisc_dequeue_head,
 	.peek		=	qdisc_peek_head,
@@ -112,7 +112,7 @@ EXPORT_SYMBOL(pfifo_qdisc_ops);
 struct Qdisc_ops bfifo_qdisc_ops __read_mostly = {
 	.id		=	"bfifo",
 	.priv_size	=	0,
-	.flags 		=	QDISC_F_FIFO,
+	.flags 		=	QDISC_F_FIFO | QDISC_F_DEFAULTABLE,
 	.enqueue	=	bfifo_enqueue,
 	.dequeue	=	qdisc_dequeue_head,
 	.peek		=	qdisc_peek_head,
@@ -128,7 +128,7 @@ EXPORT_SYMBOL(bfifo_qdisc_ops);
 struct Qdisc_ops pfifo_head_drop_qdisc_ops __read_mostly = {
 	.id		=	"pfifo_head_drop",
 	.priv_size	=	0,
-	.flags 		=	QDISC_F_FIFO,
+	.flags 		=	QDISC_F_FIFO | QDISC_F_DEFAULTABLE,
 	.enqueue	=	pfifo_tail_enqueue,
 	.dequeue	=	qdisc_dequeue_head,
 	.peek		=	qdisc_peek_head,
diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c
index f377702..e543b41 100644
--- a/net/sched/sch_fq.c
+++ b/net/sched/sch_fq.c
@@ -831,6 +831,7 @@ static int fq_dump_stats(struct Qdisc *sch, struct gnet_dump *d)
 
 static struct Qdisc_ops fq_qdisc_ops __read_mostly = {
 	.id		=	"fq",
+	.flags		=	QDISC_F_DEFAULTABLE,
 	.priv_size	=	sizeof(struct fq_sched_data),
 
 	.enqueue	=	fq_enqueue,
diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
index a9ba030..f8f5e82 100644
--- a/net/sched/sch_fq_codel.c
+++ b/net/sched/sch_fq_codel.c
@@ -625,6 +625,7 @@ static const struct Qdisc_class_ops fq_codel_class_ops = {
 static struct Qdisc_ops fq_codel_qdisc_ops __read_mostly = {
 	.cl_ops		=	&fq_codel_class_ops,
 	.id		=	"fq_codel",
+	.flags		=	QDISC_F_DEFAULTABLE,
 	.priv_size	=	sizeof(struct fq_codel_sched_data),
 	.enqueue	=	fq_codel_enqueue,
 	.dequeue	=	fq_codel_dequeue,
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 70b7713..051adf9 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -567,7 +567,7 @@ static int pfifo_fast_init(struct Qdisc *qdisc, struct nlattr *opt)
 struct Qdisc_ops pfifo_fast_ops __read_mostly = {
 	.id		=	"pfifo_fast",
 	.priv_size	=	sizeof(struct pfifo_fast_priv),
-	.flags		=	QDISC_F_FIFO,
+	.flags		=	QDISC_F_FIFO | QDISC_F_DEFAULTABLE,
 	.enqueue	=	pfifo_fast_enqueue,
 	.dequeue	=	pfifo_fast_dequeue,
 	.peek		=	pfifo_fast_peek,
diff --git a/net/sched/sch_mq.c b/net/sched/sch_mq.c
index cab9fc2..d1027bb 100644
--- a/net/sched/sch_mq.c
+++ b/net/sched/sch_mq.c
@@ -236,7 +236,7 @@ static const struct Qdisc_class_ops mq_class_ops = {
 struct Qdisc_ops mq_qdisc_ops __read_mostly = {
 	.cl_ops		= &mq_class_ops,
 	.id		= "mq",
-	.flags		= QDISC_F_MQ,
+	.flags		= QDISC_F_MQ | QDISC_F_DEFAULTABLE,
 	.priv_size	= sizeof(struct mq_sched),
 	.init		= mq_init,
 	.destroy	= mq_destroy,
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 52f75a5..0479d44 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -890,6 +890,7 @@ static const struct Qdisc_class_ops sfq_class_ops = {
 static struct Qdisc_ops sfq_qdisc_ops __read_mostly = {
 	.cl_ops		=	&sfq_class_ops,
 	.id		=	"sfq",
+	.flags		=	QDISC_F_DEFAULTABLE,
 	.priv_size	=	sizeof(struct sfq_sched_data),
 	.enqueue	=	sfq_enqueue,
 	.dequeue	=	sfq_dequeue,
-- 
1.8.3.1

Re: [Patch net-next 4/5] net_sched: forbid setting default qdisc to inappropriate ones

[Stephen Hemminger]

On Wed, 26 Aug 2015 15:41:26 -0700
Cong Wang <xiyou.wangcong@gmail.com> wrote:

> Currently there is no check for if a qdisc is appropriate
> to be used as the default qdisc. This causes we get no
> error even we set the default qdisc to an inappropriate one
> but an error will be shown up later. This is not good.
> 
> Also, for qdisc's like HTB, kernel will just crash when
> we use it as default qdisc, because some data structures are
> not even initialized yet before checking opt == NULL, the cleanup
> doing ->reset() or ->destroy() on them will just crash.

Why not fix the buggy one's instead?

Re: [Patch net-next 4/5] net_sched: forbid setting default qdisc to inappropriate ones

[Cong Wang]

On Wed, Aug 26, 2015 at 5:08 PM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Wed, 26 Aug 2015 15:41:26 -0700
> Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
>> Currently there is no check for if a qdisc is appropriate
>> to be used as the default qdisc. This causes we get no
>> error even we set the default qdisc to an inappropriate one
>> but an error will be shown up later. This is not good.
>>
>> Also, for qdisc's like HTB, kernel will just crash when
>> we use it as default qdisc, because some data structures are
>> not even initialized yet before checking opt == NULL, the cleanup
>> doing ->reset() or ->destroy() on them will just crash.
>
> Why not fix the buggy one's instead?

They are not exactly buggy, since they are fine in other ->init() calling
cases.

As in the first paragraph you quoted from me, it is more like
a usability issue, for example ingress qdisc can be set as default
without any error at any time.

Re: [Patch net-next 4/5] net_sched: forbid setting default qdisc to inappropriate ones

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Wed, 26 Aug 2015 15:41:26 -0700

> Currently there is no check for if a qdisc is appropriate
> to be used as the default qdisc. This causes we get no
> error even we set the default qdisc to an inappropriate one
> but an error will be shown up later. This is not good.
> 
> Also, for qdisc's like HTB, kernel will just crash when
> we use it as default qdisc, because some data structures are
> not even initialized yet before checking opt == NULL, the cleanup
> doing ->reset() or ->destroy() on them will just crash.
> 
> Let's fail as early as we can.
> 
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

I don't like this.

The situation is that some sophisticated qdiscs can function without
explicit parameters, some cannot.

That is the problem you need to solve.  For example, if "opts" is NULL
HTB should use a reasonable set of defaults instead of failing.

Furthermore, you can improve the behavior when this happens.

When qdisc_create_dflt() returns NULL because ops->init() fails, do
something reasonable.

I'm not applying this patch series, it papers over the issue rather
than actually addressing it properly.

Re: [Patch net-next 4/5] net_sched: forbid setting default qdisc to inappropriate ones

[Cong Wang]

On Thu, Aug 27, 2015 at 3:30 PM, David Miller <davem@davemloft.net> wrote:
> I don't like this.
>
> The situation is that some sophisticated qdiscs can function without
> explicit parameters, some cannot.

This is exactly what this patch tries to solve... I already mark those
with a DEFAULTABLE flag.

>
> That is the problem you need to solve.  For example, if "opts" is NULL
> HTB should use a reasonable set of defaults instead of failing.
>
> Furthermore, you can improve the behavior when this happens.
>
> When qdisc_create_dflt() returns NULL because ops->init() fails, do
> something reasonable.
>
> I'm not applying this patch series, it papers over the issue rather
> than actually addressing it properly.

I wish I never mention that crash, which leads you to think I am trying
to fix a crash rather than a more important issue, usability. See below.

Forget about the crash, consider the current behavior:

# echo htb > default_qdisc
# succeed without any error
(then add a root qdisc and remove it)
# failure shown here in dmesg


And compare it with the behavior after my patch:

# echo htb > default_qdisc
Invalid arguments

I think this is clearly an improvement.

Thanks.

Re: [Patch net-next 4/5] net_sched: forbid setting default qdisc to inappropriate ones

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Thu, 27 Aug 2015 15:39:12 -0700

> On Thu, Aug 27, 2015 at 3:30 PM, David Miller <davem@davemloft.net> wrote:
>> I don't like this.
>>
>> The situation is that some sophisticated qdiscs can function without
>> explicit parameters, some cannot.
> 
> This is exactly what this patch tries to solve... I already mark those
> with a DEFAULTABLE flag.

It is not solving it, if you were solving it you would make all qdisc's
capable of being default instead of giving them what is essentially
"this is broken" flag.
> I wish I never mention that crash, which leads you to think I am trying
> to fix a crash rather than a more important issue, usability. See below.
> 
> Forget about the crash, consider the current behavior:
> 
> # echo htb > default_qdisc
> # succeed without any error
> (then add a root qdisc and remove it)
> # failure shown here in dmesg
> 
> And compare it with the behavior after my patch:
> 
> # echo htb > default_qdisc
> Invalid arguments
> 
> I think this is clearly an improvement.

Long term it's the wrong fix, trust me.

If you fix it properly, by making every qdisc capable of being ->init()'d
without explicit parameters, it will be the best behavior overall.

Re: [Patch net-next 4/5] net_sched: forbid setting default qdisc to inappropriate ones

[Cong Wang]

On Thu, Aug 27, 2015 at 3:42 PM, David Miller <davem@davemloft.net> wrote:
>
> Long term it's the wrong fix, trust me.

So we have plan to convert some non-defaultable qdisc to defaultable?
I don't see a reason here.

>
> If you fix it properly, by making every qdisc capable of being ->init()'d
> without explicit parameters, it will be the best behavior overall.

The problem is ->init() is not even called when setting it as default,
since setting a default qdisc doesn't need to create a qdisc. This is
why the flag has to be in ops->flags rather than qdisc->flags.

Re: [Patch net-next 4/5] net_sched: forbid setting default qdisc to inappropriate ones

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Thu, 27 Aug 2015 15:47:55 -0700

> On Thu, Aug 27, 2015 at 3:42 PM, David Miller <davem@davemloft.net> wrote:
>> If you fix it properly, by making every qdisc capable of being ->init()'d
>> without explicit parameters, it will be the best behavior overall.
> 
> The problem is ->init() is not even called when setting it as default,
> since setting a default qdisc doesn't need to create a qdisc. This is
> why the flag has to be in ops->flags rather than qdisc->flags.

Just sounds like another shortcoming of how default qdiscs are handled,
rather than a reason to not fix things properly.

Re: [Patch net-next 4/5] net_sched: forbid setting default qdisc to inappropriate ones

[Cong Wang]

On Thu, Aug 27, 2015 at 4:18 PM, David Miller <davem@davemloft.net> wrote:
> From: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Thu, 27 Aug 2015 15:47:55 -0700
>
>> On Thu, Aug 27, 2015 at 3:42 PM, David Miller <davem@davemloft.net> wrote:
>>> If you fix it properly, by making every qdisc capable of being ->init()'d
>>> without explicit parameters, it will be the best behavior overall.
>>
>> The problem is ->init() is not even called when setting it as default,
>> since setting a default qdisc doesn't need to create a qdisc. This is
>> why the flag has to be in ops->flags rather than qdisc->flags.
>
> Just sounds like another shortcoming of how default qdiscs are handled,

It has to, due to its definition. I don't see any other way except we change
the meaning of the default qdisc.

> rather than a reason to not fix things properly.

If you mean the crash, my patch can fix it too by simply rejecting the
invalid and buggy case like HTB, even though I do have local patches
to fix it directly.

Like I said, the more important question is not if it crashes, it is if we
should reject invalid case as early as possible or just wait for an error
to happen later.

Re: [Patch net-next 4/5] net_sched: forbid setting default qdisc to inappropriate ones

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Thu, 27 Aug 2015 18:49:09 -0700

> On Thu, Aug 27, 2015 at 4:18 PM, David Miller <davem@davemloft.net> wrote:
>> From: Cong Wang <xiyou.wangcong@gmail.com>
>> Date: Thu, 27 Aug 2015 15:47:55 -0700
>>
>>> On Thu, Aug 27, 2015 at 3:42 PM, David Miller <davem@davemloft.net> wrote:
>>>> If you fix it properly, by making every qdisc capable of being ->init()'d
>>>> without explicit parameters, it will be the best behavior overall.
>>>
>>> The problem is ->init() is not even called when setting it as default,
>>> since setting a default qdisc doesn't need to create a qdisc. This is
>>> why the flag has to be in ops->flags rather than qdisc->flags.
>>
>> Just sounds like another shortcoming of how default qdiscs are handled,
> 
> It has to, due to its definition. I don't see any other way except we change
> the meaning of the default qdisc.

We are talking past eachother.

If a default qdisc like HTB is choosen, we invoke the ->init() function
and we change the HTB ->init() function to do something reasonable
if a NULL set of configuration attributes is given.  ie. make HTB use
some defaults.

Please explain to me why this won't fix the problem.

Re: [Patch net-next 4/5] net_sched: forbid setting default qdisc to inappropriate ones

[Jamal Hadi Salim]

On 08/28/15 00:23, David Miller wrote:

> If a default qdisc like HTB is choosen, we invoke the ->init() function
> and we change the HTB ->init() function to do something reasonable
> if a NULL set of configuration attributes is given.  ie. make HTB use
> some defaults.
>

That may work. Or to reduce ambiguity introduce qdisc->set_default().

cheers,
jamal

Re: [Patch net-next 4/5] net_sched: forbid setting default qdisc to inappropriate ones

[Cong Wang]

On Thu, Aug 27, 2015 at 9:23 PM, David Miller <davem@davemloft.net> wrote:
> From: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Thu, 27 Aug 2015 18:49:09 -0700
>
>> On Thu, Aug 27, 2015 at 4:18 PM, David Miller <davem@davemloft.net> wrote:
>>> From: Cong Wang <xiyou.wangcong@gmail.com>
>>> Date: Thu, 27 Aug 2015 15:47:55 -0700
>>>
>>>> On Thu, Aug 27, 2015 at 3:42 PM, David Miller <davem@davemloft.net> wrote:
>>>>> If you fix it properly, by making every qdisc capable of being ->init()'d
>>>>> without explicit parameters, it will be the best behavior overall.
>>>>
>>>> The problem is ->init() is not even called when setting it as default,
>>>> since setting a default qdisc doesn't need to create a qdisc. This is
>>>> why the flag has to be in ops->flags rather than qdisc->flags.
>>>
>>> Just sounds like another shortcoming of how default qdiscs are handled,
>>
>> It has to, due to its definition. I don't see any other way except we change
>> the meaning of the default qdisc.
>
> We are talking past eachother.
>
> If a default qdisc like HTB is choosen, we invoke the ->init() function
> and we change the HTB ->init() function to do something reasonable
> if a NULL set of configuration attributes is given.  ie. make HTB use
> some defaults.
>
> Please explain to me why this won't fix the problem.

It does, and it is exactly what my local patch does.

The problem is setting HTB as default is already invalid from the
beginning, so HTB->init() is not supposed be called since we can
reject it earlier, and this is my whole point.

If HTB is not a good example, as using HTB as default might
make some sense, please try ingress qdisc, no error at _any_ time,
and apparently defaulting to ingress is totally non-sense.

Re: [Patch net-next 4/5] net_sched: forbid setting default qdisc to inappropriate ones

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Fri, 28 Aug 2015 14:39:45 -0700

> If HTB is not a good example, as using HTB as default might
> make some sense, please try ingress qdisc, no error at _any_ time,
> and apparently defaulting to ingress is totally non-sense.

I agree that ingress should have some special flag that prevents
it from being an egress qdisc.

But HTB definitely should be allowed.

Re: [Patch net-next 4/5] net_sched: forbid setting default qdisc to inappropriate ones

[Jamal Hadi Salim]

On 08/28/15 19:20, David Miller wrote:

> But HTB definitely should be allowed.

Problem with most non-work conserving schedulers is what the meaning
of default resources means; example, for HTB:
What is the default bandwidth you allocate to a class of users?

cheers,
jamal

Re: [Patch net-next 4/5] net_sched: forbid setting default qdisc to inappropriate ones

[Cong Wang]

On Sun, Aug 30, 2015 at 12:07 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 08/28/15 19:20, David Miller wrote:
>
>> But HTB definitely should be allowed.
>
>
> Problem with most non-work conserving schedulers is what the meaning
> of default resources means; example, for HTB:
> What is the default bandwidth you allocate to a class of users?
>

Exactly, that is why it has to need at least one parameter for bandwidth,
while default qdisc requires no parameter.

Re: [Patch net-next 4/5] net_sched: forbid setting default qdisc to inappropriate ones

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Tue, 1 Sep 2015 23:05:45 -0700

> On Sun, Aug 30, 2015 at 12:07 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> On 08/28/15 19:20, David Miller wrote:
>>
>>> But HTB definitely should be allowed.
>>
>>
>> Problem with most non-work conserving schedulers is what the meaning
>> of default resources means; example, for HTB:
>> What is the default bandwidth you allocate to a class of users?
> 
> Exactly, that is why it has to need at least one parameter for bandwidth,
> while default qdisc requires no parameter.

Ok I'm convinced.

Re: [Patch net-next 4/5] net_sched: forbid setting default qdisc to inappropriate ones

[Cong Wang]

On Tue, Sep 1, 2015 at 11:19 PM, David Miller <davem@davemloft.net> wrote:
> From: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Tue, 1 Sep 2015 23:05:45 -0700
>
>> On Sun, Aug 30, 2015 at 12:07 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>> On 08/28/15 19:20, David Miller wrote:
>>>
>>>> But HTB definitely should be allowed.
>>>
>>>
>>> Problem with most non-work conserving schedulers is what the meaning
>>> of default resources means; example, for HTB:
>>> What is the default bandwidth you allocate to a class of users?
>>
>> Exactly, that is why it has to need at least one parameter for bandwidth,
>> while default qdisc requires no parameter.
>
> Ok I'm convinced.

Ok, I will update the changelog to clarify this and resend.

Thanks.

[Patch net-next 5/5] net_sched: move ingress flag into qdisc ops

[Cong Wang]

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/sch_generic.h |  2 +-
 net/sched/sch_api.c       | 11 +++++------
 net/sched/sch_ingress.c   |  1 +
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 2e6748e..a02bf04 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -48,7 +48,6 @@ struct Qdisc {
 	int 			(*enqueue)(struct sk_buff *skb, struct Qdisc *dev);
 	struct sk_buff *	(*dequeue)(struct Qdisc *dev);
 	unsigned int		flags;
-#define TCQ_F_INGRESS		2
 #define TCQ_F_CAN_BYPASS	4
 #define TCQ_F_ONETXQUEUE	0x10 /* dequeue_skb() can assume all skbs are for
 				      * q->dev_queue : It can test
@@ -183,6 +182,7 @@ struct Qdisc_ops {
 #define QDISC_F_MQ		2
 #define QDISC_F_FIFO		4
 #define QDISC_F_DEFAULTABLE	8
+#define QDISC_F_INGRESS		16
 	unsigned int		flags;
 
 	int 			(*enqueue)(struct sk_buff *, struct Qdisc *);
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index e501e9d..7f285fa8 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -281,7 +281,7 @@ static struct Qdisc *qdisc_match_from_root(struct Qdisc *root, u32 handle)
 
 void qdisc_list_add(struct Qdisc *q)
 {
-	if ((q->parent != TC_H_ROOT) && !(q->flags & TCQ_F_INGRESS)) {
+	if ((q->parent != TC_H_ROOT) && !(q->ops->flags & QDISC_F_INGRESS)) {
 		struct Qdisc *root = qdisc_dev(q)->qdisc;
 
 		WARN_ON_ONCE(root == &noop_qdisc);
@@ -292,7 +292,7 @@ EXPORT_SYMBOL(qdisc_list_add);
 
 void qdisc_list_del(struct Qdisc *q)
 {
-	if ((q->parent != TC_H_ROOT) && !(q->flags & TCQ_F_INGRESS))
+	if ((q->parent != TC_H_ROOT) && !(q->ops->flags & QDISC_F_INGRESS))
 		list_del(&q->list);
 }
 EXPORT_SYMBOL(qdisc_list_del);
@@ -812,8 +812,8 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
 
 		ingress = 0;
 		num_q = dev->num_tx_queues;
-		if ((q && q->flags & TCQ_F_INGRESS) ||
-		    (new && new->flags & TCQ_F_INGRESS)) {
+		if ((q && q->ops->flags & QDISC_F_INGRESS) ||
+		    (new && new->ops->flags & QDISC_F_INGRESS)) {
 			num_q = 1;
 			ingress = 1;
 			if (!dev_ingress_queue(dev))
@@ -937,7 +937,6 @@ qdisc_create(struct net_device *dev, struct netdev_queue *dev_queue,
 	sch->parent = parent;
 
 	if (handle == TC_H_INGRESS) {
-		sch->flags |= TCQ_F_INGRESS;
 		handle = TC_H_MAKE(TC_H_INGRESS, 0);
 		lockdep_set_class(qdisc_lock(sch), &qdisc_rx_lock);
 	} else {
@@ -982,7 +981,7 @@ qdisc_create(struct net_device *dev, struct netdev_queue *dev_queue,
 				goto err_out4;
 
 			if ((sch->parent != TC_H_ROOT) &&
-			    !(sch->flags & TCQ_F_INGRESS) &&
+			    !(sch->ops->flags & QDISC_F_INGRESS) &&
 			    (!p || !(p->ops->flags & QDISC_F_MQ)))
 				root_lock = qdisc_root_sleeping_lock(sch);
 			else
diff --git a/net/sched/sch_ingress.c b/net/sched/sch_ingress.c
index e7c648f..2e30b39 100644
--- a/net/sched/sch_ingress.c
+++ b/net/sched/sch_ingress.c
@@ -92,6 +92,7 @@ static const struct Qdisc_class_ops ingress_class_ops = {
 static struct Qdisc_ops ingress_qdisc_ops __read_mostly = {
 	.cl_ops		=	&ingress_class_ops,
 	.id		=	"ingress",
+	.flags		=	QDISC_F_INGRESS,
 	.init		=	ingress_init,
 	.destroy	=	ingress_destroy,
 	.dump		=	ingress_dump,
-- 
1.8.3.1