tc filter add ... fw ... action drop

tc filter add ... fw ... action drop

[Abhijit Menon-Sen]

Hi.

Is it a bug that:

  # tc filter add dev eth0 parent 1: protocol ip prio 0 handle 0xfffffff
    fw police rate 1 burst 1 mpu 0 mtu 1 action drop
                                         ^^^^^^^^^^^
creates a filter that looks like:

  # tc filter ls dev eth0
  filter parent 1: protocol ip pref 49152 fw 
  filter parent 1: protocol ip pref 49152 fw handle 0xfffffff police 0x1
  rate 0bit burst 0b mtu 1b action reclassify
                            ^^^^^^^^^^^^^^^^^
  ref -543190236 bind 4

(which reclassifies and thus lets 0xfffffff-marked packets through).

I'm pretty sure this used to work under 2.4.x (though I no longer have a
2.4 box to test with), but it hasn't worked on any of the 2.6.x kernels
I've tried (with both iproute2-ss060323 and 070710).

I haven't been able to find anything that suggests this change is
intentional. If it's not immediately obvious to anyone what the
problem is, I could try to track it down.

-- ams

Re: tc filter add ... fw ... action drop

[Patrick McHardy]

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

Abhijit Menon-Sen wrote:
> Hi.
> 
> Is it a bug that:
> 
>   # tc filter add dev eth0 parent 1: protocol ip prio 0 handle 0xfffffff
>     fw police rate 1 burst 1 mpu 0 mtu 1 action drop
>                                          ^^^^^^^^^^^
> creates a filter that looks like:
> 
>   # tc filter ls dev eth0
>   filter parent 1: protocol ip pref 49152 fw 
>   filter parent 1: protocol ip pref 49152 fw handle 0xfffffff police 0x1
>   rate 0bit burst 0b mtu 1b action reclassify
>                             ^^^^^^^^^^^^^^^^^
>   ref -543190236 bind 4
> 
> (which reclassifies and thus lets 0xfffffff-marked packets through).
> 
> I'm pretty sure this used to work under 2.4.x (though I no longer have a
> 2.4 box to test with), but it hasn't worked on any of the 2.6.x kernels
> I've tried (with both iproute2-ss060323 and 070710).


Good catch. It seems this is merely a parsing error, iproute doesn't
have an "action" parameter and aborts parsing, so it uses the default
value of "RECLASSIFY". It never had this parameter, so this patch
removes it from the help text and makes it return an error.


BTW, Stephen, whats the status of all the patches I sent you?

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

diff --git a/tc/m_police.c b/tc/m_police.c
index 36a7719..62e37f9 100644
--- a/tc/m_police.c
+++ b/tc/m_police.c
@@ -37,7 +37,7 @@ static void usage(void)
 	fprintf(stderr, "Usage: ... police rate BPS burst BYTES[/BYTES] [ mtu BYTES[/BYTES] ]\n");
 	fprintf(stderr, "                [ peakrate BPS ] [ avrate BPS ]\n");
 	fprintf(stderr, "                [ ACTIONTERM ]\n");
-	fprintf(stderr, "Old Syntax ACTIONTERM := action <EXCEEDACT>[/NOTEXCEEDACT] \n");
+	fprintf(stderr, "Old Syntax ACTIONTERM := <EXCEEDACT>[/NOTEXCEEDACT] \n");
 	fprintf(stderr, "New Syntax ACTIONTERM := conform-exceed <EXCEEDACT>[/NOTEXCEEDACT] \n");
 	fprintf(stderr, "Where: *EXCEEDACT := pipe | ok | reclassify | drop | continue \n");
 	fprintf(stderr, "Where:  pipe is only valid for new syntax \n");
@@ -236,7 +236,8 @@ int act_parse_police(struct action_util *a,int *argc_p, char ***argv_p, int tca_
 		} else if (strcmp(*argv, "help") == 0) {
 			usage();
 		} else {
-			break;
+			fprintf(stderr, "What is \"%s\"?\n", *argv);
+			return -1;
 		}
 		ok++;
 		argc--; argv++;

Re: tc filter add ... fw ... action drop

[Abhijit Menon-Sen]

At 2007-07-18 11:49:55 +0200, kaber@trash.net wrote:
>
> >   # tc filter add dev eth0 parent 1: protocol ip prio 0 handle
> >     0xfffffff fw police rate 1 burst 1 mpu 0 mtu 1 action drop
>
> It seems this is merely a parsing error, iproute doesn't have an
> "action" parameter and aborts parsing, so it uses the default
> value of "RECLASSIFY".

I can confirm that your patch deals with my command sensibly, and also
that "tc filter add ... conform-exceed drop/drop" does what I wanted.

> It never had this parameter [...]

That command is from a script that used to work with iproute2-ss020116
(2002!), which had the following in tc/m_police.c:

210     } else if (strcmp(*argv, "action") == 0) {
211             NEXT_ARG();
212             if (get_police_result(&p.action, &presult, *argv)) {

I don't know when that bit was dropped, but it used to be there. :-)

Thank you for your help.

-- ams

Re: tc filter add ... fw ... action drop

[Patrick McHardy]

Abhijit Menon-Sen wrote:
> At 2007-07-18 11:49:55 +0200, kaber@trash.net wrote:
> 
>>>  # tc filter add dev eth0 parent 1: protocol ip prio 0 handle
>>>    0xfffffff fw police rate 1 burst 1 mpu 0 mtu 1 action drop
>>
>>It seems this is merely a parsing error, iproute doesn't have an
>>"action" parameter and aborts parsing, so it uses the default
>>value of "RECLASSIFY".
> 
> 
> I can confirm that your patch deals with my command sensibly, and also
> that "tc filter add ... conform-exceed drop/drop" does what I wanted.
> 
> 
>>It never had this parameter [...]
> 
> 
> That command is from a script that used to work with iproute2-ss020116
> (2002!), which had the following in tc/m_police.c:
> 
> 210     } else if (strcmp(*argv, "action") == 0) {
> 211             NEXT_ARG();
> 212             if (get_police_result(&p.action, &presult, *argv)) {
> 
> I don't know when that bit was dropped, but it used to be there. :-)


Indeed, I missed that. I'll fix up the patch ..

Re: tc filter add ... fw ... action drop

[Patrick McHardy]

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

Patrick McHardy wrote:
> Abhijit Menon-Sen wrote:
> 
>>That command is from a script that used to work with iproute2-ss020116
>>(2002!), which had the following in tc/m_police.c:
>>
>>210     } else if (strcmp(*argv, "action") == 0) {
>>211             NEXT_ARG();
>>212             if (get_police_result(&p.action, &presult, *argv)) {
>>
>>I don't know when that bit was dropped, but it used to be there. :-)
> 
> 
> 
> Indeed, I missed that. I'll fix up the patch ..


OK this patch fixes parsing of "action ...". I've removed
the erroring on unknown arguments again since in that case
the caller should continue parsing.


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

diff --git a/tc/m_police.c b/tc/m_police.c
index 36a7719..76cdbe1 100644
--- a/tc/m_police.c
+++ b/tc/m_police.c
@@ -227,7 +227,8 @@ int act_parse_police(struct action_util *a,int *argc_p, char ***argv_p, int tca_
 			p.action = TC_POLICE_OK;
 		} else if (matches(*argv, "pipe") == 0) {
 			p.action = TC_POLICE_PIPE;
-		} else if (strcmp(*argv, "conform-exceed") == 0) {
+		} else if (strcmp(*argv, "action") == 0 ||
+			   strcmp(*argv, "conform-exceed") == 0) {
 			NEXT_ARG();
 			if (get_police_result(&p.action, &presult, *argv)) {
 				fprintf(stderr, "Illegal \"action\"\n");