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

[Aaron Conole]

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);