On Tue, Feb 7, 2023 at 11:54 AM Simon Horman <[email protected]> wrote:
> On Mon, Feb 06, 2023 at 12:46:10PM +0100, Ales Musil wrote: > > The inner header was not handled properly. > > Simplify the code which allows proper handling > > of the inner headers. > > > > Reported-at: https://bugzilla.redhat.com/2137754 > > Signed-off-by: Ales Musil <[email protected]> > > Nice clean-up too :) > > Reviewed-by: Simon Horman <[email protected]> > Thank you for the review. > > > diff --git a/lib/conntrack.c b/lib/conntrack.c > > index 550b2be9b..3162924ca 100644 > > --- a/lib/conntrack.c > > +++ b/lib/conntrack.c > > ... > > > static void > > -reverse_nat_packet(struct dp_packet *pkt, const struct conn *conn) > > +nat_inner_packet(struct dp_packet *pkt, struct conn_key *key, > > + uint16_t nat_action) > > { > > char *tail = dp_packet_tail(pkt); > > uint16_t pad = dp_packet_l2_pad_size(pkt); > > @@ -875,98 +827,77 @@ 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, > > + void *l3 = dp_packet_l3(pkt); > > + void *l4 = dp_packet_l4(pkt); > > + void *inner_l3; > > + /* These calls are already verified to succeed during the code path > from > > + * 'conn_key_extract()' which calls > > + * 'extract_l4_icmp()'/'extract_l4_icmp6()'. */ > > + if (key->dl_type == htons(ETH_TYPE_IP)) { > > + inner_l3 = (char *) l4 + sizeof(struct icmp_header); > > + 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; > > + } else { > > + inner_l3 = (char *) l4 + sizeof(struct icmp6_data_header); > > + extract_l3_ipv6(&inner_key, inner_l3, tail - ((char *) > inner_l3) - pad, > > + &inner_l4); > > + } > > + pkt->l3_ofs += (char *) inner_l3 - (char *) l3; > > + pkt->l4_ofs += inner_l4 - (char *) l4; > > > > - if (conn->nat_action & NAT_ACTION_SRC) { > > - packet_set_ipv4_addr(pkt, &inner_l3->ip_src, > > - conn->key.src.addr.ipv4); > > - } else if (conn->nat_action & NAT_ACTION_DST) { > > - packet_set_ipv4_addr(pkt, &inner_l3->ip_dst, > > - conn->key.dst.addr.ipv4); > > - } > > + /* Reverse the key for inner packet. */ > > + struct conn_key rev_key = *key; > > + conn_key_reverse(&rev_key); > > + > > + pat_packet(pkt, &rev_key); > > + > > + if (key->dl_type == htons(ETH_TYPE_IP)) { > > + nat_packet_ipv4(pkt, &rev_key, nat_action); > > > > - reverse_pat_packet(pkt, conn); > > + struct icmp_header *icmp = (struct icmp_header *) l4; > > icmp->icmp_csum = 0; > > nit: not for this patch, but is the above line necessary? > It actually is, because the checksum is part of the data for which the checksum is computed. It is a bit confusing, but aligned with the specification: "For purposes of computing the checksum, the value of the checksum field is zero." [0] [0] https://datatracker.ietf.org/doc/html/rfc791 > > > 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->nat_action & 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->nat_action & 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); > > + nat_packet_ipv6(pkt, &rev_key, nat_action); > > + > > + struct icmp6_data_header *icmp6 = (struct icmp6_data_header *) > l4; > > icmp6->icmp6_base.icmp6_cksum = 0; > > ditto. > > > - icmp6->icmp6_base.icmp6_cksum = packet_csum_upperlayer6(nh6, > icmp6, > > - IPPROTO_ICMPV6, tail - (char *) icmp6 - pad); > > + icmp6->icmp6_base.icmp6_cksum = > > + packet_csum_upperlayer6(l3, icmp6, IPPROTO_ICMPV6, > > + tail - (char *) icmp6 - pad); > > } > > + > > pkt->l3_ofs = orig_l3_ofs; > > pkt->l4_ofs = orig_l4_ofs; > > } > > Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
