> On Dec 28, 2016, at 3:05 PM, John Hurley <john.hur...@netronome.com> wrote: > > sorry, updated patch….
This patch is still whitespace damaged. Maybe use git format-patch and git send-email to send it? > > -------------------------------- > > Fix for a bug when sending a NATed packet to helper function in kernels > <4.6. > > Setting CHECKSUM_PARTIAL flag can lead to L4 checksum corruption. > > Corruption can occur in datapath.c/queue_userspace_packet(). > You mean this code: /* Complete checksum if needed */ if (skb->ip_summed == CHECKSUM_PARTIAL && (err = skb_checksum_help(skb))) goto out; Would you care detailing why the corruption happens? Does it always happen if ip_summed is CHECKSUM_PARTIAL? > Giving the packet an skb_dst allows the kernel to correct the checksum. > > Verified with packets mangled by Conntrack/NAT helpers. > It would also be helpful to add the reference to the patch that this fixes (“Fixes:”) > Signed-off-by: John Hurley <john.hur...@netronome.com> > --- > > diff --git a/datapath/conntrack.c b/datapath/conntrack.c > index d942884..fef67ba 100644 > --- a/datapath/conntrack.c > +++ b/datapath/conntrack.c > @@ -314,6 +314,10 @@ static int ovs_ct_helper(struct sk_buff *skb, u16 > proto) > u8 nexthdr; > int err; > > +#if LINUX_VERSION_CODE < KERNEL_VERSION(4,6,0) > + struct rtable *rt = NULL; > +#endif > + > ct = nf_ct_get(skb, &ctinfo); > if (!ct || ctinfo == IP_CT_RELATED_REPLY) > return NF_ACCEPT; > @@ -352,43 +356,28 @@ static int ovs_ct_helper(struct sk_buff *skb, u16 > proto) > #if LINUX_VERSION_CODE < KERNEL_VERSION(4,6,0) > /* Linux 4.5 and older depend on skb_dst being set when recalculating > * checksums after NAT helper has mangled TCP or UDP packet payload. > - * This dependency is avoided when skb is CHECKSUM_PARTIAL or when UDP > - * has no checksum. > * > - * The dependency is not triggered when the main NAT code updates > - * checksums after translating the IP header (address, port), so this > - * fix only needs to be executed on packets that are both being NATted > - * and that have a helper assigned. > + * skb_dst is cast to a rtable struct and the flags examined. > + * Forcing these flags to have RTCF_LOCAL set allows checksum mod > + * to be carried out in the same way as kernel versions > 4.5 Are kernels > 4.5 treating all these packets as RTCF_LOCAL, or are some of them possibly CHECKSUM_PARTIAL? > */ > if (ct->status & IPS_NAT_MASK && skb->ip_summed != CHECKSUM_PARTIAL) { > - u8 ipproto = (proto == NFPROTO_IPV4) > - ? ip_hdr(skb)->protocol : nexthdr; > - u16 offset = 0; > - > - switch (ipproto) { > - case IPPROTO_TCP: > - offset = offsetof(struct tcphdr, check); > - break; > - case IPPROTO_UDP: > - /* Skip if no csum. */ > - if (udp_hdr(skb)->check) > - offset = offsetof(struct udphdr, check); > - break; > - } > - if (offset) { > - if (unlikely(!pskb_may_pull(skb, protoff + offset + 2))) > - return NF_DROP; > - > - skb->csum_start = skb_headroom(skb) + protoff; > - skb->csum_offset = offset; > - skb->ip_summed = CHECKSUM_PARTIAL; > - } > + rt = kmalloc(sizeof(struct rtable), GFP_KERNEL); Could the struct rtable be allocated from stack instead? Or could it be a static variable in the file scope? In either case we would avoid dynamic memory allocation/free. I recall setting ip_summed to CHECKSUM_PARTIAL was deemed a simpler way to backport as it (also) avoids the dynamic memory allocations. > + rt->rt_flags = RTCF_LOCAL; > + skb_dst_set(skb, (struct dst_entry *)rt); > } > #endif > err = helper->help(skb, protoff, ct, ctinfo); > if (err != NF_ACCEPT) > return err; > > +#if LINUX_VERSION_CODE < KERNEL_VERSION(4,6,0) > + if (rt) { > + skb_dst_set(skb, NULL); > + kfree(rt); > + } > +#endif > + > /* Adjust seqs after helper. This is needed due to some helpers (e.g., > * FTP with NAT) adusting the TCP payload size when mangling IP > * addresses and/or port numbers in the text-based control connection. > > -- > > On Wed, Dec 28, 2016 at 10:08 PM, Ben Pfaff <b...@ovn.org> wrote: > >> On Wed, Dec 28, 2016 at 09:37:30PM +0000, John Hurley wrote: >>> Fix for a bug when sending a NATed packet to helper function in kernels >>> <4.6. >>> >>> Setting CHECKSUM_PARTIAL flag means packets could have L4 checksum >>> corrupted in >>> >>> datapath.c/queue_userspace_packet(). >>> >>> Giving the packet an skb_dst allows the kernel to correct the checksum if >>> packet >>> mangling happens in Conntrack/NAT helpers. >>> >>> Signed-off-by: John Hurley <john.hur...@netronome.com> >> >> I'm not the right person to review this but I did notice that the patch >> is wordwrapped and otherwise space-damaged. >> > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev