[net PATCH 0/2] Fixes for tunnel checksum and segmentation offloads

[net PATCH 0/2] Fixes for tunnel checksum and segmentation offloads

[Alexander Duyck]

This patch series is a subset of patches I had submitted for net-next.  I
plan to drop these two patches from the v3 of "Fix Tunnel features and
enable GSO partial for several drivers" and I am instead submitting them
for net since these are truly fixes and likely will need to be backported
to stable branches.

This series addresses 2 specific issues.  The first is that we could
request TSO on a v4 inner header while not supporting checksum offload of
the outer IPv6 header.  The second is that we could request an IPv6 inner
checksum offload without validating that we could actually support an inner
IPv6 checksum offload.

---

Alexander Duyck (2):
      net: Disable segmentation if checksumming is not supported
      vxlan: Add checksum check to the features check function


 include/linux/if_ether.h |    5 +++++
 include/net/vxlan.h      |    4 +++-
 net/core/dev.c           |    2 +-
 3 files changed, 9 insertions(+), 2 deletions(-)

[net PATCH 1/2] net: Disable segmentation if checksumming is not supported

[Alexander Duyck]

In the case of the mlx4 and mlx5 driver they do not support IPv6 checksum
offload for tunnels.  With this being the case we should disable GSO in
addition to the checksum offload features when we find that a device cannot
perform a checksum on a given packet type.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 net/core/dev.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 77a71cd68535..5c925ac50b95 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2802,7 +2802,7 @@ static netdev_features_t harmonize_features(struct sk_buff *skb,
 
 	if (skb->ip_summed != CHECKSUM_NONE &&
 	    !can_checksum_protocol(features, type)) {
-		features &= ~NETIF_F_CSUM_MASK;
+		features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
 	} else if (illegal_highdma(skb->dev, skb)) {
 		features &= ~NETIF_F_SG;
 	}

[net PATCH 2/2] vxlan: Add checksum check to the features check function

[Alexander Duyck]

We need to perform an additional check on the inner headers to determine if
we can offload the checksum for them.  Previously this check didn't occur
so we would generate an invalid frame in the case of an IPv6 header
encapsulated inside of an IPv4 tunnel.  To fix this I added a secondary
check to vxlan_features_check so that we can verify that we can offload the
inner checksum.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 include/linux/if_ether.h |    5 +++++
 include/net/vxlan.h      |    4 +++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/linux/if_ether.h b/include/linux/if_ether.h
index d5569734f672..548fd535fd02 100644
--- a/include/linux/if_ether.h
+++ b/include/linux/if_ether.h
@@ -28,6 +28,11 @@ static inline struct ethhdr *eth_hdr(const struct sk_buff *skb)
 	return (struct ethhdr *)skb_mac_header(skb);
 }
 
+static inline struct ethhdr *inner_eth_hdr(const struct sk_buff *skb)
+{
+	return (struct ethhdr *)skb_inner_mac_header(skb);
+}
+
 int eth_header_parse(const struct sk_buff *skb, unsigned char *haddr);
 
 extern ssize_t sysfs_format_mac(char *buf, const unsigned char *addr, int len);
diff --git a/include/net/vxlan.h b/include/net/vxlan.h
index 73ed2e951c02..35437c779da8 100644
--- a/include/net/vxlan.h
+++ b/include/net/vxlan.h
@@ -252,7 +252,9 @@ static inline netdev_features_t vxlan_features_check(struct sk_buff *skb,
 	    (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
 	     skb->inner_protocol != htons(ETH_P_TEB) ||
 	     (skb_inner_mac_header(skb) - skb_transport_header(skb) !=
-	      sizeof(struct udphdr) + sizeof(struct vxlanhdr))))
+	      sizeof(struct udphdr) + sizeof(struct vxlanhdr)) ||
+	     (skb->ip_summed != CHECKSUM_NONE &&
+	      !can_checksum_protocol(features, inner_eth_hdr(skb)->h_proto))))
 		return features & ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
 
 	return features;

Re: [net PATCH 1/2] net: Disable segmentation if checksumming is not supported

[Tom Herbert]

On Mon, May 2, 2016 at 9:25 AM, Alexander Duyck <aduyck@mirantis.com> wrote:
> In the case of the mlx4 and mlx5 driver they do not support IPv6 checksum
> offload for tunnels.  With this being the case we should disable GSO in
> addition to the checksum offload features when we find that a device cannot
> perform a checksum on a given packet type.
>
I'm not sure I understand this. If device can't support checksum
offload for tunnels doesn't that mean we have to do the checksum on
host regardless of whether GSO is being done?

Tom

> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
> ---
>  net/core/dev.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 77a71cd68535..5c925ac50b95 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2802,7 +2802,7 @@ static netdev_features_t harmonize_features(struct sk_buff *skb,
>
>         if (skb->ip_summed != CHECKSUM_NONE &&
>             !can_checksum_protocol(features, type)) {
> -               features &= ~NETIF_F_CSUM_MASK;
> +               features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
>         } else if (illegal_highdma(skb->dev, skb)) {
>                 features &= ~NETIF_F_SG;
>         }
>

Re: [net PATCH 1/2] net: Disable segmentation if checksumming is not supported

[Alexander Duyck]

On Mon, May 2, 2016 at 9:33 AM, Tom Herbert <tom@herbertland.com> wrote:
> On Mon, May 2, 2016 at 9:25 AM, Alexander Duyck <aduyck@mirantis.com> wrote:
>> In the case of the mlx4 and mlx5 driver they do not support IPv6 checksum
>> offload for tunnels.  With this being the case we should disable GSO in
>> addition to the checksum offload features when we find that a device cannot
>> perform a checksum on a given packet type.
>>
> I'm not sure I understand this. If device can't support checksum
> offload for tunnels doesn't that mean we have to do the checksum on
> host regardless of whether GSO is being done?

The use of the term GSO here might be the confusing part.  Basically
the issue is the hardware advertises it can do TSO for IPv4 on
encapsulated frames, however it doesn't indicate it can do IPv6
checksum offload.  So what ends up happening is that in the case of a
v4 over v6 tunnel we were going through validate_xmit_skb which will
check things in netif_skb_features and come out supporting the TSO but
no checksums.  As a result we would fall through and hit
skb_checksum_help and trigger the warn on in there because we had TSO
requested even though we couldn't do the checksum.

Basically I am just extending the kind of logic we have in
netdev_fix_features so that if we cannot support checksumming the
frame then we cannot support segmenting it.

- Alex

Re: [net PATCH 1/2] net: Disable segmentation if checksumming is not supported

[Tom Herbert]

On Mon, May 2, 2016 at 9:48 AM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Mon, May 2, 2016 at 9:33 AM, Tom Herbert <tom@herbertland.com> wrote:
>> On Mon, May 2, 2016 at 9:25 AM, Alexander Duyck <aduyck@mirantis.com> wrote:
>>> In the case of the mlx4 and mlx5 driver they do not support IPv6 checksum
>>> offload for tunnels.  With this being the case we should disable GSO in
>>> addition to the checksum offload features when we find that a device cannot
>>> perform a checksum on a given packet type.
>>>
>> I'm not sure I understand this. If device can't support checksum
>> offload for tunnels doesn't that mean we have to do the checksum on
>> host regardless of whether GSO is being done?
>
> The use of the term GSO here might be the confusing part.  Basically
> the issue is the hardware advertises it can do TSO for IPv4 on
> encapsulated frames, however it doesn't indicate it can do IPv6
> checksum offload.  So what ends up happening is that in the case of a
> v4 over v6 tunnel we were going through validate_xmit_skb which will
> check things in netif_skb_features and come out supporting the TSO but
> no checksums.  As a result we would fall through and hit
> skb_checksum_help and trigger the warn on in there because we had TSO
> requested even though we couldn't do the checksum.
>
> Basically I am just extending the kind of logic we have in
> netdev_fix_features so that if we cannot support checksumming the
> frame then we cannot support segmenting it.
>
Thanks for the explanation. We need to drive things so that all the
encapsulation combinations (v4/v4, v4/v6, v6/v4, v6/v6) are supported
by HW TSO if any of them are supported by a device. Maybe we should
still have some sort of warning message that HW is broken for some
combination (like it apparently it is for mlnx4)?

Tom

> - Alex

Re: [net PATCH 1/2] net: Disable segmentation if checksumming is not supported

[Alexander Duyck]

On Mon, May 2, 2016 at 10:07 AM, Tom Herbert <tom@herbertland.com> wrote:
> On Mon, May 2, 2016 at 9:48 AM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> On Mon, May 2, 2016 at 9:33 AM, Tom Herbert <tom@herbertland.com> wrote:
>>> On Mon, May 2, 2016 at 9:25 AM, Alexander Duyck <aduyck@mirantis.com> wrote:
>>>> In the case of the mlx4 and mlx5 driver they do not support IPv6 checksum
>>>> offload for tunnels.  With this being the case we should disable GSO in
>>>> addition to the checksum offload features when we find that a device cannot
>>>> perform a checksum on a given packet type.
>>>>
>>> I'm not sure I understand this. If device can't support checksum
>>> offload for tunnels doesn't that mean we have to do the checksum on
>>> host regardless of whether GSO is being done?
>>
>> The use of the term GSO here might be the confusing part.  Basically
>> the issue is the hardware advertises it can do TSO for IPv4 on
>> encapsulated frames, however it doesn't indicate it can do IPv6
>> checksum offload.  So what ends up happening is that in the case of a
>> v4 over v6 tunnel we were going through validate_xmit_skb which will
>> check things in netif_skb_features and come out supporting the TSO but
>> no checksums.  As a result we would fall through and hit
>> skb_checksum_help and trigger the warn on in there because we had TSO
>> requested even though we couldn't do the checksum.
>>
>> Basically I am just extending the kind of logic we have in
>> netdev_fix_features so that if we cannot support checksumming the
>> frame then we cannot support segmenting it.
>>
> Thanks for the explanation. We need to drive things so that all the
> encapsulation combinations (v4/v4, v4/v6, v6/v4, v6/v6) are supported
> by HW TSO if any of them are supported by a device. Maybe we should
> still have some sort of warning message that HW is broken for some
> combination (like it apparently it is for mlnx4)?

The problem is the user could end up switching features on/off via
ethtool to create the same kind of situation.  Generally the v4/v6 mix
and match is going to be a more difficult case to deal with.  By
adding the check to the VXLAN features check and updating the checksum
check to disable GSO I think we should have most if not all cases
covered.

Also as far as the mlx4 the Mellanox guys are looking into it because
they were sure the part is supposed to be able to support an outer
IPv6 header so we may see something int he future come out to address
that.  Really what I would like to see us get away from is having
hardware to do any tunnel parsing for Tx which is where I believe this
issue lies since on the Rx side the mlx4 doesn't seem to recognize
tunnels encapsulated in IPv6 since it doesn't perform an inner
checksum offload.

- Alex

Re: [net PATCH 0/2] Fixes for tunnel checksum and segmentation offloads

[David Miller]

From: Alexander Duyck <aduyck@mirantis.com>
Date: Mon, 02 May 2016 09:25:04 -0700

> This patch series is a subset of patches I had submitted for net-next.  I
> plan to drop these two patches from the v3 of "Fix Tunnel features and
> enable GSO partial for several drivers" and I am instead submitting them
> for net since these are truly fixes and likely will need to be backported
> to stable branches.
> 
> This series addresses 2 specific issues.  The first is that we could
> request TSO on a v4 inner header while not supporting checksum offload of
> the outer IPv6 header.  The second is that we could request an IPv6 inner
> checksum offload without validating that we could actually support an inner
> IPv6 checksum offload.

Series applied, thanks.