| Message ID | 20180129111311.21610-1-w.bumiller@proxmox.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
| Series | [iproute2] police: don't skip parameters after actions | expand |
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.
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.
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)
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(-)