Thanks for the review Jesse, responses below: > On Jun 20, 2016, at 2:39 PM, Jesse Gross <je...@kernel.org> wrote: > > On Fri, Jun 17, 2016 at 6:44 PM, Jarno Rajahalme <ja...@ovn.org> wrote: >> diff --git a/datapath/conntrack.c b/datapath/conntrack.c >> index 4b3b78e..40e9843 100644 >> --- a/datapath/conntrack.c >> +++ b/datapath/conntrack.c >> @@ -337,6 +338,38 @@ static int ovs_ct_helper(struct sk_buff *skb, u16 proto) >> return NF_DROP; >> } >> >> +#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 contents. >> + * This dependency is avoided when skb is CHECKSUM_PARTIAL or when >> UDP >> + * has no checksum. >> + */ >> + if (ct->status & IPS_NAT_MASK && skb->ip_summed != CHECKSUM_PARTIAL) >> { > > I have a hard time verifying that this check is in the right place. It > looks to me like it only gets executed in the case that we have an ALG > and it also seems like it is running after the main NAT code. The > benefit of putting the checksum update here (as opposed to the compat > code where you had it earlier) is obviously that it can reuse the IPv6 > extension header scanning, however, it seems like clearly correct, at > least to me. >
The main NAT code takes care of mapping the address/port in the IP header. The (nat) helper takes care of mangling the TCP/UDP payload. The problematic kernel functions are only exercised by the nat helper, so it suffices to execute the code here before a nat helper is called, and avoids doing anything for NATted connections that do not have a helper assigned. Currently we only support the FTP helper, so most connections would be ones without a helper. >> + 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: >> + case IPPROTO_UDPLITE: > > I think UDP Lite doesn't need to be here - the checksum offload code > in the core kernel doesn't have any special logic for it and so will > compute a full packet checksum, the same as regular UDP. > > I verified that UDP Lite has it's own code path separate from UDP and > won't call into the checksum update function that causes a problem. It > might be worth adding a comment explaining why only TCP/UDP over > IPv4/v6 can cause an issue. > Thanks, I'll remove the UDPLITE line for v4. >> + if (skb->len < protoff + offset + 2) >> + return NF_DROP; > > It might be safer to use pskb_may_pull() here to verify the checksum > data is in the linear data area. The checksum code asserts this and > we've had some problems in the past with that. OK, thanks! Jarno _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev