diff mbox series

[ovs-dev,v6,2/3] conntrack: split the dst and src port range iterations

Message ID 1633793320-26048-2-git-send-email-wenxu@ucloud.cn
State Superseded
Headers show
Series [ovs-dev,v6,1/3] conntrack: select correct sport range for well-known origin sport | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

wenxu Oct. 9, 2021, 3:28 p.m. UTC 
From: wenxu <wenxu@ucloud.cn>

Splitting the two port range iterations instead of keeping it nested. And
the dst port (in case of DNAT) range would have precedence over the src
manipulation in the resolution.

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

Comments

Paolo Valerio Nov. 17, 2021, 3:09 p.m. UTC | #1 
Hi wenxu,

wenxu@ucloud.cn writes:

> From: wenxu <wenxu@ucloud.cn>
>
> Splitting the two port range iterations instead of keeping it nested. And
> the dst port (in case of DNAT) range would have precedence over the src
> manipulation in the resolution.
>
> Signed-off-by: wenxu <wenxu@ucloud.cn>
> ---
>  lib/conntrack.c | 65 +++++++++++++++++++++++++++------------------------------
>  1 file changed, 31 insertions(+), 34 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 44f99f3..a6784ba 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -1036,6 +1036,17 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
>                                                      nat_action_info);
>  
>                  if (!nat_res) {
> +                    if (nat_action_info->nat_action & NAT_ACTION_DST) {
> +                        struct nat_action_info_t tmp_nat_info;
> +
> +                        memset(&tmp_nat_info, 0, sizeof tmp_nat_info);
> +                        tmp_nat_info.nat_action = NAT_ACTION_SRC;
> +                        nat_res = nat_get_unique_tuple(ct, nc, nat_conn,
> +                                                       &tmp_nat_info);
> +                        if (!nat_res) {
> +                            goto nat_res_exhaustion;
> +                        }
> +                    }
>                      goto nat_res_exhaustion;
>                  }

is there any particular reason that brought you to call
nat_get_unique_tuple() twice instead of splitting the FOR_EACH with a
single call of nat_get_unique_tuple() ?

Something like:

FOR_EACH_PORT_IN_RANGE(curr_dport, min_dport, max_dport) {
   ...
}

FOR_EACH_PORT_IN_RANGE(curr_sport, min_sport, max_sport) {
   ...
}

That was my initial understanding. I suspect that approach would require
fewer changes as well.

>  
> @@ -2258,12 +2269,12 @@ nat_range_hash(const struct conn *conn, uint32_t basis,
>  
>  /* Ports are stored in host byte order for convenience. */
>  static void
> -set_sport_range(const struct nat_action_info_t *ni, const struct conn_key *k,
> -                uint32_t hash, uint16_t *curr, uint16_t *min,
> -                uint16_t *max)
> +set_port_range(const struct nat_action_info_t *ni, const struct conn_key *k,
> +               uint32_t hash, uint16_t *curr, uint16_t *min,
> +               uint16_t *max)
>  {
> -    if (((ni->nat_action & NAT_ACTION_SNAT_ALL) == NAT_ACTION_SRC) ||
> -        ((ni->nat_action & NAT_ACTION_DST))) {
> +    if ((ni->nat_action & NAT_ACTION_SRC) &&
> +        (!(ni->nat_action & NAT_ACTION_SRC_PORT))) {
>          *curr = ntohs(k->src.port);
>          if (*curr < 512) {
>              *min = 1;
> @@ -2275,6 +2286,10 @@ set_sport_range(const struct nat_action_info_t *ni, const struct conn_key *k,
>              *min = MIN_NAT_EPHEMERAL_PORT;
>              *max = MAX_NAT_EPHEMERAL_PORT;
>          }
> +    } else if ((ni->nat_action & NAT_ACTION_DST) &&
> +               (!(ni->nat_action & NAT_ACTION_DST_PORT))) {
> +        *curr = ntohs(k->dst.port);
> +        *min = *max = *curr;
>      } else {
>          *min = ni->min_port;
>          *max = ni->max_port;
> @@ -2282,21 +2297,6 @@ set_sport_range(const struct nat_action_info_t *ni, const struct conn_key *k,
>      }
>  }
>  
> -static void
> -set_dport_range(const struct nat_action_info_t *ni, const struct conn_key *k,
> -                uint32_t hash, uint16_t *curr, uint16_t *min,
> -                uint16_t *max)
> -{
> -    if (ni->nat_action & NAT_ACTION_DST_PORT) {
> -        *min = ni->min_port;
> -        *max = ni->max_port;
> -        *curr = *min + (hash % ((*max - *min) + 1));
> -    } else {
> -        *curr = ntohs(k->dst.port);
> -        *min = *max = *curr;
> -    }
> -}
> -
>  /* Gets the initial in range address based on the hash.
>   * Addresses are kept in network order. */
>  static void
> @@ -2426,8 +2426,7 @@ nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,
>      uint32_t hash = nat_range_hash(conn, ct->hash_basis, nat_info);
>      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_port, max_port, curr_port;
>  
>      min_addr = nat_info->min_addr;
>      max_addr = nat_info->max_addr;
> @@ -2439,10 +2438,8 @@ 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(nat_info, &conn->key, hash, &curr_sport,
> -                    &min_sport, &max_sport);
> -    set_dport_range(nat_info, &conn->key, hash, &curr_dport,
> -                    &min_dport, &max_dport);
> +    set_port_range(nat_info, &conn->key, hash, &curr_port,
> +                   &min_port, &max_port);
>  
>  another_round:
>      store_addr_to_key(&curr_addr, &nat_conn->rev_key,
> @@ -2457,14 +2454,14 @@ another_round:
>          goto next_addr;
>      }
>  
> -    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) {
> -            nat_conn->rev_key.dst.port = htons(curr_sport);
> -            if (!conn_lookup(ct, &nat_conn->rev_key,
> -                             time_msec(), NULL, NULL)) {
> -                return true;
> -            }
> +    FOR_EACH_PORT_IN_RANGE(curr_port, min_port, max_port) {
> +        if (nat_info->nat_action & NAT_ACTION_SRC) {
> +            nat_conn->rev_key.dst.port = htons(curr_port);
> +        } else {
> +            nat_conn->rev_key.src.port = htons(curr_port);
> +        }
> +        if (!conn_lookup(ct, &nat_conn->rev_key, time_msec(), NULL, NULL)) {
> +            return true;
>          }
>      }
>  
> -- 
> 1.8.3.1
wenxu Nov. 19, 2021, 3:10 a.m. UTC | #2 
From: Paolo Valerio <pvalerio@redhat.com>
Date: 2021-11-17 23:09:29
To:  wenxu@ucloud.cn,i.maximets@ovn.org
Cc:  dev@openvswitch.org
Subject: Re: [PATCH v6 2/3] conntrack: split the dst and src port range iterations>Hi wenxu,
>
>wenxu@ucloud.cn writes:
>
>> From: wenxu <wenxu@ucloud.cn>
>>
>> Splitting the two port range iterations instead of keeping it nested. And
>> the dst port (in case of DNAT) range would have precedence over the src
>> manipulation in the resolution.
>>
>> Signed-off-by: wenxu <wenxu@ucloud.cn>
>> ---
>>  lib/conntrack.c | 65 +++++++++++++++++++++++++++------------------------------
>>  1 file changed, 31 insertions(+), 34 deletions(-)
>>
>> diff --git a/lib/conntrack.c b/lib/conntrack.c
>> index 44f99f3..a6784ba 100644
>> --- a/lib/conntrack.c
>> +++ b/lib/conntrack.c
>> @@ -1036,6 +1036,17 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
>>                                                      nat_action_info);
>>  
>>                  if (!nat_res) {
>> +                    if (nat_action_info->nat_action & NAT_ACTION_DST) {
>> +                        struct nat_action_info_t tmp_nat_info;
>> +
>> +                        memset(&tmp_nat_info, 0, sizeof tmp_nat_info);
>> +                        tmp_nat_info.nat_action = NAT_ACTION_SRC;
>> +                        nat_res = nat_get_unique_tuple(ct, nc, nat_conn,
>> +                                                       &tmp_nat_info);
>> +                        if (!nat_res) {
>> +                            goto nat_res_exhaustion;
>> +                        }
>> +                    }
>>                      goto nat_res_exhaustion;
>>                  }
>
>is there any particular reason that brought you to call
>nat_get_unique_tuple() twice instead of splitting the FOR_EACH with a
>single call of nat_get_unique_tuple() ?
>
>Something like:
>
>FOR_EACH_PORT_IN_RANGE(curr_dport, min_dport, max_dport) {
>   ...
>}
>
>FOR_EACH_PORT_IN_RANGE(curr_sport, min_sport, max_sport) {
>   ...
>}
>
>That was my initial understanding. I suspect that approach would require
>fewer changes as well.
This is for the only case we should do twice FOR_EACH for both src and dst port.
ALL other case only need do one FOR_EACH for src or dst port. I think this is make 
the codes more simple and understand。
>
>>  
>> @@ -2258,12 +2269,12 @@ nat_range_hash(const struct conn *conn, uint32_t basis,
>>  
>>  /* Ports are stored in host byte order for convenience. */
>>  static void
>> -set_sport_range(const struct nat_action_info_t *ni, const struct conn_key *k,
>> -                uint32_t hash, uint16_t *curr, uint16_t *min,
>> -                uint16_t *max)
>> +set_port_range(const struct nat_action_info_t *ni, const struct conn_key *k,
>> +               uint32_t hash, uint16_t *curr, uint16_t *min,
>> +               uint16_t *max)
>>  {
>> -    if (((ni->nat_action & NAT_ACTION_SNAT_ALL) == NAT_ACTION_SRC) ||
>> -        ((ni->nat_action & NAT_ACTION_DST))) {
>> +    if ((ni->nat_action & NAT_ACTION_SRC) &&
>> +        (!(ni->nat_action & NAT_ACTION_SRC_PORT))) {
>>          *curr = ntohs(k->src.port);
>>          if (*curr < 512) {
>>              *min = 1;
>> @@ -2275,6 +2286,10 @@ set_sport_range(const struct nat_action_info_t *ni, const struct conn_key *k,
>>              *min = MIN_NAT_EPHEMERAL_PORT;
>>              *max = MAX_NAT_EPHEMERAL_PORT;
>>          }
>> +    } else if ((ni->nat_action & NAT_ACTION_DST) &&
>> +               (!(ni->nat_action & NAT_ACTION_DST_PORT))) {
>> +        *curr = ntohs(k->dst.port);
>> +        *min = *max = *curr;
>>      } else {
>>          *min = ni->min_port;
>>          *max = ni->max_port;
>> @@ -2282,21 +2297,6 @@ set_sport_range(const struct nat_action_info_t *ni, const struct conn_key *k,
>>      }
>>  }
>>  
>> -static void
>> -set_dport_range(const struct nat_action_info_t *ni, const struct conn_key *k,
>> -                uint32_t hash, uint16_t *curr, uint16_t *min,
>> -                uint16_t *max)
>> -{
>> -    if (ni->nat_action & NAT_ACTION_DST_PORT) {
>> -        *min = ni->min_port;
>> -        *max = ni->max_port;
>> -        *curr = *min + (hash % ((*max - *min) + 1));
>> -    } else {
>> -        *curr = ntohs(k->dst.port);
>> -        *min = *max = *curr;
>> -    }
>> -}
>> -
>>  /* Gets the initial in range address based on the hash.
>>   * Addresses are kept in network order. */
>>  static void
>> @@ -2426,8 +2426,7 @@ nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,
>>      uint32_t hash = nat_range_hash(conn, ct->hash_basis, nat_info);
>>      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_port, max_port, curr_port;
>>  
>>      min_addr = nat_info->min_addr;
>>      max_addr = nat_info->max_addr;
>> @@ -2439,10 +2438,8 @@ 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(nat_info, &conn->key, hash, &curr_sport,
>> -                    &min_sport, &max_sport);
>> -    set_dport_range(nat_info, &conn->key, hash, &curr_dport,
>> -                    &min_dport, &max_dport);
>> +    set_port_range(nat_info, &conn->key, hash, &curr_port,
>> +                   &min_port, &max_port);
>>  
>>  another_round:
>>      store_addr_to_key(&curr_addr, &nat_conn->rev_key,
>> @@ -2457,14 +2454,14 @@ another_round:
>>          goto next_addr;
>>      }
>>  
>> -    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) {
>> -            nat_conn->rev_key.dst.port = htons(curr_sport);
>> -            if (!conn_lookup(ct, &nat_conn->rev_key,
>> -                             time_msec(), NULL, NULL)) {
>> -                return true;
>> -            }
>> +    FOR_EACH_PORT_IN_RANGE(curr_port, min_port, max_port) {
>> +        if (nat_info->nat_action & NAT_ACTION_SRC) {
>> +            nat_conn->rev_key.dst.port = htons(curr_port);
>> +        } else {
>> +            nat_conn->rev_key.src.port = htons(curr_port);
>> +        }
>> +        if (!conn_lookup(ct, &nat_conn->rev_key, time_msec(), NULL, NULL)) {
>> +            return true;
>>          }
>>      }
>>  
>> -- 
>> 1.8.3.1
>
wenxu Nov. 19, 2021, 4:08 a.m. UTC | #3 
From: Paolo Valerio <pvalerio@redhat.com>
Date: 2021-11-17 23:09:29
To:  wenxu@ucloud.cn,i.maximets@ovn.org
Cc:  dev@openvswitch.org
Subject: Re: [PATCH v6 2/3] conntrack: split the dst and src port range iterations>Hi wenxu,
>
>wenxu@ucloud.cn writes:
>
>> From: wenxu <wenxu@ucloud.cn>
>>
>> Splitting the two port range iterations instead of keeping it nested. And
>> the dst port (in case of DNAT) range would have precedence over the src
>> manipulation in the resolution.
>>
>> Signed-off-by: wenxu <wenxu@ucloud.cn>
>> ---
>>  lib/conntrack.c | 65 +++++++++++++++++++++++++++------------------------------
>>  1 file changed, 31 insertions(+), 34 deletions(-)
>>
>> diff --git a/lib/conntrack.c b/lib/conntrack.c
>> index 44f99f3..a6784ba 100644
>> --- a/lib/conntrack.c
>> +++ b/lib/conntrack.c
>> @@ -1036,6 +1036,17 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
>>                                                      nat_action_info);
>>  
>>                  if (!nat_res) {
>> +                    if (nat_action_info->nat_action & NAT_ACTION_DST) {
>> +                        struct nat_action_info_t tmp_nat_info;
>> +
>> +                        memset(&tmp_nat_info, 0, sizeof tmp_nat_info);
>> +                        tmp_nat_info.nat_action = NAT_ACTION_SRC;
>> +                        nat_res = nat_get_unique_tuple(ct, nc, nat_conn,
>> +                                                       &tmp_nat_info);
>> +                        if (!nat_res) {
>> +                            goto nat_res_exhaustion;
>> +                        }
>> +                    }
>>                      goto nat_res_exhaustion;
>>                  }
>
>is there any particular reason that brought you to call
>nat_get_unique_tuple() twice instead of splitting the FOR_EACH with a
>single call of nat_get_unique_tuple() ?
>
>Something like:
>
>FOR_EACH_PORT_IN_RANGE(curr_dport, min_dport, max_dport) {
>   ...
>}
>
>FOR_EACH_PORT_IN_RANGE(curr_sport, min_sport, max_sport) {
>   ...
>}
>
>That was my initial understanding. I suspect that approach would require
>fewer changes as well.


And I also think this splitting the FOR_EACH twice method can't handle the both dst port
and src port nat case. For eample: do dnat case  dport range {100..200} and sport {MIN_NAT_EPHEMERAL_PORT..
MAX_NAT_EPHEMERAL_PORT } . This shuld be chain each FOR-EACH together but not separate it.
>
>>  
>> @@ -2258,12 +2269,12 @@ nat_range_hash(const struct conn *conn, uint32_t basis,
>>  
>>  /* Ports are stored in host byte order for convenience. */
>>  static void
>> -set_sport_range(const struct nat_action_info_t *ni, const struct conn_key *k,
>> -                uint32_t hash, uint16_t *curr, uint16_t *min,
>> -                uint16_t *max)
>> +set_port_range(const struct nat_action_info_t *ni, const struct conn_key *k,
>> +               uint32_t hash, uint16_t *curr, uint16_t *min,
>> +               uint16_t *max)
>>  {
>> -    if (((ni->nat_action & NAT_ACTION_SNAT_ALL) == NAT_ACTION_SRC) ||
>> -        ((ni->nat_action & NAT_ACTION_DST))) {
>> +    if ((ni->nat_action & NAT_ACTION_SRC) &&
>> +        (!(ni->nat_action & NAT_ACTION_SRC_PORT))) {
>>          *curr = ntohs(k->src.port);
>>          if (*curr < 512) {
>>              *min = 1;
>> @@ -2275,6 +2286,10 @@ set_sport_range(const struct nat_action_info_t *ni, const struct conn_key *k,
>>              *min = MIN_NAT_EPHEMERAL_PORT;
>>              *max = MAX_NAT_EPHEMERAL_PORT;
>>          }
>> +    } else if ((ni->nat_action & NAT_ACTION_DST) &&
>> +               (!(ni->nat_action & NAT_ACTION_DST_PORT))) {
>> +        *curr = ntohs(k->dst.port);
>> +        *min = *max = *curr;
>>      } else {
>>          *min = ni->min_port;
>>          *max = ni->max_port;
>> @@ -2282,21 +2297,6 @@ set_sport_range(const struct nat_action_info_t *ni, const struct conn_key *k,
>>      }
>>  }
>>  
>> -static void
>> -set_dport_range(const struct nat_action_info_t *ni, const struct conn_key *k,
>> -                uint32_t hash, uint16_t *curr, uint16_t *min,
>> -                uint16_t *max)
>> -{
>> -    if (ni->nat_action & NAT_ACTION_DST_PORT) {
>> -        *min = ni->min_port;
>> -        *max = ni->max_port;
>> -        *curr = *min + (hash % ((*max - *min) + 1));
>> -    } else {
>> -        *curr = ntohs(k->dst.port);
>> -        *min = *max = *curr;
>> -    }
>> -}
>> -
>>  /* Gets the initial in range address based on the hash.
>>   * Addresses are kept in network order. */
>>  static void
>> @@ -2426,8 +2426,7 @@ nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,
>>      uint32_t hash = nat_range_hash(conn, ct->hash_basis, nat_info);
>>      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_port, max_port, curr_port;
>>  
>>      min_addr = nat_info->min_addr;
>>      max_addr = nat_info->max_addr;
>> @@ -2439,10 +2438,8 @@ 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(nat_info, &conn->key, hash, &curr_sport,
>> -                    &min_sport, &max_sport);
>> -    set_dport_range(nat_info, &conn->key, hash, &curr_dport,
>> -                    &min_dport, &max_dport);
>> +    set_port_range(nat_info, &conn->key, hash, &curr_port,
>> +                   &min_port, &max_port);
>>  
>>  another_round:
>>      store_addr_to_key(&curr_addr, &nat_conn->rev_key,
>> @@ -2457,14 +2454,14 @@ another_round:
>>          goto next_addr;
>>      }
>>  
>> -    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) {
>> -            nat_conn->rev_key.dst.port = htons(curr_sport);
>> -            if (!conn_lookup(ct, &nat_conn->rev_key,
>> -                             time_msec(), NULL, NULL)) {
>> -                return true;
>> -            }
>> +    FOR_EACH_PORT_IN_RANGE(curr_port, min_port, max_port) {
>> +        if (nat_info->nat_action & NAT_ACTION_SRC) {
>> +            nat_conn->rev_key.dst.port = htons(curr_port);
>> +        } else {
>> +            nat_conn->rev_key.src.port = htons(curr_port);
>> +        }
>> +        if (!conn_lookup(ct, &nat_conn->rev_key, time_msec(), NULL, NULL)) {
>> +            return true;
>>          }
>>      }
>>  
>> -- 
>> 1.8.3.1
>
diff mbox series

Patch

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 44f99f3..a6784ba 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -1036,6 +1036,17 @@  conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
                                                     nat_action_info);
 
                 if (!nat_res) {
+                    if (nat_action_info->nat_action & NAT_ACTION_DST) {
+                        struct nat_action_info_t tmp_nat_info;
+
+                        memset(&tmp_nat_info, 0, sizeof tmp_nat_info);
+                        tmp_nat_info.nat_action = NAT_ACTION_SRC;
+                        nat_res = nat_get_unique_tuple(ct, nc, nat_conn,
+                                                       &tmp_nat_info);
+                        if (!nat_res) {
+                            goto nat_res_exhaustion;
+                        }
+                    }
                     goto nat_res_exhaustion;
                 }
 
@@ -2258,12 +2269,12 @@  nat_range_hash(const struct conn *conn, uint32_t basis,
 
 /* Ports are stored in host byte order for convenience. */
 static void
-set_sport_range(const struct nat_action_info_t *ni, const struct conn_key *k,
-                uint32_t hash, uint16_t *curr, uint16_t *min,
-                uint16_t *max)
+set_port_range(const struct nat_action_info_t *ni, const struct conn_key *k,
+               uint32_t hash, uint16_t *curr, uint16_t *min,
+               uint16_t *max)
 {
-    if (((ni->nat_action & NAT_ACTION_SNAT_ALL) == NAT_ACTION_SRC) ||
-        ((ni->nat_action & NAT_ACTION_DST))) {
+    if ((ni->nat_action & NAT_ACTION_SRC) &&
+        (!(ni->nat_action & NAT_ACTION_SRC_PORT))) {
         *curr = ntohs(k->src.port);
         if (*curr < 512) {
             *min = 1;
@@ -2275,6 +2286,10 @@  set_sport_range(const struct nat_action_info_t *ni, const struct conn_key *k,
             *min = MIN_NAT_EPHEMERAL_PORT;
             *max = MAX_NAT_EPHEMERAL_PORT;
         }
+    } else if ((ni->nat_action & NAT_ACTION_DST) &&
+               (!(ni->nat_action & NAT_ACTION_DST_PORT))) {
+        *curr = ntohs(k->dst.port);
+        *min = *max = *curr;
     } else {
         *min = ni->min_port;
         *max = ni->max_port;
@@ -2282,21 +2297,6 @@  set_sport_range(const struct nat_action_info_t *ni, const struct conn_key *k,
     }
 }
 
-static void
-set_dport_range(const struct nat_action_info_t *ni, const struct conn_key *k,
-                uint32_t hash, uint16_t *curr, uint16_t *min,
-                uint16_t *max)
-{
-    if (ni->nat_action & NAT_ACTION_DST_PORT) {
-        *min = ni->min_port;
-        *max = ni->max_port;
-        *curr = *min + (hash % ((*max - *min) + 1));
-    } else {
-        *curr = ntohs(k->dst.port);
-        *min = *max = *curr;
-    }
-}
-
 /* Gets the initial in range address based on the hash.
  * Addresses are kept in network order. */
 static void
@@ -2426,8 +2426,7 @@  nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,
     uint32_t hash = nat_range_hash(conn, ct->hash_basis, nat_info);
     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_port, max_port, curr_port;
 
     min_addr = nat_info->min_addr;
     max_addr = nat_info->max_addr;
@@ -2439,10 +2438,8 @@  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(nat_info, &conn->key, hash, &curr_sport,
-                    &min_sport, &max_sport);
-    set_dport_range(nat_info, &conn->key, hash, &curr_dport,
-                    &min_dport, &max_dport);
+    set_port_range(nat_info, &conn->key, hash, &curr_port,
+                   &min_port, &max_port);
 
 another_round:
     store_addr_to_key(&curr_addr, &nat_conn->rev_key,
@@ -2457,14 +2454,14 @@  another_round:
         goto next_addr;
     }
 
-    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) {
-            nat_conn->rev_key.dst.port = htons(curr_sport);
-            if (!conn_lookup(ct, &nat_conn->rev_key,
-                             time_msec(), NULL, NULL)) {
-                return true;
-            }
+    FOR_EACH_PORT_IN_RANGE(curr_port, min_port, max_port) {
+        if (nat_info->nat_action & NAT_ACTION_SRC) {
+            nat_conn->rev_key.dst.port = htons(curr_port);
+        } else {
+            nat_conn->rev_key.src.port = htons(curr_port);
+        }
+        if (!conn_lookup(ct, &nat_conn->rev_key, time_msec(), NULL, NULL)) {
+            return true;
         }
     }