[PATCH iproute2] police: don't skip parameters after actions

[PATCH iproute2] police: don't skip parameters after actions

[Wolfgang Bumiller]

The 'parse_action_control()' helper advances the argument
pointers to past its parsed action already, so don't
advance it further in 'act_parse_polic()'.

Fixes: e67aba559581 ("tc: actions: add helpers to parse and print control actions")
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
Basically parse_action_control() silently added a NEXT_ARG() while the
cases before didn't have one. Not sure whether the goto is okay
style-wise, let me know if you prefer some other solution.

Example for triggering this:
Specifying a 'flowid X' after a `police ... drop` will skip the 'flowid'
and error with "What is X"

$ tc filter add dev eth0 parent ffff: basic police rate 13371337bps burst 1337b mtu 64kb drop flowid :1
What is ":1"?
...

 tc/m_police.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tc/m_police.c b/tc/m_police.c
index ff1dcb7d..f0878b3a 100644
--- a/tc/m_police.c
+++ b/tc/m_police.c
@@ -154,6 +154,7 @@ int act_parse_police(struct action_util *a, int *argc_p, char ***argv_p,
 			   matches(*argv, "goto") == 0) {
 			if (parse_action_control(&argc, &argv, &p.action, false))
 				return -1;
+			goto keep_arg;
 		} else if (strcmp(*argv, "conform-exceed") == 0) {
 			NEXT_ARG();
 			if (parse_action_control_slash(&argc, &argv, &p.action,
@@ -174,8 +175,9 @@ int act_parse_police(struct action_util *a, int *argc_p, char ***argv_p,
 		} else {
 			break;
 		}
-		ok++;
 		argc--; argv++;
+keep_arg:
+		ok++;
 	}
 
 	if (!ok)
-- 
2.11.0

Re: [PATCH iproute2] police: don't skip parameters after actions

[Stephen Hemminger]

On Mon, 29 Jan 2018 12:13:11 +0100
Wolfgang Bumiller <w.bumiller@proxmox.com> wrote:

> The 'parse_action_control()' helper advances the argument
> pointers to past its parsed action already, so don't
> advance it further in 'act_parse_polic()'.
> 
> Fixes: e67aba559581 ("tc: actions: add helpers to parse and print control actions")
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> ---
> Basically parse_action_control() silently added a NEXT_ARG() while the
> cases before didn't have one. Not sure whether the goto is okay
> style-wise, let me know if you prefer some other solution.
> 
> Example for triggering this:
> Specifying a 'flowid X' after a `police ... drop` will skip the 'flowid'
> and error with "What is X"
> 
> $ tc filter add dev eth0 parent ffff: basic police rate 13371337bps burst 1337b mtu 64kb drop flowid :1
> What is ":1"?

Thank you for the patch. It is a real problem, and your patch addresses it.
I just don't like jumping around in the the argument parsing with goto's.
There was a similar problem recently, and the better fix was to fix the semantics
of the parsing function to not  do the extra implicit NEXT_ARG in the parsing logic.
There is less likely to be future problems if all parsing functions leave the
with the same argument location.

Please try that and resubmit.

Re: [PATCH iproute2] police: don't skip parameters after actions

[Wolfgang Bumiller]

On Mon, Jan 29, 2018 at 08:07:23AM -0800, Stephen Hemminger wrote:
> On Mon, 29 Jan 2018 12:13:11 +0100
> Wolfgang Bumiller <w.bumiller@proxmox.com> wrote:
> 
> > The 'parse_action_control()' helper advances the argument
> > pointers to past its parsed action already, so don't
> > advance it further in 'act_parse_polic()'.
> > 
> > Fixes: e67aba559581 ("tc: actions: add helpers to parse and print control actions")
> > Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> > ---
> > Basically parse_action_control() silently added a NEXT_ARG() while the
> > cases before didn't have one. Not sure whether the goto is okay
> > style-wise, let me know if you prefer some other solution.
> > 
> > Example for triggering this:
> > Specifying a 'flowid X' after a `police ... drop` will skip the 'flowid'
> > and error with "What is X"
> > 
> > $ tc filter add dev eth0 parent ffff: basic police rate 13371337bps burst 1337b mtu 64kb drop flowid :1
> > What is ":1"?
> 
> Thank you for the patch. It is a real problem, and your patch addresses it.
> I just don't like jumping around in the the argument parsing with goto's.
> There was a similar problem recently, and the better fix was to fix the semantics
> of the parsing function to not  do the extra implicit NEXT_ARG in the parsing logic.
> There is less likely to be future problems if all parsing functions leave the
> with the same argument location.
> 
> Please try that and resubmit.

Actually I must apologize. Apparently I did not properly look at the
master branch but just the v4.13.0 tag. In master your requested
behavior already seems to be implemented. (There's even a commit with
the same `Fixes` tag by Michal Privoznik (3572e01a090).)
Sorry for the noise.