[ovs-dev,2/4] conntrack: remove redundant comparation in nat_packet and un_nat_packet

From: hepeng <hepeng.0320@bytedance.com>

In this patch, we split pat_packet and un_pat_packet function into _src
and _dst two functions. When calling this two functions, we can usually
infer the specific nat actions from the caller.

This patch makes the conntrack code cleaner.

Signed-off-by: Peng He <hepeng.0320@bytedance.com>
---
 lib/conntrack.c | 261 ++++++++++++++++++++++++++++--------------------
 1 file changed, 152 insertions(+), 109 deletions(-)

Bleep bloop.  Greetings hepeng.0320, 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.


checkpatch:
ERROR: Author hepeng <hepeng.0320@bytedance.com> needs to sign off.
WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Peng He <hepeng.0320@bytedance.com>
WARNING: Line is 99 characters long (recommended limit is 79)
#175 FILE: lib/conntrack.c:815:
static void do_nat4_src(struct dp_packet *pkt, struct ip_header *inner_l3, const struct conn *conn)

WARNING: Line is 99 characters long (recommended limit is 79)
#181 FILE: lib/conntrack.c:821:
static void do_nat4_dst(struct dp_packet *pkt, struct ip_header *inner_l3, const struct conn *conn)

WARNING: Line is 113 characters long (recommended limit is 79)
#187 FILE: lib/conntrack.c:827:
static void do_nat6_src(struct dp_packet *pkt, struct ovs_16aligned_ip6_hdr *inner_l3_6, const struct conn *conn)

WARNING: Line is 113 characters long (recommended limit is 79)
#194 FILE: lib/conntrack.c:834:
static void do_nat6_dst(struct dp_packet *pkt, struct ovs_16aligned_ip6_hdr *inner_l3_6, const struct conn *conn)

WARNING: Line is 101 characters long (recommended limit is 79)
#203 FILE: lib/conntrack.c:843:
                    void (*do_nat4)(struct dp_packet *, struct ip_header *, const struct conn *conn))

ERROR: Inappropriate spacing around cast
#234 FILE: lib/conntrack.c:858:
    extract_l3_ipv4(&inner_key, inner_l3, tail - ((char *)inner_l3) - pad,

WARNING: Line is 109 characters long (recommended limit is 79)
#277 FILE: lib/conntrack.c:874:
                    void (*do_nat6)(struct dp_packet *, struct ovs_16aligned_ip6_hdr *, const struct conn *))

ERROR: Inappropriate spacing around cast
#293 FILE: lib/conntrack.c:890:
            tail - ((char *)inner_l3_6) - pad,

WARNING: Line is 96 characters long (recommended limit is 79)
#311 FILE: lib/conntrack.c:907:
                   void (*do_nat4)(struct dp_packet *, struct ip_header *, const struct conn *),

WARNING: Line is 108 characters long (recommended limit is 79)
#312 FILE: lib/conntrack.c:908:
                   void (*do_nat6)(struct dp_packet *, struct ovs_16aligned_ip6_hdr *, const struct conn *))

Lines checked: 348, Warnings: 9, Errors: 3


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

Thanks,
0-day Robot

"hepeng.0320" <hepeng.0320@bytedance.com> writes:

> From: hepeng <hepeng.0320@bytedance.com>
>
> In this patch, we split pat_packet and un_pat_packet function into _src
> and _dst two functions. When calling this two functions, we can usually
> infer the specific nat actions from the caller.
>
> This patch makes the conntrack code cleaner.

This is doing much more than the above mentioned - it refactors v4/v6
code, and makes a generic callin that takes a function to perform
tcp/udp/other port nat.  It's a bit difficult to review for that
reason.

> Signed-off-by: Peng He <hepeng.0320@bytedance.com>
> ---
>  lib/conntrack.c | 261 ++++++++++++++++++++++++++++--------------------
>  1 file changed, 152 insertions(+), 109 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 73d3a2fb2..a22252a63 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -701,24 +701,26 @@ handle_alg_ctl(struct conntrack *ct, const struct conn_lookup_ctx *ctx,
>  }
>  
>  static void
> -pat_packet(struct dp_packet *pkt, const struct conn *conn)
> +pat_packet_src(struct dp_packet *pkt, const struct conn *conn)
>  {
> -    if (conn->conn_flags & NAT_ACTION_SRC) {
> -        if (conn->key.nw_proto == IPPROTO_TCP) {
> -            struct tcp_header *th = dp_packet_l4(pkt);
> -            packet_set_tcp_port(pkt, conn->rev_key.dst.port, th->tcp_dst);
> -        } else if (conn->key.nw_proto == IPPROTO_UDP) {
> -            struct udp_header *uh = dp_packet_l4(pkt);
> -            packet_set_udp_port(pkt, conn->rev_key.dst.port, uh->udp_dst);
> -        }
> -    } else if (conn->conn_flags & NAT_ACTION_DST) {
> -        if (conn->key.nw_proto == IPPROTO_TCP) {
> -            struct tcp_header *th = dp_packet_l4(pkt);
> -            packet_set_tcp_port(pkt, th->tcp_src, conn->rev_key.src.port);
> -        } else if (conn->key.nw_proto == IPPROTO_UDP) {
> -            struct udp_header *uh = dp_packet_l4(pkt);
> -            packet_set_udp_port(pkt, uh->udp_src, conn->rev_key.src.port);
> -        }
> +    if (conn->key.nw_proto == IPPROTO_TCP) {
> +        struct tcp_header *th = dp_packet_l4(pkt);
> +        packet_set_tcp_port(pkt, conn->rev_key.dst.port, th->tcp_dst);
> +    } else if (conn->key.nw_proto == IPPROTO_UDP) {
> +        struct udp_header *uh = dp_packet_l4(pkt);
> +        packet_set_udp_port(pkt, conn->rev_key.dst.port, uh->udp_dst);
> +    }
> +}
> +
> +static void
> +pat_packet_dst(struct dp_packet *pkt, const struct conn *conn)
> +{
> +    if (conn->key.nw_proto == IPPROTO_TCP) {
> +        struct tcp_header *th = dp_packet_l4(pkt);
> +        packet_set_tcp_port(pkt, th->tcp_src, conn->rev_key.src.port);
> +    } else if (conn->key.nw_proto == IPPROTO_UDP) {
> +        struct udp_header *uh = dp_packet_l4(pkt);
> +        packet_set_udp_port(pkt, uh->udp_src, conn->rev_key.src.port);
>      }
>  }
>  
> @@ -738,7 +740,7 @@ nat_packet(struct dp_packet *pkt, const struct conn *conn, bool related)
>                                   &conn->rev_key.dst.addr.ipv6, true);
>          }
>          if (!related) {
> -            pat_packet(pkt, conn);
> +            pat_packet_src(pkt, conn);
>          }
>      } else if (conn->conn_flags & NAT_ACTION_DST) {
>          pkt->md.ct_state |= CS_DST_NAT;
> @@ -753,61 +755,92 @@ nat_packet(struct dp_packet *pkt, const struct conn *conn, bool related)
>                                   &conn->rev_key.src.addr.ipv6, true);
>          }
>          if (!related) {
> -            pat_packet(pkt, conn);
> +            pat_packet_dst(pkt, conn);
>          }
>      }
>  }
>  
>  static void
> -un_pat_packet(struct dp_packet *pkt, const struct conn *conn)
> +un_pat_packet_src(struct dp_packet *pkt, const struct conn *conn)
>  {
> -    if (conn->conn_flags & NAT_ACTION_SRC) {
> -        if (conn->key.nw_proto == IPPROTO_TCP) {
> -            struct tcp_header *th = dp_packet_l4(pkt);
> -            packet_set_tcp_port(pkt, th->tcp_src, conn->key.src.port);
> -        } else if (conn->key.nw_proto == IPPROTO_UDP) {
> -            struct udp_header *uh = dp_packet_l4(pkt);
> -            packet_set_udp_port(pkt, uh->udp_src, conn->key.src.port);
> -        }
> -    } else if (conn->conn_flags & NAT_ACTION_DST) {
> -        if (conn->key.nw_proto == IPPROTO_TCP) {
> -            struct tcp_header *th = dp_packet_l4(pkt);
> -            packet_set_tcp_port(pkt, conn->key.dst.port, th->tcp_dst);
> -        } else if (conn->key.nw_proto == IPPROTO_UDP) {
> -            struct udp_header *uh = dp_packet_l4(pkt);
> -            packet_set_udp_port(pkt, conn->key.dst.port, uh->udp_dst);
> -        }
> +    if (conn->key.nw_proto == IPPROTO_TCP) {
> +        struct tcp_header *th = dp_packet_l4(pkt);
> +        packet_set_tcp_port(pkt, th->tcp_src, conn->key.src.port);
> +    } else if (conn->key.nw_proto == IPPROTO_UDP) {
> +        struct udp_header *uh = dp_packet_l4(pkt);
> +        packet_set_udp_port(pkt, uh->udp_src, conn->key.src.port);
>      }
>  }
>  
>  static void
> -reverse_pat_packet(struct dp_packet *pkt, const struct conn *conn)
> +un_pat_packet_dst(struct dp_packet *pkt, const struct conn *conn)
>  {
> -    if (conn->conn_flags & NAT_ACTION_SRC) {
> -        if (conn->key.nw_proto == IPPROTO_TCP) {
> -            struct tcp_header *th_in = dp_packet_l4(pkt);
> -            packet_set_tcp_port(pkt, conn->key.src.port,
> -                                th_in->tcp_dst);
> -        } else if (conn->key.nw_proto == IPPROTO_UDP) {
> -            struct udp_header *uh_in = dp_packet_l4(pkt);
> -            packet_set_udp_port(pkt, conn->key.src.port,
> -                                uh_in->udp_dst);
> -        }
> -    } else if (conn->conn_flags & NAT_ACTION_DST) {
> -        if (conn->key.nw_proto == IPPROTO_TCP) {
> -            struct tcp_header *th_in = dp_packet_l4(pkt);
> -            packet_set_tcp_port(pkt, th_in->tcp_src,
> -                                conn->key.dst.port);
> -        } else if (conn->key.nw_proto == IPPROTO_UDP) {
> -            struct udp_header *uh_in = dp_packet_l4(pkt);
> -            packet_set_udp_port(pkt, uh_in->udp_src,
> -                                conn->key.dst.port);
> -        }
> +    if (conn->key.nw_proto == IPPROTO_TCP) {
> +        struct tcp_header *th = dp_packet_l4(pkt);
> +        packet_set_tcp_port(pkt, conn->key.dst.port, th->tcp_dst);
> +    } else if (conn->key.nw_proto == IPPROTO_UDP) {
> +        struct udp_header *uh = dp_packet_l4(pkt);
> +        packet_set_udp_port(pkt, conn->key.dst.port, uh->udp_dst);
> +    }
> +}
> +
> +static void
> +reverse_pat_packet_src(struct dp_packet *pkt, const struct conn *conn)
> +{
> +    if (conn->key.nw_proto == IPPROTO_TCP) {
> +        struct tcp_header *th_in = dp_packet_l4(pkt);
> +        packet_set_tcp_port(pkt, conn->key.src.port,
> +                th_in->tcp_dst);
> +    } else if (conn->key.nw_proto == IPPROTO_UDP) {
> +        struct udp_header *uh_in = dp_packet_l4(pkt);
> +        packet_set_udp_port(pkt, conn->key.src.port,
> +                uh_in->udp_dst);
>      }
>  }
>  
>  static void
> -reverse_nat_packet(struct dp_packet *pkt, const struct conn *conn)
> +reverse_pat_packet_dst(struct dp_packet *pkt, const struct conn *conn)
> +{
> +    if (conn->key.nw_proto == IPPROTO_TCP) {
> +        struct tcp_header *th_in = dp_packet_l4(pkt);
> +        packet_set_tcp_port(pkt, th_in->tcp_src,
> +                conn->key.dst.port);
> +    } else if (conn->key.nw_proto == IPPROTO_UDP) {
> +        struct udp_header *uh_in = dp_packet_l4(pkt);
> +        packet_set_udp_port(pkt, uh_in->udp_src,
> +                conn->key.dst.port);
> +    }
> +}
> +
> +static void do_nat4_src(struct dp_packet *pkt, struct ip_header *inner_l3, const struct conn *conn)
> +{
> +    packet_set_ipv4_addr(pkt, &inner_l3->ip_src, conn->key.src.addr.ipv4);
> +    reverse_pat_packet_src(pkt, conn);
> +}
> +
> +static void do_nat4_dst(struct dp_packet *pkt, struct ip_header *inner_l3, const struct conn *conn)
> +{
> +    packet_set_ipv4_addr(pkt, &inner_l3->ip_dst, conn->key.dst.addr.ipv4);
> +    reverse_pat_packet_dst(pkt, conn);
> +}
> +
> +static void do_nat6_src(struct dp_packet *pkt, struct ovs_16aligned_ip6_hdr *inner_l3_6, const struct conn *conn)
> +{
> +    packet_set_ipv6_addr(pkt, conn->key.nw_proto, inner_l3_6->ip6_src.be32,
> +                &conn->key.src.addr.ipv6, true);
> +    reverse_pat_packet_src(pkt, conn);
> +}
> +
> +static void do_nat6_dst(struct dp_packet *pkt, struct ovs_16aligned_ip6_hdr *inner_l3_6, const struct conn *conn)
> +{
> +    packet_set_ipv6_addr(pkt, conn->key.nw_proto, inner_l3_6->ip6_dst.be32,
> +                &conn->key.dst.addr.ipv6, true);
> +    reverse_pat_packet_dst(pkt, conn);
> +}
> +
> +static void
> +reverse_nat_packet4(struct dp_packet *pkt, const struct conn *conn,
> +                    void (*do_nat4)(struct dp_packet *, struct ip_header *, const struct conn *conn))
>  {
>      char *tail = dp_packet_tail(pkt);
>      uint8_t pad = dp_packet_l2_pad_size(pkt);
> @@ -816,57 +849,67 @@ reverse_nat_packet(struct dp_packet *pkt, const struct conn *conn)
>      uint16_t orig_l3_ofs = pkt->l3_ofs;
>      uint16_t orig_l4_ofs = pkt->l4_ofs;
>  
> -    if (conn->key.dl_type == htons(ETH_TYPE_IP)) {
> -        struct ip_header *nh = dp_packet_l3(pkt);
> -        struct icmp_header *icmp = dp_packet_l4(pkt);
> -        struct ip_header *inner_l3 = (struct ip_header *) (icmp + 1);
> -        /* This call is already verified to succeed during the code path from
> -         * 'conn_key_extract()' which calls 'extract_l4_icmp()'. */
> -        extract_l3_ipv4(&inner_key, inner_l3, tail - ((char *)inner_l3) - pad,
> -                        &inner_l4, false);
> -        pkt->l3_ofs += (char *) inner_l3 - (char *) nh;
> -        pkt->l4_ofs += inner_l4 - (char *) icmp;
> -
> -        if (conn->conn_flags & NAT_ACTION_SRC) {
> -            packet_set_ipv4_addr(pkt, &inner_l3->ip_src,
> -                                 conn->key.src.addr.ipv4);
> -        } else if (conn->conn_flags & NAT_ACTION_DST) {
> -            packet_set_ipv4_addr(pkt, &inner_l3->ip_dst,
> -                                 conn->key.dst.addr.ipv4);
> -        }
> +    struct ip_header *nh = dp_packet_l3(pkt);
> +    struct icmp_header *icmp = dp_packet_l4(pkt);
> +    struct ip_header *inner_l3 = (struct ip_header *) (icmp + 1);
> +    /* This call is already verified to succeed during the code path from
> +     * 'conn_key_extract()' which calls 'extract_l4_icmp()'. */
> +    extract_l3_ipv4(&inner_key, inner_l3, tail - ((char *)inner_l3) - pad,
> +            &inner_l4, false);
> +    pkt->l3_ofs += (char *) inner_l3 - (char *) nh;
> +    pkt->l4_ofs += inner_l4 - (char *) icmp;
>  
> -        reverse_pat_packet(pkt, conn);
> -        icmp->icmp_csum = 0;
> -        icmp->icmp_csum = csum(icmp, tail - (char *) icmp - pad);
> -    } else {
> -        struct ovs_16aligned_ip6_hdr *nh6 = dp_packet_l3(pkt);
> -        struct icmp6_data_header *icmp6 = dp_packet_l4(pkt);
> -        struct ovs_16aligned_ip6_hdr *inner_l3_6 =
> -            (struct ovs_16aligned_ip6_hdr *) (icmp6 + 1);
> -        /* This call is already verified to succeed during the code path from
> -         * 'conn_key_extract()' which calls 'extract_l4_icmp6()'. */
> -        extract_l3_ipv6(&inner_key, inner_l3_6,
> -                        tail - ((char *)inner_l3_6) - pad,
> -                        &inner_l4);
> -        pkt->l3_ofs += (char *) inner_l3_6 - (char *) nh6;
> -        pkt->l4_ofs += inner_l4 - (char *) icmp6;
> -
> -        if (conn->conn_flags & NAT_ACTION_SRC) {
> -            packet_set_ipv6_addr(pkt, conn->key.nw_proto,
> -                                 inner_l3_6->ip6_src.be32,
> -                                 &conn->key.src.addr.ipv6, true);
> -        } else if (conn->conn_flags & NAT_ACTION_DST) {
> -            packet_set_ipv6_addr(pkt, conn->key.nw_proto,
> -                                 inner_l3_6->ip6_dst.be32,
> -                                 &conn->key.dst.addr.ipv6, true);
> -        }
> -        reverse_pat_packet(pkt, conn);
> -        icmp6->icmp6_base.icmp6_cksum = 0;
> -        icmp6->icmp6_base.icmp6_cksum = packet_csum_upperlayer6(nh6, icmp6,
> +    do_nat4(pkt, inner_l3, conn);
> +    icmp->icmp_csum = 0;
> +    icmp->icmp_csum = csum(icmp, tail - (char *) icmp - pad);
> +
> +    pkt->l3_ofs = orig_l3_ofs;
> +    pkt->l4_ofs = orig_l4_ofs;
> +}
> +
> +static void
> +reverse_nat_packet6(struct dp_packet *pkt, const struct conn *conn,
> +                    void (*do_nat6)(struct dp_packet *, struct ovs_16aligned_ip6_hdr *, const struct conn *))
> +{
> +    char *tail = dp_packet_tail(pkt);
> +    uint8_t pad = dp_packet_l2_pad_size(pkt);
> +    struct conn_key inner_key;
> +    const char *inner_l4 = NULL;
> +    uint16_t orig_l3_ofs = pkt->l3_ofs;
> +    uint16_t orig_l4_ofs = pkt->l4_ofs;
> +
> +    struct ovs_16aligned_ip6_hdr *nh6 = dp_packet_l3(pkt);
> +    struct icmp6_data_header *icmp6 = dp_packet_l4(pkt);
> +    struct ovs_16aligned_ip6_hdr *inner_l3_6 =
> +        (struct ovs_16aligned_ip6_hdr *) (icmp6 + 1);
> +    /* This call is already verified to succeed during the code path from
> +     * 'conn_key_extract()' which calls 'extract_l4_icmp6()'. */
> +    extract_l3_ipv6(&inner_key, inner_l3_6,
> +            tail - ((char *)inner_l3_6) - pad,
> +            &inner_l4);
> +    pkt->l3_ofs += (char *) inner_l3_6 - (char *) nh6;
> +    pkt->l4_ofs += inner_l4 - (char *) icmp6;
> +
> +    do_nat6(pkt, inner_l3_6, conn);
> +    icmp6->icmp6_base.icmp6_cksum = 0;
> +    icmp6->icmp6_base.icmp6_cksum = packet_csum_upperlayer6(nh6, icmp6,
>              IPPROTO_ICMPV6, tail - (char *) icmp6 - pad);
> -    }
> +
>      pkt->l3_ofs = orig_l3_ofs;
>      pkt->l4_ofs = orig_l4_ofs;
> +
> +}
> +
> +static void
> +reverse_nat_packet(struct dp_packet *pkt, const struct conn *conn,
> +                   void (*do_nat4)(struct dp_packet *, struct ip_header *, const struct conn *),
> +                   void (*do_nat6)(struct dp_packet *, struct ovs_16aligned_ip6_hdr *, const struct conn *))
> +{
> +    if (conn->key.dl_type == htons(ETH_TYPE_IP)) {
> +        reverse_nat_packet4(pkt, conn, do_nat4);
> +    } else {
> +        reverse_nat_packet6(pkt, conn, do_nat6);
> +    }
>  }

The above change (where you refactor ipv4 and ipv6 code separate) should
be a separate logical change.

>  static void
> @@ -886,9 +929,9 @@ un_nat_packet(struct dp_packet *pkt, const struct conn *conn, bool related)
>          }
>  
>          if (OVS_UNLIKELY(related)) {
> -            reverse_nat_packet(pkt, conn);
> +            reverse_nat_packet(pkt, conn, do_nat4_src, do_nat6_src);
>          } else {
> -            un_pat_packet(pkt, conn);
> +            un_pat_packet_src(pkt, conn);
>          }
>      } else if (conn->conn_flags & NAT_ACTION_DST) {
>          pkt->md.ct_state |= CS_SRC_NAT;
> @@ -904,9 +947,9 @@ un_nat_packet(struct dp_packet *pkt, const struct conn *conn, bool related)
>          }
>  
>          if (OVS_UNLIKELY(related)) {
> -            reverse_nat_packet(pkt, conn);
> +            reverse_nat_packet(pkt, conn, do_nat4_dst, do_nat6_dst);
>          } else {
> -            un_pat_packet(pkt, conn);
> +            un_pat_packet_dst(pkt, conn);
>          }
>      }
>  }