* [PATCH iproute2 0/2] two fixes for tc ct command @ 2022-11-16 7:33 Roi Dayan 2022-11-16 7:33 ` [PATCH iproute2 1/2] tc: ct: Fix ct commit nat forcing addr Roi Dayan 2022-11-16 7:33 ` [PATCH iproute2 2/2] tc: ct: Fix invalid pointer derefence Roi Dayan 0 siblings, 2 replies; 7+ messages in thread From: Roi Dayan @ 2022-11-16 7:33 UTC (permalink / raw) To: netdev; +Cc: Roi Dayan, Paul Blakey, Stephen Hemminger, David Ahern Hi, The first first is that ct commit nat src/dst should accept the command even without an address and the driver fills the address from the tuple. The second commit fixes a segmentaion fault reading invalid argv pointer which is passing the available args passed by the user. Thanks, Roi Roi Dayan (2): tc: ct: Fix ct commit nat forcing addr tc: ct: Fix invalid pointer derefence man/man8/tc-ct.8 | 2 +- tc/m_ct.c | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) -- 2.38.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH iproute2 1/2] tc: ct: Fix ct commit nat forcing addr 2022-11-16 7:33 [PATCH iproute2 0/2] two fixes for tc ct command Roi Dayan @ 2022-11-16 7:33 ` Roi Dayan 2022-11-16 18:21 ` Stephen Hemminger 2022-11-16 7:33 ` [PATCH iproute2 2/2] tc: ct: Fix invalid pointer derefence Roi Dayan 1 sibling, 1 reply; 7+ messages in thread From: Roi Dayan @ 2022-11-16 7:33 UTC (permalink / raw) To: netdev; +Cc: Roi Dayan, Paul Blakey, Stephen Hemminger, David Ahern Action ct commit should accept nat src/dst without an addr. Fix it. Fixes: c8a494314c40 ("tc: Introduce tc ct action") Signed-off-by: Roi Dayan <roid@nvidia.com> Reviewed-by: Paul Blakey <paulb@nvidia.com> --- man/man8/tc-ct.8 | 2 +- tc/m_ct.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/man/man8/tc-ct.8 b/man/man8/tc-ct.8 index 2fb81ca29aa4..78d05e430c36 100644 --- a/man/man8/tc-ct.8 +++ b/man/man8/tc-ct.8 @@ -47,7 +47,7 @@ Specify a masked 32bit mark to set for the connection (only valid with commit). Specify a masked 128bit label to set for the connection (only valid with commit). .TP .BI nat " NAT_SPEC" -.BI Where " NAT_SPEC " ":= {src|dst} addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]" +.BI Where " NAT_SPEC " ":= {src|dst} [addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]]" Specify src/dst and range of nat to configure for the connection (only valid with commit). .RS diff --git a/tc/m_ct.c b/tc/m_ct.c index a02bf0cc1655..1b8984075a67 100644 --- a/tc/m_ct.c +++ b/tc/m_ct.c @@ -23,7 +23,7 @@ usage(void) " ct commit [force] [zone ZONE] [mark MASKED_MARK] [label MASKED_LABEL] [nat NAT_SPEC]\n" " ct [nat] [zone ZONE]\n" "Where: ZONE is the conntrack zone table number\n" - " NAT_SPEC is {src|dst} addr addr1[-addr2] [port port1[-port2]]\n" + " NAT_SPEC is {src|dst} [addr addr1[-addr2] [port port1[-port2]]]\n" "\n"); exit(-1); } @@ -234,7 +234,7 @@ parse_ct(struct action_util *a, int *argc_p, char ***argv_p, int tca_id, NEXT_ARG(); if (matches(*argv, "addr") != 0) - usage(); + continue; NEXT_ARG(); ret = ct_parse_nat_addr_range(*argv, n); -- 2.38.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH iproute2 1/2] tc: ct: Fix ct commit nat forcing addr 2022-11-16 7:33 ` [PATCH iproute2 1/2] tc: ct: Fix ct commit nat forcing addr Roi Dayan @ 2022-11-16 18:21 ` Stephen Hemminger 2022-11-17 5:35 ` Roi Dayan 0 siblings, 1 reply; 7+ messages in thread From: Stephen Hemminger @ 2022-11-16 18:21 UTC (permalink / raw) To: Roi Dayan; +Cc: netdev, Paul Blakey, David Ahern On Wed, 16 Nov 2022 09:33:11 +0200 Roi Dayan <roid@nvidia.com> wrote: > Action ct commit should accept nat src/dst without an addr. Fix it. > > Fixes: c8a494314c40 ("tc: Introduce tc ct action") > Signed-off-by: Roi Dayan <roid@nvidia.com> > Reviewed-by: Paul Blakey <paulb@nvidia.com> > --- > man/man8/tc-ct.8 | 2 +- > tc/m_ct.c | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/man/man8/tc-ct.8 b/man/man8/tc-ct.8 > index 2fb81ca29aa4..78d05e430c36 100644 > --- a/man/man8/tc-ct.8 > +++ b/man/man8/tc-ct.8 > @@ -47,7 +47,7 @@ Specify a masked 32bit mark to set for the connection (only valid with commit). > Specify a masked 128bit label to set for the connection (only valid with commit). > .TP > .BI nat " NAT_SPEC" > -.BI Where " NAT_SPEC " ":= {src|dst} addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]" > +.BI Where " NAT_SPEC " ":= {src|dst} [addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]]" > > Specify src/dst and range of nat to configure for the connection (only valid with commit). > .RS > diff --git a/tc/m_ct.c b/tc/m_ct.c > index a02bf0cc1655..1b8984075a67 100644 > --- a/tc/m_ct.c > +++ b/tc/m_ct.c > @@ -23,7 +23,7 @@ usage(void) > " ct commit [force] [zone ZONE] [mark MASKED_MARK] [label MASKED_LABEL] [nat NAT_SPEC]\n" > " ct [nat] [zone ZONE]\n" > "Where: ZONE is the conntrack zone table number\n" > - " NAT_SPEC is {src|dst} addr addr1[-addr2] [port port1[-port2]]\n" > + " NAT_SPEC is {src|dst} [addr addr1[-addr2] [port port1[-port2]]]\n" > "\n"); > exit(-1); > } > @@ -234,7 +234,7 @@ parse_ct(struct action_util *a, int *argc_p, char ***argv_p, int tca_id, > > NEXT_ARG(); > if (matches(*argv, "addr") != 0) > - usage(); > + continue; > This confuses me. Doing continue here will cause the current argument to be reprocessed so it would expect it to be zone | nat | clear | commit | force | index | mark | label which is not right. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH iproute2 1/2] tc: ct: Fix ct commit nat forcing addr 2022-11-16 18:21 ` Stephen Hemminger @ 2022-11-17 5:35 ` Roi Dayan 2022-11-17 14:00 ` Roi Dayan 0 siblings, 1 reply; 7+ messages in thread From: Roi Dayan @ 2022-11-17 5:35 UTC (permalink / raw) To: Stephen Hemminger; +Cc: netdev, Paul Blakey, David Ahern On 16/11/2022 20:21, Stephen Hemminger wrote: > On Wed, 16 Nov 2022 09:33:11 +0200 > Roi Dayan <roid@nvidia.com> wrote: > >> Action ct commit should accept nat src/dst without an addr. Fix it. >> >> Fixes: c8a494314c40 ("tc: Introduce tc ct action") >> Signed-off-by: Roi Dayan <roid@nvidia.com> >> Reviewed-by: Paul Blakey <paulb@nvidia.com> >> --- >> man/man8/tc-ct.8 | 2 +- >> tc/m_ct.c | 4 ++-- >> 2 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/man/man8/tc-ct.8 b/man/man8/tc-ct.8 >> index 2fb81ca29aa4..78d05e430c36 100644 >> --- a/man/man8/tc-ct.8 >> +++ b/man/man8/tc-ct.8 >> @@ -47,7 +47,7 @@ Specify a masked 32bit mark to set for the connection (only valid with commit). >> Specify a masked 128bit label to set for the connection (only valid with commit). >> .TP >> .BI nat " NAT_SPEC" >> -.BI Where " NAT_SPEC " ":= {src|dst} addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]" >> +.BI Where " NAT_SPEC " ":= {src|dst} [addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]]" >> >> Specify src/dst and range of nat to configure for the connection (only valid with commit). >> .RS >> diff --git a/tc/m_ct.c b/tc/m_ct.c >> index a02bf0cc1655..1b8984075a67 100644 >> --- a/tc/m_ct.c >> +++ b/tc/m_ct.c >> @@ -23,7 +23,7 @@ usage(void) >> " ct commit [force] [zone ZONE] [mark MASKED_MARK] [label MASKED_LABEL] [nat NAT_SPEC]\n" >> " ct [nat] [zone ZONE]\n" >> "Where: ZONE is the conntrack zone table number\n" >> - " NAT_SPEC is {src|dst} addr addr1[-addr2] [port port1[-port2]]\n" >> + " NAT_SPEC is {src|dst} [addr addr1[-addr2] [port port1[-port2]]]\n" >> "\n"); >> exit(-1); >> } >> @@ -234,7 +234,7 @@ parse_ct(struct action_util *a, int *argc_p, char ***argv_p, int tca_id, >> >> NEXT_ARG(); >> if (matches(*argv, "addr") != 0) >> - usage(); >> + continue; >> > > This confuses me. Doing continue here will cause the current argument to be reprocessed so > it would expect it to be zone | nat | clear | commit | force | index | mark | label > which is not right. > > its the opposite. "nat" came first. if matches() didn't find "addr" it continues the loop of args. if matches did find "addr" it continues to next line which is ct_parse_nat_addr_range() to parse the address. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH iproute2 1/2] tc: ct: Fix ct commit nat forcing addr 2022-11-17 5:35 ` Roi Dayan @ 2022-11-17 14:00 ` Roi Dayan 2022-11-24 8:21 ` Roi Dayan 0 siblings, 1 reply; 7+ messages in thread From: Roi Dayan @ 2022-11-17 14:00 UTC (permalink / raw) To: Stephen Hemminger; +Cc: netdev, Paul Blakey, David Ahern On 17/11/2022 7:35, Roi Dayan wrote: > > > On 16/11/2022 20:21, Stephen Hemminger wrote: >> On Wed, 16 Nov 2022 09:33:11 +0200 >> Roi Dayan <roid@nvidia.com> wrote: >> >>> Action ct commit should accept nat src/dst without an addr. Fix it. >>> >>> Fixes: c8a494314c40 ("tc: Introduce tc ct action") >>> Signed-off-by: Roi Dayan <roid@nvidia.com> >>> Reviewed-by: Paul Blakey <paulb@nvidia.com> >>> --- >>> man/man8/tc-ct.8 | 2 +- >>> tc/m_ct.c | 4 ++-- >>> 2 files changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/man/man8/tc-ct.8 b/man/man8/tc-ct.8 >>> index 2fb81ca29aa4..78d05e430c36 100644 >>> --- a/man/man8/tc-ct.8 >>> +++ b/man/man8/tc-ct.8 >>> @@ -47,7 +47,7 @@ Specify a masked 32bit mark to set for the connection (only valid with commit). >>> Specify a masked 128bit label to set for the connection (only valid with commit). >>> .TP >>> .BI nat " NAT_SPEC" >>> -.BI Where " NAT_SPEC " ":= {src|dst} addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]" >>> +.BI Where " NAT_SPEC " ":= {src|dst} [addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]]" >>> >>> Specify src/dst and range of nat to configure for the connection (only valid with commit). >>> .RS >>> diff --git a/tc/m_ct.c b/tc/m_ct.c >>> index a02bf0cc1655..1b8984075a67 100644 >>> --- a/tc/m_ct.c >>> +++ b/tc/m_ct.c >>> @@ -23,7 +23,7 @@ usage(void) >>> " ct commit [force] [zone ZONE] [mark MASKED_MARK] [label MASKED_LABEL] [nat NAT_SPEC]\n" >>> " ct [nat] [zone ZONE]\n" >>> "Where: ZONE is the conntrack zone table number\n" >>> - " NAT_SPEC is {src|dst} addr addr1[-addr2] [port port1[-port2]]\n" >>> + " NAT_SPEC is {src|dst} [addr addr1[-addr2] [port port1[-port2]]]\n" >>> "\n"); >>> exit(-1); >>> } >>> @@ -234,7 +234,7 @@ parse_ct(struct action_util *a, int *argc_p, char ***argv_p, int tca_id, >>> >>> NEXT_ARG(); >>> if (matches(*argv, "addr") != 0) >>> - usage(); >>> + continue; >>> >> >> This confuses me. Doing continue here will cause the current argument to be reprocessed so >> it would expect it to be zone | nat | clear | commit | force | index | mark | label >> which is not right. >> >> > > its the opposite. "nat" came first. if matches() didn't find "addr" > it continues the loop of args. if matches did find "addr" it continues > to next line which is ct_parse_nat_addr_range() to parse the address. > > Got your comment wrong so yes the current arg will be reprocessed and this is what we want. This will make "addr" optional and there should be some action after ct commit nat. next loop iteration should break and continue parse next action usually a goto action. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH iproute2 1/2] tc: ct: Fix ct commit nat forcing addr 2022-11-17 14:00 ` Roi Dayan @ 2022-11-24 8:21 ` Roi Dayan 0 siblings, 0 replies; 7+ messages in thread From: Roi Dayan @ 2022-11-24 8:21 UTC (permalink / raw) To: Stephen Hemminger; +Cc: netdev, Paul Blakey, David Ahern On 17/11/2022 16:00, Roi Dayan wrote: > > > On 17/11/2022 7:35, Roi Dayan wrote: >> >> >> On 16/11/2022 20:21, Stephen Hemminger wrote: >>> On Wed, 16 Nov 2022 09:33:11 +0200 >>> Roi Dayan <roid@nvidia.com> wrote: >>> >>>> Action ct commit should accept nat src/dst without an addr. Fix it. >>>> >>>> Fixes: c8a494314c40 ("tc: Introduce tc ct action") >>>> Signed-off-by: Roi Dayan <roid@nvidia.com> >>>> Reviewed-by: Paul Blakey <paulb@nvidia.com> >>>> --- >>>> man/man8/tc-ct.8 | 2 +- >>>> tc/m_ct.c | 4 ++-- >>>> 2 files changed, 3 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/man/man8/tc-ct.8 b/man/man8/tc-ct.8 >>>> index 2fb81ca29aa4..78d05e430c36 100644 >>>> --- a/man/man8/tc-ct.8 >>>> +++ b/man/man8/tc-ct.8 >>>> @@ -47,7 +47,7 @@ Specify a masked 32bit mark to set for the connection (only valid with commit). >>>> Specify a masked 128bit label to set for the connection (only valid with commit). >>>> .TP >>>> .BI nat " NAT_SPEC" >>>> -.BI Where " NAT_SPEC " ":= {src|dst} addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]" >>>> +.BI Where " NAT_SPEC " ":= {src|dst} [addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]]" >>>> >>>> Specify src/dst and range of nat to configure for the connection (only valid with commit). >>>> .RS >>>> diff --git a/tc/m_ct.c b/tc/m_ct.c >>>> index a02bf0cc1655..1b8984075a67 100644 >>>> --- a/tc/m_ct.c >>>> +++ b/tc/m_ct.c >>>> @@ -23,7 +23,7 @@ usage(void) >>>> " ct commit [force] [zone ZONE] [mark MASKED_MARK] [label MASKED_LABEL] [nat NAT_SPEC]\n" >>>> " ct [nat] [zone ZONE]\n" >>>> "Where: ZONE is the conntrack zone table number\n" >>>> - " NAT_SPEC is {src|dst} addr addr1[-addr2] [port port1[-port2]]\n" >>>> + " NAT_SPEC is {src|dst} [addr addr1[-addr2] [port port1[-port2]]]\n" >>>> "\n"); >>>> exit(-1); >>>> } >>>> @@ -234,7 +234,7 @@ parse_ct(struct action_util *a, int *argc_p, char ***argv_p, int tca_id, >>>> >>>> NEXT_ARG(); >>>> if (matches(*argv, "addr") != 0) >>>> - usage(); >>>> + continue; >>>> >>> >>> This confuses me. Doing continue here will cause the current argument to be reprocessed so >>> it would expect it to be zone | nat | clear | commit | force | index | mark | label >>> which is not right. >>> >>> >> >> its the opposite. "nat" came first. if matches() didn't find "addr" >> it continues the loop of args. if matches did find "addr" it continues >> to next line which is ct_parse_nat_addr_range() to parse the address. >> >> > > Got your comment wrong so yes the current arg will be reprocessed > and this is what we want. > This will make "addr" optional and there should be some action > after ct commit nat. next loop iteration should break and > continue parse next action usually a goto action. > Hi Stephen, Can you look at this again please? this is the same as done in other args like "port" right after and probably in other actions. If "addr" is not the next arg we continue the loop and parse again as expected for the other ct args or getting to else and breaking to continue. Thanks, Roi ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH iproute2 2/2] tc: ct: Fix invalid pointer derefence 2022-11-16 7:33 [PATCH iproute2 0/2] two fixes for tc ct command Roi Dayan 2022-11-16 7:33 ` [PATCH iproute2 1/2] tc: ct: Fix ct commit nat forcing addr Roi Dayan @ 2022-11-16 7:33 ` Roi Dayan 1 sibling, 0 replies; 7+ messages in thread From: Roi Dayan @ 2022-11-16 7:33 UTC (permalink / raw) To: netdev; +Cc: Roi Dayan, Paul Blakey, Stephen Hemminger, David Ahern Using mcaro NEXT_ARG_FWD does not validate argc. Use macro NEXT_ARG which validates argc while parsing args in the same loop iteration. Fixes: c8a494314c40 ("tc: Introduce tc ct action") Signed-off-by: Roi Dayan <roid@nvidia.com> Reviewed-by: Paul Blakey <paulb@nvidia.com> --- tc/m_ct.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tc/m_ct.c b/tc/m_ct.c index 1b8984075a67..e1c4b822f8ac 100644 --- a/tc/m_ct.c +++ b/tc/m_ct.c @@ -243,7 +243,7 @@ parse_ct(struct action_util *a, int *argc_p, char ***argv_p, int tca_id, return -1; } - NEXT_ARG_FWD(); + NEXT_ARG(); if (matches(*argv, "port") != 0) continue; -- 2.38.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-11-24 8:21 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-11-16 7:33 [PATCH iproute2 0/2] two fixes for tc ct command Roi Dayan 2022-11-16 7:33 ` [PATCH iproute2 1/2] tc: ct: Fix ct commit nat forcing addr Roi Dayan 2022-11-16 18:21 ` Stephen Hemminger 2022-11-17 5:35 ` Roi Dayan 2022-11-17 14:00 ` Roi Dayan 2022-11-24 8:21 ` Roi Dayan 2022-11-16 7:33 ` [PATCH iproute2 2/2] tc: ct: Fix invalid pointer derefence Roi Dayan
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.