[PATCH net 00/13] cls_u32 cleanups and fixes.

[PATCH net 00/13] cls_u32 cleanups and fixes.

[Al Viro]

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

A series of net/sched/cls_u32.c cleanups and fixes:
	1) fix hnode refcounting.  Refcounting for tc_u_hnode is broken;
it's not hard to trigger oopsen (including one inside an interrupt handler,
with resulting panic) as well as memory corruption.  Definitely -stable
fodder.
	2) mark root hnode explicitly.  Consistent errors on attempts to
delete root hnodes.  Less serious than 1/13.
	3) disallow linking to root hnode.  Prohibit creating links to
root hnodes; not critical (nothing actually breaks if we do allow those),
but gets rid of surprising cases.
	4) make sure that divisor is a power of 2.  Missing validation -
divisor is documented as power of 2, but that's not actually enforced.
Results are moderately bogus (i.e. the kernel doesn't break), but rather
surprising.

Those are fixes, or at least can be argued to be such.  The rest are pure
cleanups:
	5) get rid of unused argument of u32_destroy_key()
	6) get rid of tc_u_knode ->tp
	7) get rid of tc_u_common ->rcu
Eliminate some unused fields.
	8) clean tc_u_common hashtable.
Hash lookups are best done with minimum of calculations per chain
element - comparing the field in each candidate with f(key) where
f() is actually a pure function is not nice, especially when
compiler doesn't see f() as such...  Better calculate f(key) once,
especially since we need its value to choose the hash chain in
the first place.
	9) pass tc_u_common to u32_set_parms() instead of tc_u_hnode
	10) the tp_c argument of u32_set_parms() is always tp->data
	11) get rid of hnode ->tp_c and tp_c argument of u32_set_parms()
Massage that ends with getting rid of a redundant field.
	12) keep track of knodes count in tc_u_common
	13) simplify the hell out u32_delete() emptiness check
Checking if a filter needs to be killed after u32_delete() can be
done much easier - the test is equivalent to "filter doesn't
have ->data shared with anyone else and it has no knodes left
in it" and keeping track of the number of knodes is trivial.

[PATCH net 01/13] net: sched: cls_u32: fix hnode refcounting

[Al Viro]

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

cls_u32.c misuses refcounts for struct tc_u_hnode - it counts references via
->hlist and via ->tp_root together.  u32_destroy() drops the former and, in
case when there had been links, leaves the sucker on the list.  As the result,
there's nothing to protect it from getting freed once links are dropped.
That also makes the "is it busy" check incapable of catching the root hnode -
it *is* busy (there's a reference from tp), but we don't see it as something
separate.  "Is it our root?" check partially covers that, but the problem
exists for others' roots as well.

AFAICS, the minimal fix preserving the existing behaviour (where it doesn't
include oopsen, that is) would be this:
        * count tp->root and tp_c->hlist as separate references.  I.e.
have u32_init() set refcount to 2, not 1.
	* in u32_destroy() we always drop the former; in u32_destroy_hnode() -
the latter.

	That way we have *all* references contributing to refcount.  List
removal happens in u32_destroy_hnode() (called only when ->refcnt is 1)
an in u32_destroy() in case of tc_u_common going away, along with everything
reachable from it.  IOW, that way we know that u32_destroy_key() won't
free something still on the list (or pointed to by someone's ->root).

Cc: stable@vger.kernel.org
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 net/sched/cls_u32.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index f218ccf1e2d9..b2c3406a2cf2 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -398,6 +398,7 @@ static int u32_init(struct tcf_proto *tp)
 	rcu_assign_pointer(tp_c->hlist, root_ht);
 	root_ht->tp_c = tp_c;
 
+	root_ht->refcnt++;
 	rcu_assign_pointer(tp->root, root_ht);
 	tp->data = tp_c;
 	return 0;
@@ -610,7 +611,7 @@ static int u32_destroy_hnode(struct tcf_proto *tp, struct tc_u_hnode *ht,
 	struct tc_u_hnode __rcu **hn;
 	struct tc_u_hnode *phn;
 
-	WARN_ON(ht->refcnt);
+	WARN_ON(--ht->refcnt);
 
 	u32_clear_hnode(tp, ht, extack);
 
@@ -649,7 +650,7 @@ static void u32_destroy(struct tcf_proto *tp, struct netlink_ext_ack *extack)
 
 	WARN_ON(root_ht == NULL);
 
-	if (root_ht && --root_ht->refcnt == 0)
+	if (root_ht && --root_ht->refcnt == 1)
 		u32_destroy_hnode(tp, root_ht, extack);
 
 	if (--tp_c->refcnt == 0) {
@@ -698,7 +699,6 @@ static int u32_delete(struct tcf_proto *tp, void *arg, bool *last,
 	}
 
 	if (ht->refcnt == 1) {
-		ht->refcnt--;
 		u32_destroy_hnode(tp, ht, extack);
 	} else {
 		NL_SET_ERR_MSG_MOD(extack, "Can not delete in-use filter");
@@ -708,11 +708,11 @@ static int u32_delete(struct tcf_proto *tp, void *arg, bool *last,
 out:
 	*last = true;
 	if (root_ht) {
-		if (root_ht->refcnt > 1) {
+		if (root_ht->refcnt > 2) {
 			*last = false;
 			goto ret;
 		}
-		if (root_ht->refcnt == 1) {
+		if (root_ht->refcnt == 2) {
 			if (!ht_empty(root_ht)) {
 				*last = false;
 				goto ret;
-- 
2.11.0

Re: [PATCH net 01/13] net: sched: cls_u32: fix hnode refcounting

[Jamal Hadi Salim]

On 2018-09-08 9:31 p.m., Al Viro wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> cls_u32.c misuses refcounts for struct tc_u_hnode - it counts references via
> ->hlist and via ->tp_root together.  u32_destroy() drops the former and, in
> case when there had been links, leaves the sucker on the list.  As the result,
> there's nothing to protect it from getting freed once links are dropped.
> That also makes the "is it busy" check incapable of catching the root hnode -
> it *is* busy (there's a reference from tp), but we don't see it as something
> separate.  "Is it our root?" check partially covers that, but the problem
> exists for others' roots as well.
> 
> AFAICS, the minimal fix preserving the existing behaviour (where it doesn't
> include oopsen, that is) would be this:
>          * count tp->root and tp_c->hlist as separate references.  I.e.
> have u32_init() set refcount to 2, not 1.
> 	* in u32_destroy() we always drop the former; in u32_destroy_hnode() -
> the latter.
> 
> 	That way we have *all* references contributing to refcount.  List
> removal happens in u32_destroy_hnode() (called only when ->refcnt is 1)
> an in u32_destroy() in case of tc_u_common going away, along with everything
> reachable from it.  IOW, that way we know that u32_destroy_key() won't
> free something still on the list (or pointed to by someone's ->root).
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

per Cong's earlier remark - the reproducer going in the changelog
would be nice to show i.e this you posted earlier, otherwise:

Tested-by: Jamal Hadi Salim <jhs@mojatatu.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

Reminder:

Re: [PATCH net 01/13] net: sched: cls_u32: fix hnode refcounting

[Jamal Hadi Salim]

On 2018-09-08 9:31 p.m., Al Viro wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> cls_u32.c misuses refcounts for struct tc_u_hnode - it counts references via
> ->hlist and via ->tp_root together.  u32_destroy() drops the former and, in
> case when there had been links, leaves the sucker on the list.  As the result,
> there's nothing to protect it from getting freed once links are dropped.
> That also makes the "is it busy" check incapable of catching the root hnode -
> it *is* busy (there's a reference from tp), but we don't see it as something
> separate.  "Is it our root?" check partially covers that, but the problem
> exists for others' roots as well.
> 
> AFAICS, the minimal fix preserving the existing behaviour (where it doesn't
> include oopsen, that is) would be this:
>          * count tp->root and tp_c->hlist as separate references.  I.e.
> have u32_init() set refcount to 2, not 1.
> 	* in u32_destroy() we always drop the former; in u32_destroy_hnode() -
> the latter.
> 
> 	That way we have *all* references contributing to refcount.  List
> removal happens in u32_destroy_hnode() (called only when ->refcnt is 1)
> an in u32_destroy() in case of tc_u_common going away, along with everything
> reachable from it.  IOW, that way we know that u32_destroy_key() won't
> free something still on the list (or pointed to by someone's ->root).
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

per Cong's earlier remark - the reproducer going in the changelog
would be nice to show i.e this you posted earlier, otherwise:

Tested-by: Jamal Hadi Salim <jhs@mojatatu.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

Reminder:
--
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
tc filter add dev eth0 parent ffff: protocol ip prio 100 handle 1:0:11 
u32 ht 1: link 801: offset at 0 mask 0f00 shift 6 plus 0 eat match ip 
protocol 6 ff
tc filter delete dev eth0 parent ffff: protocol ip prio 200
tc filter change dev eth0 parent ffff: protocol ip prio 100 handle 
1:0:11 u32 ht 1: link 0: offset at 0 mask 0f00 shift 6 plus 0 eat match 
ip protocol 6 ff
tc filter delete dev eth0 parent ffff: protocol ip prio 100
---

cheers,
jamal

[PATCH net 02/13] net: sched: cls_u32: mark root hnode explicitly

[Al Viro]

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

... 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>
---
 net/sched/cls_u32.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

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;
 	}
-- 
2.11.0

Re: [PATCH net 02/13] net: sched: cls_u32: mark root hnode explicitly

[Jamal Hadi Salim]

On 2018-09-08 9:31 p.m., Al Viro wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

[PATCH net 03/13] net: sched: cls_u32: disallow linking to root hnode

[Al Viro]

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>
---
 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 c4782aa808c7..72459b09d910 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

Re: [PATCH net 03/13] net: sched: cls_u32: disallow linking to root hnode

[Jamal Hadi Salim]

On 2018-09-08 9:31 p.m., Al Viro wrote:
> 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>

Tested-by: Jamal Hadi Salim <jhs@mojatatu.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

[PATCH net 04/13] net: sched: cls_u32: make sure that divisor is a power of 2

[Al Viro]

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

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 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 72459b09d910..d9923d474b65 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 04/13] net: sched: cls_u32: make sure that divisor is a power of 2

[Jamal Hadi Salim]

On 2018-09-08 9:31 p.m., Al Viro wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
>  > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Test is by hacking user space iproute2 to allow
sending a divisor > 255

Tested-by: Jamal Hadi Salim <jhs@mojatatu.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

[PATCH net 05/13] net: sched: cls_u32: get rid of unused argument of u32_destroy_key()

[Al Viro]

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

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 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 d9923d474b65..d11862823911 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -406,8 +406,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);
 
@@ -441,7 +440,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();
 }
 
@@ -458,7 +457,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();
 }
 
@@ -601,7 +600,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

Re: [PATCH net 05/13] net: sched: cls_u32: get rid of unused argument of u32_destroy_key()

[Jamal Hadi Salim]

On 2018-09-08 9:31 p.m., Al Viro wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

[PATCH net 06/13] net: sched: cls_u32: get rid of tc_u_knode ->tp

[Al Viro]

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

not used anymore

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 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 d11862823911..281ac954511c 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

Re: [PATCH net 06/13] net: sched: cls_u32: get rid of tc_u_knode ->tp

[Jamal Hadi Salim]

On 2018-09-08 9:31 p.m., Al Viro wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> not used anymore
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

[PATCH net 07/13] net: sched: cls_u32: get rid of tc_u_common ->rcu

[Al Viro]

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

unused

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 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 281ac954511c..1bfbfcab7260 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

Re: [PATCH net 07/13] net: sched: cls_u32: get rid of tc_u_common ->rcu

[Jamal Hadi Salim]

On 2018-09-08 9:31 p.m., Al Viro wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> unused
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

[PATCH net 08/13] net: sched: cls_u32: clean tc_u_common hashtable

[Al Viro]

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>
---
 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 1bfbfcab7260..af05113c1212 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

Re: [PATCH net 08/13] net: sched: cls_u32: clean tc_u_common hashtable

[Jamal Hadi Salim]

On 2018-09-08 9:31 p.m., Al Viro wrote:
> 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>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

[PATCH net 09/13] net: sched: cls_u32: pass tc_u_common to u32_set_parms() instead of tc_u_hnode

[Al Viro]

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>
---
 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 af05113c1212..221ce532b241 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

Re: [PATCH net 09/13] net: sched: cls_u32: pass tc_u_common to u32_set_parms() instead of tc_u_hnode

[Jamal Hadi Salim]

On 2018-09-08 9:31 p.m., Al Viro wrote:
> 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>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

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

[Al Viro]

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>
---
 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 221ce532b241..12757e3ec8d8 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

Re: [PATCH net 10/13] net: sched: cls_u32: the tp_c argument of u32_set_parms() is always tp->data

[Jamal Hadi Salim]

On 2018-09-08 9:31 p.m., Al Viro wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 


Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

[PATCH net 11/13] net: sched: cls_u32: get rid of hnode ->tp_c and tp_c argument of u32_set_parms()

[Al Viro]

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

the latter is redundant, the former - never read...

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

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 12757e3ec8d8..f6bb3885598d 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;
 
 	root_ht->refcnt++;
 	rcu_assign_pointer(tp->root, root_ht);
@@ -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,8 +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,
-			    extack);
+	err = u32_set_parms(net, tp, base, n, tb, tca[TCA_RATE], ovr, extack);
 	if (err == 0) {
 		struct tc_u_knode __rcu **ins;
 		struct tc_u_knode *pins;
-- 
2.11.0

Re: [PATCH net 11/13] net: sched: cls_u32: get rid of hnode ->tp_c and tp_c argument of u32_set_parms()

[Jamal Hadi Salim]

On 2018-09-08 9:31 p.m., Al Viro wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

[PATCH net 12/13] net: sched: cls_u32: keep track of knodes count in tc_u_common

[Al Viro]

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

allows to simplify u32_delete() considerably

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 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 f6bb3885598d..86cbe4f5800e 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,
@@ -453,6 +454,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);
@@ -463,6 +465,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);
@@ -577,6 +580,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;
 
@@ -584,6 +588,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);
@@ -1140,6 +1145,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

Re: [PATCH net 12/13] net: sched: cls_u32: keep track of knodes count in tc_u_common

[Jamal Hadi Salim]

On 2018-09-08 9:31 p.m., Al Viro wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> allows to simplify u32_delete() considerably
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

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

[Al Viro]

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>
---
 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 86cbe4f5800e..ef786ec24843 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -628,17 +628,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;
@@ -676,13 +665,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 > 2) {
-			*last = false;
-			goto ret;
-		}
-		if (root_ht->refcnt == 2) {
-			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 13/13] net: sched: cls_u32: simplify the hell out u32_delete() emptiness check

[Jamal Hadi Salim]

On 2018-09-08 9:31 p.m., Al Viro wrote:
> 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>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

Re: [PATCH net 00/13] cls_u32 cleanups and fixes.

[Jamal Hadi Salim]

Since you have the momentum here: i noticed something
unusual while i was trying to craft a test that would
vet some of your changes. This has nothing to do with
your changes, same happens on my stock debian laptop
with kernel:
4.17.0-0.bpo.3-amd64 #1 SMP Debian 4.17.17-1~bpo9+1 (2018-08-27)

Looking at git - possibly introduced around the time u32
lockless was being introduced and maybe even earlier
than that. Unfortunately i dont have time to dig
further.

To reproduce what i am referring to, here's a setup:

$tc filter add dev eth0 parent ffff: protocol ip prio 102 u32 \
classid 1:2 match ip src 192.168.8.0/8
$tc filter replace dev eth0 parent ffff: protocol ip prio 102 \
handle 800:0:800 u32 classid 1:2 match ip src 1.1.0.0/24

u32_change() code path should have allowed changing of the
keynode.

cheers,
jamal

Re: [PATCH net 00/13] cls_u32 cleanups and fixes.

[Al Viro]

On Sun, Sep 09, 2018 at 08:58:50AM -0400, Jamal Hadi Salim wrote:
> 
> Since you have the momentum here: i noticed something
> unusual while i was trying to craft a test that would
> vet some of your changes. This has nothing to do with
> your changes, same happens on my stock debian laptop
> with kernel:
> 4.17.0-0.bpo.3-amd64 #1 SMP Debian 4.17.17-1~bpo9+1 (2018-08-27)
> 
> Looking at git - possibly introduced around the time u32
> lockless was being introduced and maybe even earlier
> than that.

It's always been that way, actually - before that point the old
knode simply got reused, which excluded any chance of changing
n->sel.

> Unfortunately i dont have time to dig
> further.
> 
> To reproduce what i am referring to, here's a setup:
> 
> $tc filter add dev eth0 parent ffff: protocol ip prio 102 u32 \
> classid 1:2 match ip src 192.168.8.0/8
> $tc filter replace dev eth0 parent ffff: protocol ip prio 102 \
> handle 800:0:800 u32 classid 1:2 match ip src 1.1.0.0/24
> 
> u32_change() code path should have allowed changing of the
> keynode.

Umm...  Interesting - TCA_U32_SEL is not the only thing that
gets ignored there; TCA_U32_MARK gets the same treatment.
And then there's a lovely question what to do with n->pf -
it's an array of n->sel.nkeys counters, and apparently we
want (at least in common cases) to avoid resetting those.

*If* we declare that ->nkeys mismatch means failure, it's
all relatively easy to implement.  Alternatively, we could
declare that selector change means resetting the stats.
Preferences?

Re: [PATCH net 00/13] cls_u32 cleanups and fixes.

[Al Viro]

On Sun, Sep 09, 2018 at 03:15:38PM +0100, Al Viro wrote:

> Umm...  Interesting - TCA_U32_SEL is not the only thing that
> gets ignored there; TCA_U32_MARK gets the same treatment.
> And then there's a lovely question what to do with n->pf -
> it's an array of n->sel.nkeys counters, and apparently we
> want (at least in common cases) to avoid resetting those.
> 
> *If* we declare that ->nkeys mismatch means failure, it's
> all relatively easy to implement.  Alternatively, we could
> declare that selector change means resetting the stats.
> Preferences?

BTW, shouldn't we issue u32_clear_hw_hnode() every time
we destroy an hnode?  It's done on u32_delete(), it's
done (for root ht) on u32_destroy(), but it's not done
for any other hnodes when you remove the entire (not shared)
filter.  Looks fishy...

Offloaded u32 classifier tables WAS (Re: [PATCH net 00/13] cls_u32 cleanups and fixes.

[Jamal Hadi Salim]

On 2018-09-09 11:48 a.m., Al Viro wrote:

>
> BTW, shouldn't we issue u32_clear_hw_hnode() every time
> we destroy an hnode?  It's done on u32_delete(), it's
> done (for root ht) on u32_destroy(), but it's not done
> for any other hnodes when you remove the entire (not shared)
> filter.  Looks fishy...
> 

What you are saying makes sense, but that doesnt seem
like a new thing.
All hardware offload examples I have seen use root tables
only[1]. So I am not sure if they use any other tables
although the intel hardware at least seems very capable.
Look at ixgbe_main.c for example. Theres an explicit assumption
that root is always 0x800 but oresence of

+Cc some of the NIC vendor folks..



cheers,
jamal

[1]
Here's a script posted by someone at Intel(Sridhar?) a while back
that adds 2 filters, one with skip-sw and the other with skip-hw
flag.

---
    # add ingress qdisc
    tc qdisc add dev p4p1 ingress

    # enable hw tc offload.
    ethtool -K p4p1 hw-tc-offload on

    # add u32 filter with skip-sw flag.
    tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
       handle 800:0:1 u32 ht 800: flowid 800:1 \
       skip-sw \
       match ip src 192.168.1.0/24 \
       action drop

    # add u32 filter with skip-hw flag.
    tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
       handle 800:0:2 u32 ht 800: flowid 800:2 \
       skip-hw \
       match ip src 192.168.2.0/24 \
       action drop
----

Re: Offloaded u32 classifier tables WAS (Re: [PATCH net 00/13] cls_u32 cleanups and fixes.

[Jamal Hadi Salim]

On 2018-09-10 8:25 a.m., Jamal Hadi Salim wrote:
> On 2018-09-09 11:48 a.m., Al Viro wrote:
> 
>>
>> BTW, shouldn't we issue u32_clear_hw_hnode() every time
>> we destroy an hnode?  It's done on u32_delete(), it's
>> done (for root ht) on u32_destroy(), but it's not done
>> for any other hnodes when you remove the entire (not shared)
>> filter.  Looks fishy...
>>
> 
> What you are saying makes sense, but that doesnt seem
> like a new thing.
> All hardware offload examples I have seen use root tables
> only[1]. So I am not sure if they use any other tables
> although the intel hardware at least seems very capable.
> Look at ixgbe_main.c for example. Theres an explicit assumption
> that root is always 0x800 but oresence of
>

"..but presence of other tables can be handled"

cheers,
jamal

Re: [PATCH net 00/13] cls_u32 cleanups and fixes.

[Jamal Hadi Salim]

On 2018-09-09 10:15 a.m., Al Viro wrote:

[..]

> Umm...  Interesting - TCA_U32_SEL is not the only thing that
> gets ignored there; TCA_U32_MARK gets the same treatment.
> And then there's a lovely question what to do with n->pf -
> it's an array of n->sel.nkeys counters, and apparently we
> want (at least in common cases) to avoid resetting those.

> *If* we declare that ->nkeys mismatch means failure, it's
> all relatively easy to implement.  Alternatively, we could
> declare that selector change means resetting the stats.
> Preferences?

I am now conflicted. I have sample scripts which showed
that at one point that worked. All of them seem to be
indicating the nkeys and stats never changed. So that
could be a starting point; however, if a policy changes
the match tuples then it is no longer the same rule imo.

Note: you can change the "actions" - of which the most
primitive is "set classid x:y" without changing what is
being matched. i.e changing the classid in that example
would work.

cheers,
jamal

Re: [PATCH net 00/13] cls_u32 cleanups and fixes.

[David Miller]

From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Sun,  9 Sep 2018 02:31:19 +0100

> A series of net/sched/cls_u32.c cleanups and fixes:

Al, let's separate the serious bug fixes into a separate series
targetting net.  Then we can do all of the cleanups and
simplifications in net-next once I merge net into there.

Thank you.