On Mon, Jun 20, 2016 at 2:58 PM, Jarno Rajahalme <ja...@ovn.org> wrote:
> 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.
OK, I see now that these are only being called by the helper
functions. In that case, I agree that this is the better place to
avoid the possibility that we call one without the other. It might be
a good idea to add a comment to note the reasoning here since it is
somewhat subtle.
I think the other changes are simple, so I don't feel the need to see
the series again.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
