[ovs-dev] [PATCH 2/3] datapath: compat: unset skb encapsulation bit

[Jesse Gross]

On Tue, Jul 26, 2016 at 12:30 PM, pravin shelar <pshelar at ovn.org> wrote:
> On Tue, Jul 26, 2016 at 11:14 AM, Jesse Gross <jesse at kernel.org> wrote:
>> On Tue, Jul 26, 2016 at 10:56 AM, pravin shelar <pshelar at ovn.org> wrote:
>>> On Tue, Jul 26, 2016 at 10:06 AM, Jesse Gross <jesse at kernel.org> wrote:
>>>> On Mon, Jul 25, 2016 at 5:49 PM, Pravin B Shelar <pshelar at ovn.org> wrote:
>>>>> OVS compat layer can handle tunnel GSO packets. but it does
>>>>> keep skb encapsulation on for packet handled in GSO. This can
>>>>> confuse some NIC drivers. I have seen this issue on intel devices:
>>>>>
>>>>>   i40e 0000:42:00.0: TX driver issue detected, PF reset issued
>>>>>
>>>>> Following patch resets this bit in case compat layer handles the packet.
>>>>>
>>>>> VMware-BZ: 1698877
>>>>> Signed-off-by: Pravin B Shelar <pshelar at ovn.org>
>>>>
>>>> In upstream, this is done as part of the GSO code (for example, in
>>>> __skb_udp_tunnel_segment()) so that probably makes more sense and is
>>>> safer if this is GSO specific. There is already code in
>>>> ovs_iptunnel_handle_offloads() that will clear the encapsulation bit
>>>> in the case of checksum offload on the outer header.
>>>>
>>> ovs_iptunnel_handle_offloads() is not equivalent to
>>> skb_udp_tunnel_segment(). If handle-offload clear the bit it would not
>>> be possible to check the encapsulation bit after handle-offload call
>>> in tunnel implementation. At this point this bit is not checked, so it
>>> is not issue. But that would be different behavior compared to
>>> upstream.
>>
>> I was actually referring to existing behavior in
>> ovs_iptunnel_handle_offlads(). Here is the code that I was talking
>> about:
>>
>> #if LINUX_VERSION_CODE >= KERNEL_VERSION(3,8,0)
>>         /* If packet is not gso and we are resolving any partial checksum,
>>          * clear encapsulation flag. This allows setting CHECKSUM_PARTIAL
>>          * on the outer header without confusing devices that implement
>>          * NETIF_F_IP_CSUM with encapsulation.
>>          */
>>         if (csum_help)
>>                 skb->encapsulation = 0;
>> #endif
>>
>> In the case of outer checksums being enabled and offloaded and no GSO,
>> we will have cleared skb->encapsulation bit already, so there should
>> be no confusion for the NIC driver. As a result, this seems like a
>> GSO-only problem and we can just mirror what upstream does and clear
>> the bit in the GSO code. I'm a little bit nervous about
>> indiscriminately clearing it in all cases, since it seems like it is
>> possible that we will accidentally do it in some case where we are
>> trying to use more of the network stack.
>>
>
> So you are fine with clearing the bit in ip_local_out() but only for
> GSO packets.

Effectively, yes, just in a different spot. I think inside
tnl_skb_gso_segment() would make the most sense.