From: Paolo Valerio <pvale...@redhat.com>
Date: 2021-09-06 19:00:37
To: we...@ucloud.cn,i.maxim...@ovn.org,dlu...@gmail.com,acon...@redhat.com
Cc: d...@openvswitch.org
Subject: Re: [PATCH v2 1/2] conntrack: restore the origin port for each round
with new address>Hello,
>
>we...@ucloud.cn writes:
>
>> From: wenxu <we...@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 <we...@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
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
