[PATCH net-next 00/11] net: sched: cls_u32 Various improvements

[PATCH net-next 00/11] net: sched: cls_u32 Various improvements

[Jamal Hadi Salim]

From: Jamal Hadi Salim <jhs@mojatatu.com>

Various improvements from Al.

Al Viro (11):
  net: sched: cls_u32: disallow linking to root hnode
  net: sched: cls_u32: make sure that divisor is a power of 2
  net: sched: cls_u32: get rid of unused argument of u32_destroy_key()
  net: sched: cls_u32: get rid of tc_u_knode ->tp
  net: sched: cls_u32: get rid of tc_u_common ->rcu
  net: sched: cls_u32: clean tc_u_common hashtable
  net: sched: cls_u32: pass tc_u_common to u32_set_parms() instead of
    tc_u_hnode
  net: sched: cls_u32: the tp_c argument of u32_set_parms() is always
    tp->data
  net: sched: cls_u32: keep track of knodes count in tc_u_common
  net: sched: cls_u32: simplify the hell out u32_delete() emptiness
    check
  net: sched: cls_u32: get rid of tp_c

 net/sched/cls_u32.c | 117 ++++++++++++++++------------------------------------
 1 file changed, 35 insertions(+), 82 deletions(-)

-- 
2.11.0

[PATCH net-next 01/11] net: sched: cls_u32: disallow linking to root hnode

[Jamal Hadi Salim]

From: Al Viro <viro@zeniv.linux.org.uk>

Operation makes no sense.  Nothing will actually break if we do so
(depth limit in u32_classify() will prevent infinite loops), but
according to maintainers it's best prohibited outright.

NOTE: doing so guarantees that u32_destroy() will trigger the call
of u32_destroy_hnode(); we might want to make that unconditional.

Test:
tc qdisc add dev eth0 ingress
tc filter add dev eth0 parent ffff: protocol ip prio 100 u32 \
link 800: offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6 ff
should fail with
Error: cls_u32: Not linking to root node

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/cls_u32.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 622f4657da94..3357331a80a2 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -797,6 +797,10 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp,
 				NL_SET_ERR_MSG_MOD(extack, "Link hash table not found");
 				return -EINVAL;
 			}
+			if (ht_down->is_root) {
+				NL_SET_ERR_MSG_MOD(extack, "Not linking to root node");
+				return -EINVAL;
+			}
 			ht_down->refcnt++;
 		}
 
-- 
2.11.0

[PATCH net-next 02/11] net: sched: cls_u32: make sure that divisor is a power of 2

[Jamal Hadi Salim]

From: Al Viro <viro@zeniv.linux.org.uk>

Tested by modifying iproute2 to to allow
sending a divisor > 255 

Tested-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/cls_u32.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 3357331a80a2..ce55eea448a0 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -994,7 +994,11 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 	if (tb[TCA_U32_DIVISOR]) {
 		unsigned int divisor = nla_get_u32(tb[TCA_U32_DIVISOR]);
 
-		if (--divisor > 0x100) {
+		if (!is_power_of_2(divisor)) {
+			NL_SET_ERR_MSG_MOD(extack, "Divisor is not a power of 2");
+			return -EINVAL;
+		}
+		if (divisor-- > 0x100) {
 			NL_SET_ERR_MSG_MOD(extack, "Exceeded maximum 256 hash buckets");
 			return -EINVAL;
 		}
-- 
2.11.0

Re: [PATCH net-next 02/11] net: sched: cls_u32: make sure that divisor is a power of 2

[Sergei Shtylyov]

Hello!

On 07.10.2018 19:38, Jamal Hadi Salim wrote:

> From: Al Viro <viro@zeniv.linux.org.uk>
>
> Tested by modifying iproute2 to to allow

    One "to" is enough, no? :-)

> sending a divisor > 255
>
> Tested-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
[...]

MBR, Sergei

Re: [PATCH net-next 02/11] net: sched: cls_u32: make sure that divisor is a power of 2

[Jamal Hadi Salim]

On 2018-10-08 4:46 a.m., Sergei Shtylyov wrote:
> Hello!
> 
> On 07.10.2018 19:38, Jamal Hadi Salim wrote:
> 
>> From: Al Viro <viro@zeniv.linux.org.uk>
>>
>> Tested by modifying iproute2 to to allow
> 
>     One "to" is enough, no? :-)
> 

Will fix in updated version.

cheers,
jamal

[PATCH net-next 03/11] net: sched: cls_u32: get rid of unused argument of u32_destroy_key()

[Jamal Hadi Salim]

From: Al Viro <viro@zeniv.linux.org.uk>

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/cls_u32.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index ce55eea448a0..ef0f2e6ec422 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -405,8 +405,7 @@ static int u32_init(struct tcf_proto *tp)
 	return 0;
 }
 
-static int u32_destroy_key(struct tcf_proto *tp, struct tc_u_knode *n,
-			   bool free_pf)
+static int u32_destroy_key(struct tc_u_knode *n, bool free_pf)
 {
 	struct tc_u_hnode *ht = rtnl_dereference(n->ht_down);
 
@@ -440,7 +439,7 @@ static void u32_delete_key_work(struct work_struct *work)
 					      struct tc_u_knode,
 					      rwork);
 	rtnl_lock();
-	u32_destroy_key(key->tp, key, false);
+	u32_destroy_key(key, false);
 	rtnl_unlock();
 }
 
@@ -457,7 +456,7 @@ static void u32_delete_key_freepf_work(struct work_struct *work)
 					      struct tc_u_knode,
 					      rwork);
 	rtnl_lock();
-	u32_destroy_key(key->tp, key, true);
+	u32_destroy_key(key, true);
 	rtnl_unlock();
 }
 
@@ -600,7 +599,7 @@ static void u32_clear_hnode(struct tcf_proto *tp, struct tc_u_hnode *ht,
 			if (tcf_exts_get_net(&n->exts))
 				tcf_queue_work(&n->rwork, u32_delete_key_freepf_work);
 			else
-				u32_destroy_key(n->tp, n, true);
+				u32_destroy_key(n, true);
 		}
 	}
 }
@@ -971,13 +970,13 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 				    tca[TCA_RATE], ovr, extack);
 
 		if (err) {
-			u32_destroy_key(tp, new, false);
+			u32_destroy_key(new, false);
 			return err;
 		}
 
 		err = u32_replace_hw_knode(tp, new, flags, extack);
 		if (err) {
-			u32_destroy_key(tp, new, false);
+			u32_destroy_key(new, false);
 			return err;
 		}
 
-- 
2.11.0

[PATCH net-next 04/11] net: sched: cls_u32: get rid of tc_u_knode ->tp

[Jamal Hadi Salim]

From: Al Viro <viro@zeniv.linux.org.uk>

not used anymore

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/cls_u32.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index ef0f2e6ec422..810c49ac1bbe 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -68,7 +68,6 @@ struct tc_u_knode {
 	u32			mask;
 	u32 __percpu		*pcpu_success;
 #endif
-	struct tcf_proto	*tp;
 	struct rcu_work		rwork;
 	/* The 'sel' field MUST be the last field in structure to allow for
 	 * tc_u32_keys allocated at end of structure.
@@ -896,7 +895,6 @@ static struct tc_u_knode *u32_init_knode(struct tcf_proto *tp,
 	/* Similarly success statistics must be moved as pointers */
 	new->pcpu_success = n->pcpu_success;
 #endif
-	new->tp = tp;
 	memcpy(&new->sel, s, sizeof(*s) + s->nkeys*sizeof(struct tc_u32_key));
 
 	if (tcf_exts_init(&new->exts, TCA_U32_ACT, TCA_U32_POLICE)) {
@@ -1112,7 +1110,6 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 	n->handle = handle;
 	n->fshift = s->hmask ? ffs(ntohl(s->hmask)) - 1 : 0;
 	n->flags = flags;
-	n->tp = tp;
 
 	err = tcf_exts_init(&n->exts, TCA_U32_ACT, TCA_U32_POLICE);
 	if (err < 0)
-- 
2.11.0

[PATCH net-next 05/11] net: sched: cls_u32: get rid of tc_u_common ->rcu

[Jamal Hadi Salim]

From: Al Viro <viro@zeniv.linux.org.uk>

unused

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/cls_u32.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 810c49ac1bbe..c378168f4562 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -98,7 +98,6 @@ struct tc_u_common {
 	int			refcnt;
 	struct idr		handle_idr;
 	struct hlist_node	hnode;
-	struct rcu_head		rcu;
 };
 
 static inline unsigned int u32_hash_fold(__be32 key,
-- 
2.11.0

[PATCH net-next 06/11] net: sched: cls_u32: clean tc_u_common hashtable

[Jamal Hadi Salim]

From: Al Viro <viro@zeniv.linux.org.uk>

* calculate key *once*, not for each hash chain element
* let tc_u_hash() return the pointer to chain head rather than index -
callers are cleaner that way.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/cls_u32.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index c378168f4562..3f6fba831c57 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -343,19 +343,16 @@ static void *tc_u_common_ptr(const struct tcf_proto *tp)
 		return block->q;
 }
 
-static unsigned int tc_u_hash(const struct tcf_proto *tp)
+static struct hlist_head *tc_u_hash(void *key)
 {
-	return hash_ptr(tc_u_common_ptr(tp), U32_HASH_SHIFT);
+	return tc_u_common_hash + hash_ptr(key, U32_HASH_SHIFT);
 }
 
-static struct tc_u_common *tc_u_common_find(const struct tcf_proto *tp)
+static struct tc_u_common *tc_u_common_find(void *key)
 {
 	struct tc_u_common *tc;
-	unsigned int h;
-
-	h = tc_u_hash(tp);
-	hlist_for_each_entry(tc, &tc_u_common_hash[h], hnode) {
-		if (tc->ptr == tc_u_common_ptr(tp))
+	hlist_for_each_entry(tc, tc_u_hash(key), hnode) {
+		if (tc->ptr == key)
 			return tc;
 	}
 	return NULL;
@@ -364,10 +361,8 @@ static struct tc_u_common *tc_u_common_find(const struct tcf_proto *tp)
 static int u32_init(struct tcf_proto *tp)
 {
 	struct tc_u_hnode *root_ht;
-	struct tc_u_common *tp_c;
-	unsigned int h;
-
-	tp_c = tc_u_common_find(tp);
+	void *key = tc_u_common_ptr(tp);
+	struct tc_u_common *tp_c = tc_u_common_find(key);
 
 	root_ht = kzalloc(sizeof(*root_ht), GFP_KERNEL);
 	if (root_ht == NULL)
@@ -385,12 +380,11 @@ static int u32_init(struct tcf_proto *tp)
 			kfree(root_ht);
 			return -ENOBUFS;
 		}
-		tp_c->ptr = tc_u_common_ptr(tp);
+		tp_c->ptr = key;
 		INIT_HLIST_NODE(&tp_c->hnode);
 		idr_init(&tp_c->handle_idr);
 
-		h = tc_u_hash(tp);
-		hlist_add_head(&tp_c->hnode, &tc_u_common_hash[h]);
+		hlist_add_head(&tp_c->hnode, tc_u_hash(key));
 	}
 
 	tp_c->refcnt++;
-- 
2.11.0

[PATCH net-next 07/11] net: sched: cls_u32: pass tc_u_common to u32_set_parms() instead of tc_u_hnode

[Jamal Hadi Salim]

From: Al Viro <viro@zeniv.linux.org.uk>

the only thing we used ht for was ht->tp_c and callers can get that
without going through ->tp_c at all; start with lifting that into
the callers, next commits will massage those, eventually removing
->tp_c altogether.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/cls_u32.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 3f6fba831c57..53f34f8cde8b 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -761,7 +761,7 @@ static const struct nla_policy u32_policy[TCA_U32_MAX + 1] = {
 };
 
 static int u32_set_parms(struct net *net, struct tcf_proto *tp,
-			 unsigned long base, struct tc_u_hnode *ht,
+			 unsigned long base, struct tc_u_common *tp_c,
 			 struct tc_u_knode *n, struct nlattr **tb,
 			 struct nlattr *est, bool ovr,
 			 struct netlink_ext_ack *extack)
@@ -782,7 +782,7 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp,
 		}
 
 		if (handle) {
-			ht_down = u32_lookup_ht(ht->tp_c, handle);
+			ht_down = u32_lookup_ht(tp_c, handle);
 
 			if (!ht_down) {
 				NL_SET_ERR_MSG_MOD(extack, "Link hash table not found");
@@ -957,7 +957,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 			return -ENOMEM;
 
 		err = u32_set_parms(net, tp, base,
-				    rtnl_dereference(n->ht_up), new, tb,
+				    rtnl_dereference(n->ht_up)->tp_c, new, tb,
 				    tca[TCA_RATE], ovr, extack);
 
 		if (err) {
@@ -1124,7 +1124,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 	}
 #endif
 
-	err = u32_set_parms(net, tp, base, ht, n, tb, tca[TCA_RATE], ovr,
+	err = u32_set_parms(net, tp, base, ht->tp_c, n, tb, tca[TCA_RATE], ovr,
 			    extack);
 	if (err == 0) {
 		struct tc_u_knode __rcu **ins;
-- 
2.11.0

[PATCH net-next 08/11] net: sched: cls_u32: the tp_c argument of u32_set_parms() is always tp->data

[Jamal Hadi Salim]

From: Al Viro <viro@zeniv.linux.org.uk>

It must be tc_u_common associated with that tp (i.e. tp->data).
Proof:
	* both ->ht_up and ->tp_c are assign-once
	* ->tp_c of anything inserted into tp_c->hlist is tp_c
	* hnodes never get reinserted into the lists or moved
between those, so anything found by u32_lookup_ht(tp->data, ...)
will have ->tp_c equal to tp->data.
	* tp->root->tp_c == tp->data.
	* ->ht_up of anything inserted into hnode->ht[...] is
equal to hnode.
	* knodes never get reinserted into hash chains or moved
between those, so anything returned by u32_lookup_key(ht, ...)
will have ->ht_up equal to ht.
	* any knode returned by u32_get(tp, ...) will have ->ht_up->tp_c
point to tp->data

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/cls_u32.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 53f34f8cde8b..3ed2c9866b36 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -956,8 +956,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 		if (!new)
 			return -ENOMEM;
 
-		err = u32_set_parms(net, tp, base,
-				    rtnl_dereference(n->ht_up)->tp_c, new, tb,
+		err = u32_set_parms(net, tp, base, tp_c, new, tb,
 				    tca[TCA_RATE], ovr, extack);
 
 		if (err) {
@@ -1124,7 +1123,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 	}
 #endif
 
-	err = u32_set_parms(net, tp, base, ht->tp_c, n, tb, tca[TCA_RATE], ovr,
+	err = u32_set_parms(net, tp, base, tp_c, n, tb, tca[TCA_RATE], ovr,
 			    extack);
 	if (err == 0) {
 		struct tc_u_knode __rcu **ins;
-- 
2.11.0

[PATCH net-next 09/11] net: sched: cls_u32: get rid of tp_c

[Jamal Hadi Salim]

From: Al Viro <viro@zeniv.linux.org.uk>

Both hnode ->tp_c and tp_c argument of u32_set_parms()
the latter is redundant, the former - never read...

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/cls_u32.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 3ed2c9866b36..3d4c360f9b0c 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -79,7 +79,6 @@ struct tc_u_hnode {
 	struct tc_u_hnode __rcu	*next;
 	u32			handle;
 	u32			prio;
-	struct tc_u_common	*tp_c;
 	int			refcnt;
 	unsigned int		divisor;
 	struct idr		handle_idr;
@@ -390,7 +389,6 @@ static int u32_init(struct tcf_proto *tp)
 	tp_c->refcnt++;
 	RCU_INIT_POINTER(root_ht->next, tp_c->hlist);
 	rcu_assign_pointer(tp_c->hlist, root_ht);
-	root_ht->tp_c = tp_c;
 
 	rcu_assign_pointer(tp->root, root_ht);
 	tp->data = tp_c;
@@ -761,7 +759,7 @@ static const struct nla_policy u32_policy[TCA_U32_MAX + 1] = {
 };
 
 static int u32_set_parms(struct net *net, struct tcf_proto *tp,
-			 unsigned long base, struct tc_u_common *tp_c,
+			 unsigned long base,
 			 struct tc_u_knode *n, struct nlattr **tb,
 			 struct nlattr *est, bool ovr,
 			 struct netlink_ext_ack *extack)
@@ -782,7 +780,7 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp,
 		}
 
 		if (handle) {
-			ht_down = u32_lookup_ht(tp_c, handle);
+			ht_down = u32_lookup_ht(tp->data, handle);
 
 			if (!ht_down) {
 				NL_SET_ERR_MSG_MOD(extack, "Link hash table not found");
@@ -956,7 +954,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 		if (!new)
 			return -ENOMEM;
 
-		err = u32_set_parms(net, tp, base, tp_c, new, tb,
+		err = u32_set_parms(net, tp, base, new, tb,
 				    tca[TCA_RATE], ovr, extack);
 
 		if (err) {
@@ -1012,7 +1010,6 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 				return err;
 			}
 		}
-		ht->tp_c = tp_c;
 		ht->refcnt = 1;
 		ht->divisor = divisor;
 		ht->handle = handle;
@@ -1123,7 +1120,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 	}
 #endif
 
-	err = u32_set_parms(net, tp, base, tp_c, n, tb, tca[TCA_RATE], ovr,
+	err = u32_set_parms(net, tp, base, n, tb, tca[TCA_RATE], ovr,
 			    extack);
 	if (err == 0) {
 		struct tc_u_knode __rcu **ins;
-- 
2.11.0

[PATCH net-next 10/11] net: sched: cls_u32: keep track of knodes count in tc_u_common

[Jamal Hadi Salim]

From: Al Viro <viro@zeniv.linux.org.uk>

allows to simplify u32_delete() considerably

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/cls_u32.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 3d4c360f9b0c..61593bee08db 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -97,6 +97,7 @@ struct tc_u_common {
 	int			refcnt;
 	struct idr		handle_idr;
 	struct hlist_node	hnode;
+	long			knodes;
 };
 
 static inline unsigned int u32_hash_fold(__be32 key,
@@ -452,6 +453,7 @@ static void u32_delete_key_freepf_work(struct work_struct *work)
 
 static int u32_delete_key(struct tcf_proto *tp, struct tc_u_knode *key)
 {
+	struct tc_u_common *tp_c = tp->data;
 	struct tc_u_knode __rcu **kp;
 	struct tc_u_knode *pkp;
 	struct tc_u_hnode *ht = rtnl_dereference(key->ht_up);
@@ -462,6 +464,7 @@ static int u32_delete_key(struct tcf_proto *tp, struct tc_u_knode *key)
 		     kp = &pkp->next, pkp = rtnl_dereference(*kp)) {
 			if (pkp == key) {
 				RCU_INIT_POINTER(*kp, key->next);
+				tp_c->knodes--;
 
 				tcf_unbind_filter(tp, &key->res);
 				idr_remove(&ht->handle_idr, key->handle);
@@ -576,6 +579,7 @@ static int u32_replace_hw_knode(struct tcf_proto *tp, struct tc_u_knode *n,
 static void u32_clear_hnode(struct tcf_proto *tp, struct tc_u_hnode *ht,
 			    struct netlink_ext_ack *extack)
 {
+	struct tc_u_common *tp_c = tp->data;
 	struct tc_u_knode *n;
 	unsigned int h;
 
@@ -583,6 +587,7 @@ static void u32_clear_hnode(struct tcf_proto *tp, struct tc_u_hnode *ht,
 		while ((n = rtnl_dereference(ht->ht[h])) != NULL) {
 			RCU_INIT_POINTER(ht->ht[h],
 					 rtnl_dereference(n->next));
+			tp_c->knodes--;
 			tcf_unbind_filter(tp, &n->res);
 			u32_remove_hw_knode(tp, n, extack);
 			idr_remove(&ht->handle_idr, n->handle);
@@ -1141,6 +1146,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 
 		RCU_INIT_POINTER(n->next, pins);
 		rcu_assign_pointer(*ins, n);
+		tp_c->knodes++;
 		*arg = n;
 		return 0;
 	}
-- 
2.11.0

[PATCH net-next 11/11] net: sched: cls_u32: simplify the hell out u32_delete() emptiness check

[Jamal Hadi Salim]

From: Al Viro <viro@zeniv.linux.org.uk>

Now that we have the knode count, we can instantly check if
any hnodes are non-empty.  And that kills the check for extra
references to root hnode - those could happen only if there was
a knode to carry such a link.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/cls_u32.c | 48 +-----------------------------------------------
 1 file changed, 1 insertion(+), 47 deletions(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 61593bee08db..ac79a40a0392 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -627,17 +627,6 @@ static int u32_destroy_hnode(struct tcf_proto *tp, struct tc_u_hnode *ht,
 	return -ENOENT;
 }
 
-static bool ht_empty(struct tc_u_hnode *ht)
-{
-	unsigned int h;
-
-	for (h = 0; h <= ht->divisor; h++)
-		if (rcu_access_pointer(ht->ht[h]))
-			return false;
-
-	return true;
-}
-
 static void u32_destroy(struct tcf_proto *tp, struct netlink_ext_ack *extack)
 {
 	struct tc_u_common *tp_c = tp->data;
@@ -675,13 +664,9 @@ static int u32_delete(struct tcf_proto *tp, void *arg, bool *last,
 		      struct netlink_ext_ack *extack)
 {
 	struct tc_u_hnode *ht = arg;
-	struct tc_u_hnode *root_ht = rtnl_dereference(tp->root);
 	struct tc_u_common *tp_c = tp->data;
 	int ret = 0;
 
-	if (ht == NULL)
-		goto out;
-
 	if (TC_U32_KEY(ht->handle)) {
 		u32_remove_hw_knode(tp, (struct tc_u_knode *)ht, extack);
 		ret = u32_delete_key(tp, (struct tc_u_knode *)ht);
@@ -702,38 +687,7 @@ static int u32_delete(struct tcf_proto *tp, void *arg, bool *last,
 	}
 
 out:
-	*last = true;
-	if (root_ht) {
-		if (root_ht->refcnt > 1) {
-			*last = false;
-			goto ret;
-		}
-		if (root_ht->refcnt == 1) {
-			if (!ht_empty(root_ht)) {
-				*last = false;
-				goto ret;
-			}
-		}
-	}
-
-	if (tp_c->refcnt > 1) {
-		*last = false;
-		goto ret;
-	}
-
-	if (tp_c->refcnt == 1) {
-		struct tc_u_hnode *ht;
-
-		for (ht = rtnl_dereference(tp_c->hlist);
-		     ht;
-		     ht = rtnl_dereference(ht->next))
-			if (!ht_empty(ht)) {
-				*last = false;
-				break;
-			}
-	}
-
-ret:
+	*last = tp_c->refcnt == 1 && tp_c->knodes == 0;
 	return ret;
 }
 
-- 
2.11.0

Re: [PATCH net-next 00/11] net: sched: cls_u32 Various improvements

[David Miller]

From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Sun,  7 Oct 2018 12:38:00 -0400

> Various improvements from Al.

Please submit changes that actually are compile tested:

  CC [M]  net/sched/cls_u32.o
net/sched/cls_u32.c: In function ‘u32_delete’:
net/sched/cls_u32.c:674:6: error: ‘root_ht’ undeclared (first use in this function); did you mean ‘root_user’?
  if (root_ht == ht) {
      ^~~~~~~
      root_user
net/sched/cls_u32.c:674:6: note: each undeclared identifier is reported only once for each function it appears in
net/sched/cls_u32.c: In function ‘u32_set_parms’:
net/sched/cls_u32.c:746:15: error: ‘struct tc_u_hnode’ has no member named ‘is_root’
    if (ht_down->is_root) {
               ^~

Re: [PATCH net-next 00/11] net: sched: cls_u32 Various improvements

[Al Viro]

On Sun, Oct 07, 2018 at 09:25:01PM -0700, David Miller wrote:
> From: Jamal Hadi Salim <jhs@mojatatu.com>
> Date: Sun,  7 Oct 2018 12:38:00 -0400
> 
> > Various improvements from Al.
> 
> Please submit changes that actually are compile tested:
> 
>   CC [M]  net/sched/cls_u32.o
> net/sched/cls_u32.c: In function ‘u32_delete’:
> net/sched/cls_u32.c:674:6: error: ‘root_ht’ undeclared (first use in this function); did you mean ‘root_user’?
>   if (root_ht == ht) {
>       ^~~~~~~
>       root_user
> net/sched/cls_u32.c:674:6: note: each undeclared identifier is reported only once for each function it appears in
> net/sched/cls_u32.c: In function ‘u32_set_parms’:
> net/sched/cls_u32.c:746:15: error: ‘struct tc_u_hnode’ has no member named ‘is_root’
>     if (ht_down->is_root) {
>                ^~

Er...  Both are due to missing in the very beginning of the series (well, on
top of "net: sched: cls_u32: fix hnode refcounting") commit

Author: Al Viro <viro@zeniv.linux.org.uk>
Date:   Mon Sep 3 14:39:02 2018 -0400

    net: sched: cls_u32: mark root hnode explicitly
    
    ... and produce consistent error on attempt to delete such.
    Existing check in u32_delete() is inconsistent - after
    
    tc qdisc add dev eth0 ingress
    tc filter add dev eth0 parent ffff: protocol ip prio 100 handle 1: u32 divisor 1
    tc filter add dev eth0 parent ffff: protocol ip prio 200 handle 2: u32 divisor 1
    
    both
    
    tc filter delete dev eth0 parent ffff: protocol ip prio 100 handle 801: u32
    
    and
    
    tc filter delete dev eth0 parent ffff: protocol ip prio 100 handle 800: u32
    
    will fail (at least with refcounting fixes), but the former will complain
    about an attempt to remove a busy table, while the latter will recognize
    it as root and yield "Not allowed to delete root node" instead.
    
    The problem with the existing check is that several tcf_proto instances might
    share the same tp->data and handle-to-hnode lookup will be the same for all
    of them.  So comparing an hnode to be deleted with tp->root won't catch the
    case when one tp is used to try deleting the root of another.  Solution is
    trivial - mark the root hnodes explicitly upon allocation and check for that.
    
    Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index b2c3406a2cf2..c4782aa808c7 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -84,6 +84,7 @@ struct tc_u_hnode {
 	int			refcnt;
 	unsigned int		divisor;
 	struct idr		handle_idr;
+	bool			is_root;
 	struct rcu_head		rcu;
 	u32			flags;
 	/* The 'ht' field MUST be the last field in structure to allow for
@@ -377,6 +378,7 @@ static int u32_init(struct tcf_proto *tp)
 	root_ht->refcnt++;
 	root_ht->handle = tp_c ? gen_new_htid(tp_c, root_ht) : 0x80000000;
 	root_ht->prio = tp->prio;
+	root_ht->is_root = true;
 	idr_init(&root_ht->handle_idr);
 
 	if (tp_c == NULL) {
@@ -693,7 +695,7 @@ static int u32_delete(struct tcf_proto *tp, void *arg, bool *last,
 		goto out;
 	}
 
-	if (root_ht == ht) {
+	if (ht->is_root) {
 		NL_SET_ERR_MSG_MOD(extack, "Not allowed to delete root node");
 		return -EINVAL;
 	}

Re: [PATCH net-next 00/11] net: sched: cls_u32 Various improvements

[David Miller]

From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Mon, 8 Oct 2018 06:45:15 +0100

> Er...  Both are due to missing in the very beginning of the series (well, on
> top of "net: sched: cls_u32: fix hnode refcounting") commit

All dependencies like this must be explicitly stated.

And in such situations you actually should wait for the dependency to
get into 'net', eventually get merged into 'net-next', and then you
can submit the stuff that depends upon it.

Not the way this was done.

Re: [PATCH net-next 00/11] net: sched: cls_u32 Various improvements

[Al Viro]

On Sun, Oct 07, 2018 at 10:55:52PM -0700, David Miller wrote:
> From: Al Viro <viro@ZenIV.linux.org.uk>
> Date: Mon, 8 Oct 2018 06:45:15 +0100
> 
> > Er...  Both are due to missing in the very beginning of the series (well, on
> > top of "net: sched: cls_u32: fix hnode refcounting") commit
> 
> All dependencies like this must be explicitly stated.
>
> And in such situations you actually should wait for the dependency to
> get into 'net', eventually get merged into 'net-next', and then you
> can submit the stuff that depends upon it.
> 
> Not the way this was done.

Point (and this commit is simply missing - it's not that it went into
net).  FWIW, this was simply "this is what the breakage is caused by",
not an attempt to amend the submission or anything like that...

Re: [PATCH net-next 00/11] net: sched: cls_u32 Various improvements

[Jamal Hadi Salim]

On 2018-10-08 2:11 a.m., Al Viro wrote:
> On Sun, Oct 07, 2018 at 10:55:52PM -0700, David Miller wrote:
>> From: Al Viro <viro@ZenIV.linux.org.uk>
>> Date: Mon, 8 Oct 2018 06:45:15 +0100
>>
>>> Er...  Both are due to missing in the very beginning of the series (well, on
>>> top of "net: sched: cls_u32: fix hnode refcounting") commit
>>
>> All dependencies like this must be explicitly stated.
>>
>> And in such situations you actually should wait for the dependency to
>> get into 'net', eventually get merged into 'net-next', and then you
>> can submit the stuff that depends upon it.
>>
>> Not the way this was done.
> 
> Point (and this commit is simply missing - it's not that it went into
> net).  FWIW, this was simply "this is what the breakage is caused by",
> not an attempt to amend the submission or anything like that...
> 

My bad. I was wondering why it compiled for me. There is no dependency
on the single patch that went to net. I will submit an updated version.

cheers,
jamal