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

[Tom Herbert]

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.