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

[Alexander Duyck]

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.

However I can see your point.  At least then this driver would be
forward compatible in the event of an offload request for something
that isn't supported by the hardware.

> The patch parses the extension headers and verifies the protocol is
> supported (TCP, UDP, SCTP). But how does the device know the offset of
> the transport header to perform the checksum? Is the device capable of
> parsing the extension headers on transmit?

The device doesn't need to parse the extension headers.  All the
device needs is a pointer to the IPv6 header and the L4 header in
order to compute and place the L4 checksum.  That is the data that is
being placed in the descriptor by the tx_csum function.

- Alex