netdev - Re: [PATCH iproute2 1/2] tc: ct: Fix ct commit nat forcing addr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <9955e54b-5f97-c35b-12cd-e0764ab2267b@nvidia.com>
Date:   Thu, 24 Nov 2022 10:21:08 +0200
From:   Roi Dayan <roid@...dia.com>
To:     Stephen Hemminger <stephen@...workplumber.org>
Cc:     netdev@...r.kernel.org, Paul Blakey <paulb@...dia.com>,
        David Ahern <dsahern@...nel.org>
Subject: Re: [PATCH iproute2 1/2] tc: ct: Fix ct commit nat forcing addr



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@...dia.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@...dia.com>
>>>> Reviewed-by: Paul Blakey <paulb@...dia.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