Thanks for the review Jesse, responses below:
> On Jun 20, 2016, at 2:39 PM, Jesse Gross <[email protected]> wrote:
>
> On Fri, Jun 17, 2016 at 6:44 PM, Jarno Rajahalme <[email protected]> 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
[email protected]
http://openvswitch.org/mailman/listinfo/dev
