[Intel-wired-lan] [PATCH V2] ixgbe: Handle extended IPv6 headers in tx path

[Alexander Duyck]

On Tue, Nov 17, 2015 at 8:48 AM, Tom Herbert <tom at herbertland.com> wrote:
> On Tue, Nov 17, 2015 at 8:41 AM, Alexander Duyck
> <alexander.duyck at gmail.com> wrote:
>> On Mon, Nov 16, 2015 at 9:21 PM, Tom Herbert <tom at herbertland.com> wrote:
>>> On Mon, Nov 16, 2015 at 4:26 PM, Mark D Rustad <mark.d.rustad at intel.com> wrote:
>>>> Check for and handle IPv6 extended headers so that tx checksum
>>>> offload can be done. Thanks to Tom Herbert for noticing this
>>>> problem. Thanks to Alexander Duyck for recognizing problems with
>>>> the first version of this patch.
>>>>
>>>> Reported-by: Tom Herbert <tom at herbertland.com>
>>>> Signed-off-by: Mark Rustad <mark.d.rustad at intel.com>
>>>> ---
>>>> Changed in V2:
>>>> - Use ipv6_skip_exthdr instead of ipv6_find_hdr
>>>> - Move code into L3 processing and avoid any possible loop
>>>> - Handle encapsulated IPv6 extended headers correctly
>>>> ---
>>>>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   19 +++++++++++++++++++
>>>>  1 file changed, 19 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> index 1ffbe85eab7b..23f316a9ad5a 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> @@ -7229,6 +7229,8 @@ static void ixgbe_tx_csum(struct ixgbe_ring *tx_ring,
>>>>                         struct tcphdr *tcphdr;
>>>>                         u8 *raw;
>>>>                 } transport_hdr;
>>>> +               __be16 frag_off;
>>>> +               int rc;
>>>>
>>>>                 if (skb->encapsulation) {
>>>>                         network_hdr.raw = skb_inner_network_header(skb);
>>>> @@ -7252,6 +7254,23 @@ static void ixgbe_tx_csum(struct ixgbe_ring *tx_ring,
>>>>                 case 6:
>>>>                         vlan_macip_lens |= transport_hdr.raw - network_hdr.raw;
>>>>                         l4_hdr = network_hdr.ipv6->nexthdr;
>>>> +                       if (likely((transport_hdr.raw - network_hdr.raw) ==
>>>> +                                  sizeof(struct ipv6hdr)))
>>>> +                               break;
>>>> +                       rc = ipv6_skip_exthdr(skb, network_hdr.raw +
>>>> +                                                  sizeof(struct ipv6hdr) -
>>>> +                                                  skb->data,
>>>> +                                             &l4_hdr, &frag_off);
>>>> +                       if (rc < 0) {
>>>> +                               if (unlikely(net_ratelimit())) {
>>>> +                                       dev_warn(tx_ring->dev,
>>>> +                                                "ipv6_skip_exthdr returned %d\n",
>>>> +                                                rc);
>>>> +                               }
>>>> +                               break;
>>>> +                       }
>>>> +                       if (frag_off)
>>>> +                               l4_hdr = 0;
>>>>                         break;
>>>>                 default:
>>>>                         if (unlikely(net_ratelimit())) {
>>>>
>>>
>>> Failures should not result in dev_warn, just call skb_checksum_help
>>> instead. Ignoring CHECKSUM_PARTIAL is guaranteed to be sending a bad
>>> checksum!
>>
>> The problem is there are scenarios here where a checksum shouldn't be
>> computed.  If for example the frame is fragmented we should be
>> displaying some sort of error message and not computing the checksum
>> because no matter what we compute it isn't going to be valid.
>>
> If the stack is sending fragments with CHECKSUM_PARTIAL set then that
> is a bug higher in the stack-- the driver does not need to worry about
> that.

Right, but it is still something that needs to get fixed.  By
triggering the dev_warn here we at least create visibility that
something has gone horribly wrong.

- Alex