patch: annoying u32 double listing

patch: annoying u32 double listing

[jamal]

[-- Attachment #1: Type: text/plain, Size: 1786 bytes --]


bug is around since 2.1.x; never cared about chasing it until now
because it is affecting someone i know.
Dave, please apply. I will prepare a 2.4.x version.

To see the problem:

tc qdisc add dev eth0 ingress
#add two filters at different prio levels
tc filter add dev eth0 parent ffff: protocol ip prio 6 \
u32 match ip src 10.0.0.210/32 flowid 1:16
#
tc filter add dev eth0 parent ffff: protocol ip prio 7 \
u32 match ip src 10.0.0.144/32 flowid 1:15 
#now list them..
root@jdev:/# tc -s filter show parent ffff: dev eth0 

filter protocol ip pref 6 u32 
filter protocol ip pref 6 u32 fh 801: ht divisor 1 
filter protocol ip pref 6 u32 fh 801::800 order 2048 key ht 801 bkt 0
flowid 1:15 
  match 0a000090/ffffffff at 12
filter protocol ip pref 6 u32 fh 800: ht divisor 1 
filter protocol ip pref 6 u32 fh 800::800 order 2048 key ht 800 bkt 0
flowid 1:16 
  match 0a0000d2/ffffffff at 12
filter protocol ip pref 7 u32 
filter protocol ip pref 7 u32 fh 801: ht divisor 1 
filter protocol ip pref 7 u32 fh 801::800 order 2048 key ht 801 bkt 0
flowid 1:15 
  match 0a000090/ffffffff at 12
filter protocol ip pref 7 u32 fh 800: ht divisor 1 
filter protocol ip pref 7 u32 fh 800::800 order 2048 key ht 800 bkt 0
flowid 1:16 
  match 0a0000d2/ffffffff at 12

each filter is listed in each priority. So if you had 5 rules in 5 prios
(which is where sherlock started) you see 25 outputs.

AFTER FIX: 
---------

root@jdev:/usr/src# tc -s filter show parent ffff: dev eth0
filter protocol ip pref 6 u32
filter protocol ip pref 6 u32 fh 800::800 order 2048 key ht 800 bkt 0
flowid 1:16  
  match 0a0000d2/ffffffff at 12 
  filter protocol ip pref 7 u32
  filter protocol ip pref 7 u32 fh 801::800 order 2048 key ht 801 bkt 0
flowid 1:15  
    match 0a000090/ffffffff at 12 


cheers,
jamal

[-- Attachment #2: u32multi_p --]
[-- Type: text/plain, Size: 1448 bytes --]

--- 2611-rc3+bk3/net/sched/cls_api.c	2005/02/06 16:28:26	1.1
+++ 2611-rc3+bk3/net/sched/cls_api.c	2005/02/06 16:41:47
@@ -326,6 +326,7 @@
 {
 	struct tcmsg *tcm;
 	struct nlmsghdr  *nlh;
+	int ret;
 	unsigned char	 *b = skb->tail;
 
 	nlh = NLMSG_PUT(skb, pid, seq, event, sizeof(*tcm));
@@ -339,8 +340,11 @@
 	tcm->tcm_handle = fh;
 	if (RTM_DELTFILTER != event) {
 		tcm->tcm_handle = 0;
-		if (tp->ops->dump && tp->ops->dump(tp, fh, skb, tcm) < 0)
-			goto rtattr_failure;
+		if (tp->ops->dump) {
+			ret = tp->ops->dump(tp, fh, skb, tcm);
+			if (ret < 0)
+				goto rtattr_failure;
+		}
 	}
 	nlh->nlmsg_len = skb->tail - b;
 	return skb->len;
@@ -348,6 +352,8 @@
 nlmsg_failure:
 rtattr_failure:
 	skb_trim(skb, b - skb->data);
+	if (ret == -ENOENT)
+		return skb->len;
 	return -1;
 }
 
--- 2611-rc3+bk3/net/sched/cls_u32.c	2005/02/06 16:30:44	1.1
+++ 2611-rc3+bk3/net/sched/cls_u32.c	2005/02/06 16:33:13
@@ -76,6 +76,7 @@
 	char                     indev[IFNAMSIZ];
 #endif
 	u8			fshift;
+	u32			prio;
 	struct tcf_result	res;
 	struct tc_u_hnode	*ht_down;
 #ifdef CONFIG_CLS_U32_PERF
@@ -641,6 +642,7 @@
 	memcpy(&n->sel, s, sizeof(*s) + s->nkeys*sizeof(struct tc_u32_key));
 	n->ht_up = ht;
 	n->handle = handle;
+	n->prio = tp->prio;
 {
 	u8 i = 0;
 	u32 mask = s->hmask;
@@ -736,6 +738,9 @@
 	if (n == NULL)
 		return skb->len;
 
+	if (n->prio != tp->prio)
+		return -ENOENT;
+
 	t->tcm_handle = n->handle;
 
 	rta = (struct rtattr*)b;

2.4.29 version: Re: patch: annoying u32 double listing

[jamal]

[-- Attachment #1: Type: text/plain, Size: 235 bytes --]

On Sun, 2005-02-06 at 14:49, jamal wrote:
> bug is around since 2.1.x; never cared about chasing it until now
> because it is affecting someone i know.
> Dave, please apply. I will prepare a 2.4.x version.
> 

Attached.

cheers,
jamal

[-- Attachment #2: p2429 --]
[-- Type: text/plain, Size: 1509 bytes --]

--- cls_api.c.orig	2004-02-18 08:36:32.000000000 -0500
+++ cls_api.c	2005-02-06 12:21:32.976531360 -0500
@@ -289,6 +289,7 @@
 {
 	struct tcmsg *tcm;
 	struct nlmsghdr  *nlh;
+	int ret;
 	unsigned char	 *b = skb->tail;
 
 	nlh = NLMSG_PUT(skb, pid, seq, event, sizeof(*tcm));
@@ -300,14 +301,19 @@
 	tcm->tcm_handle = 0;
 	tcm->tcm_info = TC_H_MAKE(tp->prio, tp->protocol);
 	RTA_PUT(skb, TCA_KIND, IFNAMSIZ, tp->ops->kind);
-	if (tp->ops->dump && tp->ops->dump(tp, fh, skb, tcm) < 0)
-		goto rtattr_failure;
+	if (tp->ops->dump) {
+		ret = tp->ops->dump(tp, fh, skb, tcm);
+		if (ret < 0)
+			goto rtattr_failure;
+	}
 	nlh->nlmsg_len = skb->tail - b;
 	return skb->len;
 
 nlmsg_failure:
 rtattr_failure:
 	skb_trim(skb, b - skb->data);
+	if (ret == -ENOENT)
+		return skb->len;
 	return -1;
 }
 
--- cls_u32.c.orig	2004-11-17 06:54:22.000000000 -0500
+++ cls_u32.c	2005-02-06 12:25:31.000346240 -0500
@@ -61,6 +61,7 @@
 #ifdef CONFIG_NET_CLS_POLICE
 	struct tcf_police	*police;
 #endif
+	u32			prio;
 	struct tcf_result	res;
 	struct tc_u_hnode	*ht_down;
 	struct tc_u32_sel	sel;
@@ -577,6 +578,7 @@
 	memcpy(&n->sel, s, sizeof(*s) + s->nkeys*sizeof(struct tc_u32_key));
 	n->ht_up = ht;
 	n->handle = handle;
+	n->prio = tp->prio;
 	err = u32_set_parms(tp->q, base, ht, n, tb, tca[TCA_RATE-1]);
 	if (err == 0) {
 		struct tc_u_knode **ins;
@@ -639,6 +641,9 @@
 	if (n == NULL)
 		return skb->len;
 
+	if (n->prio != tp->prio)
+		return -ENOENT;
+
 	t->tcm_handle = n->handle;
 
 	rta = (struct rtattr*)b;

Re: patch: annoying u32 double listing

[Thomas Graf]

* jamal <1107719343.1055.22.camel@jzny.localdomain> 2005-02-06 14:49
> 
> bug is around since 2.1.x; never cared about chasing it until now
> because it is affecting someone i know.

Good catch, I've put a workaround for this into libnl a while back. I
wasn't sure whether it's supposed to be a feature or not. ;-> Another
quite anoying detail is the fact that cbq uses the same handle for
both the root qdisc and root class which makes it impossible to uniquely
assign filters attached to it, i.e. listing filters based on
parent == XX: && dev == %DEV will result in the filters being listed
for both the qdisc and the class. I haven't found a fix that wouldn't
break many scripts so if you have a good idea, go ahead. ;->

Re: patch: annoying u32 double listing

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 1574 bytes --]

jamal wrote:

>bug is around since 2.1.x; never cared about chasing it until now
>because it is affecting someone i know.
>Dave, please apply. I will prepare a 2.4.x version.
>
The patch is wrong. The "divisor"-lines are missing in the output with
your patch and it only hides the real error. ->walk is supposed to walk
all filters of the given priority/protocol, but u32 walks all filters.
This patch fixes it. Output with new patch:

filter protocol ip pref 6 u32
filter protocol ip pref 6 u32 fh 800: ht divisor 1
filter protocol ip pref 6 u32 fh 800::800 order 2048 key ht 800 bkt 0 
flowid 1:16  (rule hit 0 success 0)
  match 0a0000d2/ffffffff at 12 (success 0 )
filter protocol ip pref 7 u32
filter protocol ip pref 7 u32 fh 801: ht divisor 1
filter protocol ip pref 7 u32 fh 801::800 order 2048 key ht 801 bkt 0 
flowid 1:15  (rule hit 0 success 0)
  match 0a000090/ffffffff at 12 (success 0 )

Signed-off-by: Patrick McHardy <kaber@trash.net>

BTW: u32_walk seems to have another bug, it uses tcf_walker->count and
tcf_walker->skip for skipping over both hnodes and knodes and doesn't
skip hnodes properly at all. I'll have a look at this now.

Regards
Patrick

>AFTER FIX: 
>---------
>
>root@jdev:/usr/src# tc -s filter show parent ffff: dev eth0
>filter protocol ip pref 6 u32
>filter protocol ip pref 6 u32 fh 800::800 order 2048 key ht 800 bkt 0
>flowid 1:16  
>  match 0a0000d2/ffffffff at 12 
>  filter protocol ip pref 7 u32
>  filter protocol ip pref 7 u32 fh 801::800 order 2048 key ht 801 bkt 0
>flowid 1:15  
>    match 0a000090/ffffffff at 12 
>
>  
>

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 754 bytes --]

===== net/sched/cls_u32.c 1.25 vs edited =====
--- 1.25/net/sched/cls_u32.c	2005-01-11 20:25:16 +01:00
+++ edited/net/sched/cls_u32.c	2005-02-06 22:20:33 +01:00
@@ -91,6 +91,7 @@
 {
 	struct tc_u_hnode	*next;
 	u32			handle;
+	u32			prio;
 	struct tc_u_common	*tp_c;
 	int			refcnt;
 	unsigned		divisor;
@@ -323,6 +324,7 @@
 	root_ht->divisor = 0;
 	root_ht->refcnt++;
 	root_ht->handle = tp_c ? gen_new_htid(tp_c) : 0x80000000;
+	root_ht->prio = tp->prio;
 
 	if (tp_c == NULL) {
 		tp_c = kmalloc(sizeof(*tp_c), GFP_KERNEL);
@@ -703,6 +705,8 @@
 		return;
 
 	for (ht = tp_c->hlist; ht; ht = ht->next) {
+		if (ht->prio != tp->prio)
+			continue;
 		if (arg->count >= arg->skip) {
 			if (arg->fn(tp, (unsigned long)ht, arg) < 0) {
 				arg->stop = 1;

Re: patch: annoying u32 double listing

[jamal]

On Sun, 2005-02-06 at 16:29, Thomas Graf wrote:

> assign filters attached to it, i.e. listing filters based on
> parent == XX: && dev == %DEV will result in the filters being listed
> for both the qdisc and the class. I haven't found a fix that wouldn't
> break many scripts so if you have a good idea, go ahead. ;->

I cant say ive run into this one - mostly i attach filters to root.
Example script?

BTW, cycles allow me to work on the eaction stuff today. So far
ive come to the conclusion that whats really needed to simplify
writting some actions is to just provide generic methods they
can use without them necessarily knowing details. I hope to test
later on, maybe write a simple action and its user apce piece, and post
patch.

cheers,
jamal

Re: patch: annoying u32 double listing

[Patrick McHardy]

Thomas Graf wrote:

>Another
>quite anoying detail is the fact that cbq uses the same handle for
>both the root qdisc and root class which makes it impossible to uniquely
>assign filters attached to it, i.e. listing filters based on
>parent == XX: && dev == %DEV will result in the filters being listed
>for both the qdisc and the class.
>
CBQ and HFSC only support filters attached to classes for exactly this
reason :) Shouldn't make any difference from a users perspective.

Regards
Patrick

Re: patch: annoying u32 double listing

[Patrick McHardy]

Patrick McHardy wrote:

> The patch is wrong. The "divisor"-lines are missing in the output with
> your patch and it only hides the real error. ->walk is supposed to walk
> all filters of the given priority/protocol, but u32 walks all filters.
> This patch fixes it. Output with new patch:

The patch also applies to 2.4.

Regards
Patrick

>
>------------------------------------------------------------------------
>
>===== net/sched/cls_u32.c 1.25 vs edited =====
>--- 1.25/net/sched/cls_u32.c	2005-01-11 20:25:16 +01:00
>+++ edited/net/sched/cls_u32.c	2005-02-06 22:20:33 +01:00
>@@ -91,6 +91,7 @@
> {
> 	struct tc_u_hnode	*next;
> 	u32			handle;
>+	u32			prio;
> 	struct tc_u_common	*tp_c;
> 	int			refcnt;
> 	unsigned		divisor;
>@@ -323,6 +324,7 @@
> 	root_ht->divisor = 0;
> 	root_ht->refcnt++;
> 	root_ht->handle = tp_c ? gen_new_htid(tp_c) : 0x80000000;
>+	root_ht->prio = tp->prio;
> 
> 	if (tp_c == NULL) {
> 		tp_c = kmalloc(sizeof(*tp_c), GFP_KERNEL);
>@@ -703,6 +705,8 @@
> 		return;
> 
> 	for (ht = tp_c->hlist; ht; ht = ht->next) {
>+		if (ht->prio != tp->prio)
>+			continue;
> 		if (arg->count >= arg->skip) {
> 			if (arg->fn(tp, (unsigned long)ht, arg) < 0) {
> 				arg->stop = 1;
>  
>

Re: patch: annoying u32 double listing

[jamal]

On Sun, 2005-02-06 at 16:39, Patrick McHardy wrote:
> jamal wrote:
> 
> >bug is around since 2.1.x; never cared about chasing it until now
> >because it is affecting someone i know.
> >Dave, please apply. I will prepare a 2.4.x version.
> >
> The patch is wrong. The "divisor"-lines are missing in the output with

I should have caught that output missing divisor ;->
Dave please apply Patricks version. Should probably apply cleanly on
2.4.x as well.

cheers,
jamal

Re: patch: annoying u32 double listing

[Thomas Graf]

* Patrick McHardy <4206906A.2060501@trash.net> 2005-02-06 22:47
> Thomas Graf wrote:
> 
> >Another
> >quite anoying detail is the fact that cbq uses the same handle for
> >both the root qdisc and root class which makes it impossible to uniquely
> >assign filters attached to it, i.e. listing filters based on
> >parent == XX: && dev == %DEV will result in the filters being listed
> >for both the qdisc and the class.
> >
> CBQ and HFSC only support filters attached to classes for exactly this
> reason :) Shouldn't make any difference from a users perspective.

I'm aware of it and that's not the problem. The problem is that if the
user requests the filters attached to parent XX: he can't tell whether
the qdisc or class parent is meant without checking for hardcoded
qdisc kinds which makes it pretty much impossible to generate
qdisc/class/filter trees in a generic way. It's just something anoying
not a bug.

Re: patch: annoying u32 double listing

[Thomas Graf]

* jamal <1107726133.1055.34.camel@jzny.localdomain> 2005-02-06 16:42
> On Sun, 2005-02-06 at 16:29, Thomas Graf wrote:
> 
> > assign filters attached to it, i.e. listing filters based on
> > parent == XX: && dev == %DEV will result in the filters being listed
> > for both the qdisc and the class. I haven't found a fix that wouldn't
> > break many scripts so if you have a good idea, go ahead. ;->
> 
> I cant say ive run into this one - mostly i attach filters to root.
> Example script?

See the post as reply to Patick's comment. You won't notice it in
tc because it doesn't output complete trees.

> BTW, cycles allow me to work on the eaction stuff today. So far
> ive come to the conclusion that whats really needed to simplify
> writting some actions is to just provide generic methods they
> can use without them necessarily knowing details. I hope to test
> later on, maybe write a simple action and its user apce piece, and post
> patch.

Sounds good, I've been mostly occupied by writing a API documentation
to libnl and clean it up which almost doubled its usefulnes. There
is still some effort to go into the core to make it easier to use for
applications needing non blocking sockets or weird multiplexing. 

The traffic control part is slowly evoling but it take some more time
to implement all the remaining qdiscs and classifiers. A bit of luck
allowed me to provide a XML based external interface to other
applications. It should be easy to extend it to make it bidirectional
and get distributed configuration via some kind of existing or
new protocol. The most problematic issue remaining is byte ordering.
I guess I will end up having all the modules define their part of
the grammar specifying the required byte order of every field and
have a generic parser to do transformations as necessary.

Re: patch: annoying u32 double listing

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 354 bytes --]

jamal wrote:

>I should have caught that output missing divisor ;->
>Dave please apply Patricks version. Should probably apply cleanly on
>2.4.x as well.
>
My last patch has a similar problem, new hash tables added with
"u32 divisor .." are skipped while walking. This patch is better
tested and should be fine. Applies to 2.4 and 2.6.

Regards
Patrick


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 1341 bytes --]

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2005/02/07 00:16:17+01:00 kaber@coreworks.de 
#   [PKT_SCHED]: Fix u32 double listing
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# net/sched/cls_u32.c
#   2005/02/07 00:16:08+01:00 kaber@coreworks.de +5 -0
#   [PKT_SCHED]: Fix u32 double listing
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
diff -Nru a/net/sched/cls_u32.c b/net/sched/cls_u32.c
--- a/net/sched/cls_u32.c	2005-02-07 00:16:37 +01:00
+++ b/net/sched/cls_u32.c	2005-02-07 00:16:37 +01:00
@@ -91,6 +91,7 @@
 {
 	struct tc_u_hnode	*next;
 	u32			handle;
+	u32			prio;
 	struct tc_u_common	*tp_c;
 	int			refcnt;
 	unsigned		divisor;
@@ -323,6 +324,7 @@
 	root_ht->divisor = 0;
 	root_ht->refcnt++;
 	root_ht->handle = tp_c ? gen_new_htid(tp_c) : 0x80000000;
+	root_ht->prio = tp->prio;
 
 	if (tp_c == NULL) {
 		tp_c = kmalloc(sizeof(*tp_c), GFP_KERNEL);
@@ -587,6 +589,7 @@
 		ht->refcnt = 0;
 		ht->divisor = divisor;
 		ht->handle = handle;
+		ht->prio = tp->prio;
 		ht->next = tp_c->hlist;
 		tp_c->hlist = ht;
 		*arg = (unsigned long)ht;
@@ -703,6 +706,8 @@
 		return;
 
 	for (ht = tp_c->hlist; ht; ht = ht->next) {
+		if (ht->prio != tp->prio)
+			continue;
 		if (arg->count >= arg->skip) {
 			if (arg->fn(tp, (unsigned long)ht, arg) < 0) {
 				arg->stop = 1;

Re: patch: annoying u32 double listing

[David S. Miller]

On Mon, 07 Feb 2005 00:17:30 +0100
Patrick McHardy <kaber@trash.net> wrote:

> jamal wrote:
> 
> >I should have caught that output missing divisor ;->
> >Dave please apply Patricks version. Should probably apply cleanly on
> >2.4.x as well.
> >
> My last patch has a similar problem, new hash tables added with
> "u32 divisor .." are skipped while walking. This patch is better
> tested and should be fine. Applies to 2.4 and 2.6.

After seeing 4+ revisions of the same bug fix on the same day,
let's let this sink for another day to make sure it's really
the final version :-)

Re: patch: annoying u32 double listing

[jamal]

I believe the last patch is correct. I can explain the many patches ;->
Call it the excitement to fix it if you may.
Its a problem that has existed for many years and many users have
complained about it but noone has really zoned on the cause fatale.
The 90% of it was really the patch i posted which zoned on it - for
years theory was the fix was in tc(user space). 
Patricks reincarnation got it to 95% and his second one got it to 100%.
IMO: Push it in, let some user whine.

cheers,
jamal

On Mon, 2005-02-07 at 01:36, David S. Miller wrote:
> On Mon, 07 Feb 2005 00:17:30 +0100
> Patrick McHardy <kaber@trash.net> wrote:
> 
> > jamal wrote:
> > 
> > >I should have caught that output missing divisor ;->
> > >Dave please apply Patricks version. Should probably apply cleanly on
> > >2.4.x as well.
> > >
> > My last patch has a similar problem, new hash tables added with
> > "u32 divisor .." are skipped while walking. This patch is better
> > tested and should be fine. Applies to 2.4 and 2.6.
> 
> After seeing 4+ revisions of the same bug fix on the same day,
> let's let this sink for another day to make sure it's really
> the final version :-)
> 

Re: patch: annoying u32 double listing

[David S. Miller]

On Mon, 07 Feb 2005 00:17:30 +0100
Patrick McHardy <kaber@trash.net> wrote:

> jamal wrote:
> 
> >I should have caught that output missing divisor ;->
> >Dave please apply Patricks version. Should probably apply cleanly on
> >2.4.x as well.
> >
> My last patch has a similar problem, new hash tables added with
> "u32 divisor .." are skipped while walking. This patch is better
> tested and should be fine. Applies to 2.4 and 2.6.

Applied, thanks Patrick.