[ovs-dev] [patch v1 1/2] conntrack: Fix wasted work for ICMP NAT.

[Darrell Ball]

I sent a V2 for the series as I changed patch 1 slightly.

On Mon, Feb 25, 2019 at 3:36 PM Darrell Ball <dlu998 at gmail.com> wrote:

> ICMPv4 and ICMPv6 are not subject to port address translation (PAT),
> however, a loop increments a local variable unnecessarily for
> ephemeral ports, resulting in wasted work for ICMPv4 and ICMPv6 packets
> subject to NAT.  Fix this by checking for PAT being enabled before
> incrementing the local port variable and bail out otherwise.
>
> Signed-off-by: Darrell Ball <dlu998 at gmail.com>
> ---
>  lib/conntrack.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 4028ba9..ecac584 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -2179,6 +2179,8 @@ nat_select_range_tuple(struct conntrack *ct, const
> struct conn *conn,
>      bool ephemeral_ports_tried = conn->nat_info->nat_action &
> NAT_ACTION_DST
>                                   ? true : false;
>      union ct_addr first_addr = ct_addr;
> +    bool pat_enabled = conn->key.nw_proto != IPPROTO_ICMP &&
> +                       conn->key.nw_proto != IPPROTO_ICMPV6;
>
>      while (true) {
>          if (conn->nat_info->nat_action & NAT_ACTION_SRC) {
> @@ -2186,11 +2188,7 @@ nat_select_range_tuple(struct conntrack *ct, const
> struct conn *conn,
>          } else {
>              nat_conn->rev_key.src.addr = ct_addr;
>          }
> -
> -        if ((conn->key.nw_proto == IPPROTO_ICMP) ||
> -            (conn->key.nw_proto == IPPROTO_ICMPV6)) {
> -            all_ports_tried = true;
> -        } else if (conn->nat_info->nat_action & NAT_ACTION_SRC) {
> +        if (conn->nat_info->nat_action & NAT_ACTION_SRC) {
>              nat_conn->rev_key.dst.port = htons(port);
>          } else {
>              nat_conn->rev_key.src.port = htons(port);
> @@ -2200,7 +2198,7 @@ nat_select_range_tuple(struct conntrack *ct, const
> struct conn *conn,
>                                                 ct->hash_basis);
>          if (new_insert) {
>              return true;
> -        } else if (!all_ports_tried) {
> +        } else if (pat_enabled && !all_ports_tried) {
>              if (min_port == max_port) {
>                  all_ports_tried = true;
>              } else if (port == max_port) {
> @@ -2222,7 +2220,7 @@ nat_select_range_tuple(struct conntrack *ct, const
> struct conn *conn,
>                  ct_addr = conn->nat_info->min_addr;
>              }
>              if (!memcmp(&ct_addr, &first_addr, sizeof ct_addr)) {
> -                if (!ephemeral_ports_tried) {
> +                if (pat_enabled && !ephemeral_ports_tried) {
>                      ephemeral_ports_tried = true;
>                      ct_addr = conn->nat_info->min_addr;
>                      first_addr = ct_addr;
> --
> 1.9.1
>
>