[ovs-dev,v2,1/2] conntrack: restore the origin port for each round with new address

From: wenxu <wenxu@ucloud.cn>

It is better to choose the origin select port as current port
for each port search round with new address.

Signed-off-by: wenxu <wenxu@ucloud.cn>
---
 lib/conntrack.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Hello,

wenxu@ucloud.cn writes:

> From: wenxu <wenxu@ucloud.cn>
>
> It is better to choose the origin select port as current port
> for each port search round with new address.
>
> Signed-off-by: wenxu <wenxu@ucloud.cn>
> ---

This should happen normally.
It doesn't happen in the case of source port manipulation when the
default ephemeral range is used and the packet source port is below
1024. In that case the first IP iteration uses the packet source port,
whereas the others don't.

if we want to change this behavior, there are some more ways we can
consider, e.g.:

- Using 1024 as initial curr_sport for source ports < 1024.
- picking different default ranges
  - e.g. the kernel uses the range [1, 511], if the source port is < 512,
    [600, 1023], if it's >= 512 and < 1024, and [1024,65535] for the
    remaining ports.

What do you think if we do something like the third bullet and squash
this change, if needed, in your next patch?

In any case, there are a couple of small nits/questions, see inline.

>  lib/conntrack.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 551c206..2d14205 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -2412,8 +2412,8 @@ nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,
>      uint32_t hash = nat_range_hash(conn, ct->hash_basis);
>      bool pat_proto = conn->key.nw_proto == IPPROTO_TCP ||
>                       conn->key.nw_proto == IPPROTO_UDP;
> -    uint16_t min_dport, max_dport, curr_dport;
> -    uint16_t min_sport, max_sport, curr_sport;
> +    uint16_t min_dport, max_dport, curr_dport, orig_dport;
> +    uint16_t min_sport, max_sport, curr_sport, orig_sport;

can we keep the reverse xmas tree style here?

>  
>      min_addr = conn->nat_info->min_addr;
>      max_addr = conn->nat_info->max_addr;
> @@ -2425,9 +2425,9 @@ nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,
>       * we can stop once we reach it. */
>      guard_addr = curr_addr;
>  
> -    set_sport_range(conn->nat_info, &conn->key, hash, &curr_sport,
> +    set_sport_range(conn->nat_info, &conn->key, hash, &orig_sport,
>                      &min_sport, &max_sport);
> -    set_dport_range(conn->nat_info, &conn->key, hash, &curr_dport,
> +    set_dport_range(conn->nat_info, &conn->key, hash, &orig_dport,
>                      &min_dport, &max_dport);
>  
>  another_round:
> @@ -2443,6 +2443,9 @@ another_round:
>          goto next_addr;
>      }
>  
> +    curr_sport = orig_sport;
> +    curr_dport = orig_dport;
> +

can this happen with dport?

>      FOR_EACH_PORT_IN_RANGE(curr_dport, min_dport, max_dport) {
>          nat_conn->rev_key.src.port = htons(curr_dport);
>          FOR_EACH_PORT_IN_RANGE(curr_sport, min_sport, max_sport) {
> -- 
> 1.8.3.1

From: Paolo Valerio <pvalerio@redhat.com>
Date: 2021-09-06 19:00:37
To:  wenxu@ucloud.cn,i.maximets@ovn.org,dlu998@gmail.com,aconole@redhat.com
Cc:  dev@openvswitch.org
Subject: Re: [PATCH v2 1/2] conntrack: restore the origin port for each round with new address>Hello,
>
>wenxu@ucloud.cn writes:
>
>> From: wenxu <wenxu@ucloud.cn>
>>
>> It is better to choose the origin select port as current port
>> for each port search round with new address.
>>
>> Signed-off-by: wenxu <wenxu@ucloud.cn>
>> ---
>
>This should happen normally.
>It doesn't happen in the case of source port manipulation when the
>default ephemeral range is used and the packet source port is below
>1024. In that case the first IP iteration uses the packet source port,
>whereas the others don't.
>
>if we want to change this behavior, there are some more ways we can
>consider, e.g.:


This can be done for avoding keep old_sport for source ports < 1024 case. 
>
>- Using 1024 as initial curr_sport for source ports < 1024.
>- picking different default ranges
>  - e.g. the kernel uses the range [1, 511], if the source port is < 512,
>    [600, 1023], if it's >= 512 and < 1024, and [1024,65535] for the
>    remaining ports.
>
>What do you think if we do something like the third bullet and squash
>this change, if needed, in your next patch?
>
>In any case, there are a couple of small nits/questions, see inline.
>
>>  lib/conntrack.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/conntrack.c b/lib/conntrack.c
>> index 551c206..2d14205 100644
>> --- a/lib/conntrack.c
>> +++ b/lib/conntrack.c
>> @@ -2412,8 +2412,8 @@ nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,
>>      uint32_t hash = nat_range_hash(conn, ct->hash_basis);
>>      bool pat_proto = conn->key.nw_proto == IPPROTO_TCP ||
>>                       conn->key.nw_proto == IPPROTO_UDP;
>> -    uint16_t min_dport, max_dport, curr_dport;
>> -    uint16_t min_sport, max_sport, curr_sport;
>> +    uint16_t min_dport, max_dport, curr_dport, orig_dport;
>> +    uint16_t min_sport, max_sport, curr_sport, orig_sport;
>
>can we keep the reverse xmas tree style here?
>
>>  
>>      min_addr = conn->nat_info->min_addr;
>>      max_addr = conn->nat_info->max_addr;
>> @@ -2425,9 +2425,9 @@ nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,
>>       * we can stop once we reach it. */
>>      guard_addr = curr_addr;
>>  
>> -    set_sport_range(conn->nat_info, &conn->key, hash, &curr_sport,
>> +    set_sport_range(conn->nat_info, &conn->key, hash, &orig_sport,
>>                      &min_sport, &max_sport);
>> -    set_dport_range(conn->nat_info, &conn->key, hash, &curr_dport,
>> +    set_dport_range(conn->nat_info, &conn->key, hash, &orig_dport,
>>                      &min_dport, &max_dport);
>>  
>>  another_round:
>> @@ -2443,6 +2443,9 @@ another_round:
>>          goto next_addr;
>>      }
>>  
>> +    curr_sport = orig_sport;
>> +    curr_dport = orig_dport;
>> +
>
>can this happen with dport?
>
>>      FOR_EACH_PORT_IN_RANGE(curr_dport, min_dport, max_dport) {
>>          nat_conn->rev_key.src.port = htons(curr_dport);
>>          FOR_EACH_PORT_IN_RANGE(curr_sport, min_sport, max_sport) {
>> -- 
>> 1.8.3.1
>

From: Paolo Valerio <pvalerio@redhat.com>
Date: 2021-09-06 19:00:37
To:  wenxu@ucloud.cn,i.maximets@ovn.org,dlu998@gmail.com,aconole@redhat.com
Cc:  dev@openvswitch.org
Subject: Re: [PATCH v2 1/2] conntrack: restore the origin port for each round with new address>Hello,
>
>wenxu@ucloud.cn writes:
>
>> From: wenxu <wenxu@ucloud.cn>
>>
>> It is better to choose the origin select port as current port
>> for each port search round with new address.
>>
>> Signed-off-by: wenxu <wenxu@ucloud.cn>
>> ---
>
>This should happen normally.
>It doesn't happen in the case of source port manipulation when the
>default ephemeral range is used and the packet source port is below
>1024. In that case the first IP iteration uses the packet source port,
>whereas the others don't.
>
>if we want to change this behavior, there are some more ways we can
>consider, e.g.:


" In that case the first IP iteration uses the packet source port, whereas the others don't"


I think the above rule is not matter with the curr_sport picking different ranges?
So you means for source ports < 1024, each IP iteration should using different source port?




> >- Using 1024 as initial curr_sport for source ports < 1024. >- picking different default ranges > - e.g. the kernel uses the range [1, 511], if the source port is < 512, > [600, 1023], if it's >= 512 and < 1024, and [1024,65535] for the > remaining ports. > >What do you think if we do something like the third bullet and squash >this change, if needed, in your next patch? > >In any case, there are a couple of small nits/questions, see inline. > >> lib/conntrack.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/lib/conntrack.c b/lib/conntrack.c >> index 551c206..2d14205 100644 >> --- a/lib/conntrack.c >> +++ b/lib/conntrack.c >> @@ -2412,8 +2412,8 @@ nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn, >> uint32_t hash = nat_range_hash(conn, ct->hash_basis); >> bool pat_proto = conn->key.nw_proto == IPPROTO_TCP || >> conn->key.nw_proto == IPPROTO_UDP; >> - uint16_t min_dport, max_dport, curr_dport; >> - uint16_t min_sport, max_sp
 ort, curr_sport; >> + uint16_t min_dport, max_dport, curr_dport, orig_dport; >> + uint16_t min_sport, max_sport, curr_sport, orig_sport; > >can we keep the reverse xmas tree style here? > >> >> min_addr = conn->nat_info->min_addr; >> max_addr = conn->nat_info->max_addr; >> @@ -2425,9 +2425,9 @@ nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn, >> * we can stop once we reach it. */ >> guard_addr = curr_addr; >> >> - set_sport_range(conn->nat_info, &conn->key, hash, &curr_sport, >> + set_sport_range(conn->nat_info, &conn->key, hash, &orig_sport, >> &min_sport, &max_sport); >> - set_dport_range(conn->nat_info, &conn->key, hash, &curr_dport, >> + set_dport_range(conn->nat_info, &conn->key, hash, &orig_dport, >> &min_dport, &max_dport); >> >> another_round: >> @@ -2443,6 +2443,9 @@ another_round: >> goto next_addr; >> } >> >> + curr_sport = orig_sport; >> + curr_dport = orig_dport; >> + > >can this happen with dport? > >> FOR_EACH_PORT_IN_RANGE(curr_dport, min_dport, ma
 x_dport) { >> nat_conn->rev_key.src.port = htons(curr_dport); >> FOR_EACH_PORT_IN_RANGE(curr_sport, min_sport, max_sport) { >> -- >> 1.8.3.1 >

wenxu <wenxu@ucloud.cn> writes:

> From: Paolo Valerio <pvalerio@redhat.com>
> Date: 2021-09-06 19:00:37
> To:  wenxu@ucloud.cn,i.maximets@ovn.org,dlu998@gmail.com,aconole@redhat.com
> Cc:  dev@openvswitch.org
> Subject: Re: [PATCH v2 1/2] conntrack: restore the origin port for each round with new address>Hello,
>>
>>wenxu@ucloud.cn writes:
>>
>>> From: wenxu <wenxu@ucloud.cn>
>>>
>>> It is better to choose the origin select port as current port
>>> for each port search round with new address.
>>>
>>> Signed-off-by: wenxu <wenxu@ucloud.cn>
>>> ---
>>
>>This should happen normally.
>>It doesn't happen in the case of source port manipulation when the
>>default ephemeral range is used and the packet source port is below
>>1024. In that case the first IP iteration uses the packet source port,
>>whereas the others don't.
>>
>>if we want to change this behavior, there are some more ways we can
>>consider, e.g.:
>
> " In that case the first IP iteration uses the packet source port, whereas the others don't"
>
> I think the above rule is not matter with the curr_sport picking different ranges?
>
> So you means for source ports < 1024, each IP iteration should using different source port?
>

What I meant is that, without your patch, in that case, the first
attempt of the first IP iteration is performed using the original source
port. The next IP iterations use MIN_NAT_EPHEMERAL_PORT as a starting
port.

E.g. with ct(nat(commit,src=10.1.1.240-10.1.1.241)), packet src port 500, and all
the lookups detect a collision, results in:

curr IP: 10.1.1.240, curr source port: 500, 1024, 1025, ..., MAX_NAT_EPHEMERAL_PORT
curr IP: 10.1.1.241, curr source port: 1024, 1025, ..., MAX_NAT_EPHEMERAL_PORT

wenxu@ucloud.cn writes:

> From: wenxu <wenxu@ucloud.cn>
>
> It is better to choose the origin select port as current port
> for each port search round with new address.
>
> Signed-off-by: wenxu <wenxu@ucloud.cn>
> ---

Hi Wenxu,

Paolo has done a good job reviewing, so I won't look too much at the
code, but I think we might want to include a test or two in the
system-traffic.at file that can catch these NAT collision cases.  WDYT?

>  lib/conntrack.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 551c206..2d14205 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -2412,8 +2412,8 @@ nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,
>      uint32_t hash = nat_range_hash(conn, ct->hash_basis);
>      bool pat_proto = conn->key.nw_proto == IPPROTO_TCP ||
>                       conn->key.nw_proto == IPPROTO_UDP;
> -    uint16_t min_dport, max_dport, curr_dport;
> -    uint16_t min_sport, max_sport, curr_sport;
> +    uint16_t min_dport, max_dport, curr_dport, orig_dport;
> +    uint16_t min_sport, max_sport, curr_sport, orig_sport;
>  
>      min_addr = conn->nat_info->min_addr;
>      max_addr = conn->nat_info->max_addr;
> @@ -2425,9 +2425,9 @@ nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,
>       * we can stop once we reach it. */
>      guard_addr = curr_addr;
>  
> -    set_sport_range(conn->nat_info, &conn->key, hash, &curr_sport,
> +    set_sport_range(conn->nat_info, &conn->key, hash, &orig_sport,
>                      &min_sport, &max_sport);
> -    set_dport_range(conn->nat_info, &conn->key, hash, &curr_dport,
> +    set_dport_range(conn->nat_info, &conn->key, hash, &orig_dport,
>                      &min_dport, &max_dport);
>  
>  another_round:
> @@ -2443,6 +2443,9 @@ another_round:
>          goto next_addr;
>      }
>  
> +    curr_sport = orig_sport;
> +    curr_dport = orig_dport;
> +
>      FOR_EACH_PORT_IN_RANGE(curr_dport, min_dport, max_dport) {
>          nat_conn->rev_key.src.port = htons(curr_dport);
>          FOR_EACH_PORT_IN_RANGE(curr_sport, min_sport, max_sport) {

From: Aaron Conole <aconole@redhat.com>
Date: 2021-09-07 21:46:29
To:  wenxu@ucloud.cn
Cc:  i.maximets@ovn.org,dlu998@gmail.com,pvalerio@redhat.com,dev@openvswitch.org
Subject: Re: [PATCH v2 1/2] conntrack: restore the origin port for each round with new address>wenxu@ucloud.cn writes:
>
>> From: wenxu <wenxu@ucloud.cn>
>>
>> It is better to choose the origin select port as current port
>> for each port search round with new address.
>>
>> Signed-off-by: wenxu <wenxu@ucloud.cn>
>> ---
>
>Hi Wenxu,
>
>Paolo has done a good job reviewing, so I won't look too much at the
>code, but I think we might want to include a test or two in the
>system-traffic.at file that can catch these NAT collision cases.  WDYT?
This patch does not resolve the collison case.  Only optimazation the first src port


 selection.  And I think nat collision case is not easy to  setup through some little test.
>
>>  lib/conntrack.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/conntrack.c b/lib/conntrack.c
>> index 551c206..2d14205 100644
>> --- a/lib/conntrack.c
>> +++ b/lib/conntrack.c
>> @@ -2412,8 +2412,8 @@ nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,
>>      uint32_t hash = nat_range_hash(conn, ct->hash_basis);
>>      bool pat_proto = conn->key.nw_proto == IPPROTO_TCP ||
>>                       conn->key.nw_proto == IPPROTO_UDP;
>> -    uint16_t min_dport, max_dport, curr_dport;
>> -    uint16_t min_sport, max_sport, curr_sport;
>> +    uint16_t min_dport, max_dport, curr_dport, orig_dport;
>> +    uint16_t min_sport, max_sport, curr_sport, orig_sport;
>>  
>>      min_addr = conn->nat_info->min_addr;
>>      max_addr = conn->nat_info->max_addr;
>> @@ -2425,9 +2425,9 @@ nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,
>>       * we can stop once we reach it. */
>>      guard_addr = curr_addr;
>>  
>> -    set_sport_range(conn->nat_info, &conn->key, hash, &curr_sport,
>> +    set_sport_range(conn->nat_info, &conn->key, hash, &orig_sport,
>>                      &min_sport, &max_sport);
>> -    set_dport_range(conn->nat_info, &conn->key, hash, &curr_dport,
>> +    set_dport_range(conn->nat_info, &conn->key, hash, &orig_dport,
>>                      &min_dport, &max_dport);
>>  
>>  another_round:
>> @@ -2443,6 +2443,9 @@ another_round:
>>          goto next_addr;
>>      }
>>  
>> +    curr_sport = orig_sport;
>> +    curr_dport = orig_dport;
>> +
>>      FOR_EACH_PORT_IN_RANGE(curr_dport, min_dport, max_dport) {
>>          nat_conn->rev_key.src.port = htons(curr_dport);
>>          FOR_EACH_PORT_IN_RANGE(curr_sport, min_sport, max_sport) {
>

Bleep bloop.  Greetings wenxu, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


git-am:
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0001 conntrack: restore the origin port for each round with new address
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


Patch skipped due to previous failure.

Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot