[ovs-dev] [PATCH v2] ipf: fix only nat the first fragment in the reass process

ze wang wangze712 at gmail.com
Fri Aug 6 08:48:57 UTC 2021


We had the same problem, when I send a series of fragments, only the
first piece of the packet is NATed. We reviewed and tested the patch,
it do solve the problem, and we think there are not any other bugs in
it.
Reviewed-by: wangze <wangze712 at gmail.com>

wenxu <wenxu at ucloud.cn> 于2021年8月6日周五 下午4:38写道:
>
>
> My case :
>
>
> ovs-ofctl add-flow manbr "table=0,ct_state=-trk,ip,in_port=dpdk2, actions=ct(table=1, nat)"
> ovs-ofctl add-flow manbr "table=0,ct_state=-trk,ip,in_port=dpdk3, actions=ct(table=1, nat)"
> ovs-ofctl add-flow manbr "table=0,table=1,ct_state=+trk+new,ip,in_port=dpdk2, actions=ct(commit, nat(src=1.1.1.2)),dpdk3"
> ovs-ofctl add-flow manbr "table=0,table=1,ct_state=+trk+est,ip,in_port=dpdk2, actions=dpdk3"
> ovs-ofctl add-flow manbr "table=0,table=1,ct_state=+trk+est,ip,in_port=dpdk3, actions=dpdk2"
> ovs-ofctl add-flow manbr "table=0,arp,actions=normal"
>
>
>
> With the frag-packet in udp-frag.pcap. And the wrong nated packet. nat-packet.pcap
>
>
> send the packet:
> iperf -u -c 1.1.1.3 -t 120 -i 2  -b 1 -l 1800
>
>
>
>
> From: Aaron Conole <aconole at redhat.com>
> Date: 2021-07-09 04:11:12
> To:  wenxu at ucloud.cn
> Cc:  i.maximets at ovn.org,dev at openvswitch.org
> Subject: Re: [ovs-dev] [PATCH v2] ipf: fix only nat the first fragment in the reass process>wenxu at ucloud.cn writes:
> >
> >> From: wenxu <wenxu at ucloud.cn>
> >>
> >> The ipf collect original fragment packets and reass a new pkt
> >> to do the conntrack logic. After finsh the conntrack things
> >> copy the ct meta info to each orignal packet and modify the
> >> l4 header in the first fragment. It should modify the ip src/
> >> dst info for all the fragments.
> >>
> >> Signed-off-by: wenxu <wenxu at ucloud.cn>
> >> Co-authored-by: luke.li <luke.li at ucloud.cn>
> >> Signed-off-by: luke.li <luke.li at ucloud.cn>
> >> ---
> >
> >This looks okay.  Please post a set of sample packets that showcase the
> >issue.  I will write a test case that can demonstrate it that we can
> >check in with this fix.
> >Alternately, please add such a test case.
> >
> >>  lib/ipf.c | 82 +++++++++++++++++++++++++++++++++------------------------------
> >>  1 file changed, 43 insertions(+), 39 deletions(-)
> >>
> >> diff --git a/lib/ipf.c b/lib/ipf.c
> >> index 9c83f19..795e801 100644
> >> --- a/lib/ipf.c
> >> +++ b/lib/ipf.c
> >> @@ -1149,52 +1149,56 @@ ipf_post_execute_reass_pkts(struct ipf *ipf,
> >>           * NETDEV_MAX_BURST. */
> >>          DP_PACKET_BATCH_REFILL_FOR_EACH (pb_idx, pb_cnt, pkt, pb) {
> >>              if (rp && pkt == rp->list->reass_execute_ctx) {
> >> +                const struct ipf_frag *frag_0 = &rp->list->frag_list[0];
> >> +                void *l4_frag = dp_packet_l4(frag_0->pkt);
> >> +                void *l4_reass = dp_packet_l4(pkt);
> >> +                memcpy(l4_frag, l4_reass, dp_packet_l4_size(frag_0->pkt));
> >> +
> >>                  for (int i = 0; i <= rp->list->last_inuse_idx; i++) {
> >> -                    rp->list->frag_list[i].pkt->md.ct_label = pkt->md.ct_label;
> >> -                    rp->list->frag_list[i].pkt->md.ct_mark = pkt->md.ct_mark;
> >> -                    rp->list->frag_list[i].pkt->md.ct_state = pkt->md.ct_state;
> >> -                    rp->list->frag_list[i].pkt->md.ct_zone = pkt->md.ct_zone;
> >> -                    rp->list->frag_list[i].pkt->md.ct_orig_tuple_ipv6 =
> >> -                        pkt->md.ct_orig_tuple_ipv6;
> >> +                    const struct ipf_frag *frag_i = &rp->list->frag_list[i];
> >> +
> >> +                    frag_i->pkt->md.ct_label = pkt->md.ct_label;
> >> +                    frag_i->pkt->md.ct_mark = pkt->md.ct_mark;
> >> +                    frag_i->pkt->md.ct_state = pkt->md.ct_state;
> >> +                    frag_i->pkt->md.ct_zone = pkt->md.ct_zone;
> >> +                    frag_i->pkt->md.ct_orig_tuple_ipv6 =
> >> +                        pkt->md.ct_orig_tuple_ipv6;
> >>                      if (pkt->md.ct_orig_tuple_ipv6) {
> >> -                        rp->list->frag_list[i].pkt->md.ct_orig_tuple.ipv6 =
> >> +                        frag_i->pkt->md.ct_orig_tuple.ipv6 =
> >>                              pkt->md.ct_orig_tuple.ipv6;
> >>                      } else {
> >> -                        rp->list->frag_list[i].pkt->md.ct_orig_tuple.ipv4  =
> >> +                        frag_i->pkt->md.ct_orig_tuple.ipv4 =
> >>                              pkt->md.ct_orig_tuple.ipv4;
> >>                      }
> >> -                }
> >> -
> >> -                const struct ipf_frag *frag_0 = &rp->list->frag_list[0];
> >> -                void *l4_frag = dp_packet_l4(frag_0->pkt);
> >> -                void *l4_reass = dp_packet_l4(pkt);
> >> -                memcpy(l4_frag, l4_reass, dp_packet_l4_size(frag_0->pkt));
> >> -
> >> -                if (v6) {
> >> -                    struct ovs_16aligned_ip6_hdr *l3_frag
> >> -                        = dp_packet_l3(frag_0->pkt);
> >> -                    struct ovs_16aligned_ip6_hdr *l3_reass = dp_packet_l3(pkt);
> >> -                    l3_frag->ip6_src = l3_reass->ip6_src;
> >> -                    l3_frag->ip6_dst = l3_reass->ip6_dst;
> >> -                } else {
> >> -                    struct ip_header *l3_frag = dp_packet_l3(frag_0->pkt);
> >> -                    struct ip_header *l3_reass = dp_packet_l3(pkt);
> >> -                    if (!dp_packet_hwol_is_ipv4(frag_0->pkt)) {
> >> -                        ovs_be32 reass_ip =
> >> -                            get_16aligned_be32(&l3_reass->ip_src);
> >> -                        ovs_be32 frag_ip =
> >> -                            get_16aligned_be32(&l3_frag->ip_src);
> >> -
> >> -                        l3_frag->ip_csum = recalc_csum32(l3_frag->ip_csum,
> >> -                                                         frag_ip, reass_ip);
> >> -                        reass_ip = get_16aligned_be32(&l3_reass->ip_dst);
> >> -                        frag_ip = get_16aligned_be32(&l3_frag->ip_dst);
> >> -                        l3_frag->ip_csum = recalc_csum32(l3_frag->ip_csum,
> >> -                                                         frag_ip, reass_ip);
> >> +                    if (v6) {
> >> +                        struct ovs_16aligned_ip6_hdr *l3_frag
> >> +                            = dp_packet_l3(frag_i->pkt);
> >> +                        struct ovs_16aligned_ip6_hdr *l3_reass
> >> +                            = dp_packet_l3(pkt);
> >> +                        l3_frag->ip6_src = l3_reass->ip6_src;
> >> +                        l3_frag->ip6_dst = l3_reass->ip6_dst;
> >> +                    } else {
> >> +                        struct ip_header *l3_frag = dp_packet_l3(frag_i->pkt);
> >> +                        struct ip_header *l3_reass = dp_packet_l3(pkt);
> >> +                        if (!dp_packet_hwol_is_ipv4(frag_i->pkt)) {
> >> +                            ovs_be32 reass_ip =
> >> +                                get_16aligned_be32(&l3_reass->ip_src);
> >> +                            ovs_be32 frag_ip =
> >> +                                get_16aligned_be32(&l3_frag->ip_src);
> >> +
> >> +                            l3_frag->ip_csum = recalc_csum32(l3_frag->ip_csum,
> >> +                                                             frag_ip,
> >> +                                                             reass_ip);
> >> +                            reass_ip = get_16aligned_be32(&l3_reass->ip_dst);
> >> +                            frag_ip = get_16aligned_be32(&l3_frag->ip_dst);
> >> +                            l3_frag->ip_csum = recalc_csum32(l3_frag->ip_csum,
> >> +                                                             frag_ip,
> >> +                                                             reass_ip);
> >> +                        }
> >> +
> >> +                        l3_frag->ip_src = l3_reass->ip_src;
> >> +                        l3_frag->ip_dst = l3_reass->ip_dst;
> >>                      }
> >> -
> >> -                    l3_frag->ip_src = l3_reass->ip_src;
> >> -                    l3_frag->ip_dst = l3_reass->ip_dst;
> >>                  }
> >>
> >>                  ipf_completed_list_add(&ipf->frag_complete_list, rp->list);
> >
>
>
>
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list