[PATCHv3 net-next 0/2] net: add support for ip generic checksum offload for gre

This patchset it to add ip generic csum processing first in
skb_csum_hwoffload_help() in Patch 1/2 and then add csum
offload support for GRE header in Patch 2/2.

v1->v2:
  - See each patch's changelog.
v2->v3:
  - See the 1st patch.

Xin Long (2):
  net: support ip generic csum processing in skb_csum_hwoffload_help
  ip_gre: add csum offload support for gre header

 include/net/gre.h      | 19 +++++++------------
 net/core/dev.c         | 13 ++++++++++++-
 net/ipv4/gre_offload.c | 15 +++++++++++++--
 3 files changed, 32 insertions(+), 15 deletions(-)

-- 
2.1.0

This patch is to add csum offload support for gre header:

On the TX path in gre_build_header(), when CHECKSUM_PARTIAL's set
for inner proto, it will calculate the csum for outer proto, and
inner csum will be offloaded later. Otherwise, CHECKSUM_PARTIAL
and csum_start/offset will be set for outer proto, and the outer
csum will be offloaded later.

On the GSO path in gre_gso_segment(), when CHECKSUM_PARTIAL is
not set for inner proto and the hardware supports csum offload,
CHECKSUM_PARTIAL and csum_start/offset will be set for outer
proto, and outer csum will be offloaded later. Otherwise, it
will do csum for outer proto by calling gso_make_checksum().

Note that SCTP has to do the csum by itself for non GSO path in
sctp_packet_pack(), as gre_build_header() can't handle the csum
with CHECKSUM_PARTIAL set for SCTP CRC csum offload.

v1->v2:
  - remove the SCTP part, as GRE dev doesn't support SCTP CRC CSUM
    and it will always do checksum for SCTP in sctp_packet_pack()
    when it's not a GSO packet.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/gre.h      | 19 +++++++------------
 net/ipv4/gre_offload.c | 15 +++++++++++++--
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/include/net/gre.h b/include/net/gre.h
index b60f212..4e20970 100644
--- a/include/net/gre.h
+++ b/include/net/gre.h

@@ -106,17 +106,6 @@ static inline __be16 gre_tnl_flags_to_gre_flags(__be16 tflags)
 	return flags;
 }
 
-static inline __sum16 gre_checksum(struct sk_buff *skb)
-{
-	__wsum csum;
-
-	if (skb->ip_summed == CHECKSUM_PARTIAL)
-		csum = lco_csum(skb);
-	else
-		csum = skb_checksum(skb, 0, skb->len, 0);
-	return csum_fold(csum);
-}
-
 static inline void gre_build_header(struct sk_buff *skb, int hdr_len,
 				    __be16 flags, __be16 proto,
 				    __be32 key, __be32 seq)

@@ -146,7 +135,13 @@ static inline void gre_build_header(struct sk_buff *skb, int hdr_len,
 		    !(skb_shinfo(skb)->gso_type &
 		      (SKB_GSO_GRE | SKB_GSO_GRE_CSUM))) {
 			*ptr = 0;
-			*(__sum16 *)ptr = gre_checksum(skb);
+			if (skb->ip_summed == CHECKSUM_PARTIAL) {
+				*(__sum16 *)ptr = csum_fold(lco_csum(skb));
+			} else {
+				skb->ip_summed = CHECKSUM_PARTIAL;
+				skb->csum_start = skb_transport_header(skb) - skb->head;
+				skb->csum_offset = sizeof(*greh);
+			}
 		}
 	}
 }

diff --git a/net/ipv4/gre_offload.c b/net/ipv4/gre_offload.c
index 10bc49b..1121a9d 100644
--- a/net/ipv4/gre_offload.c
+++ b/net/ipv4/gre_offload.c

@@ -15,10 +15,10 @@ static struct sk_buff *gre_gso_segment(struct sk_buff *skb,
 				       netdev_features_t features)
 {
 	int tnl_hlen = skb_inner_mac_header(skb) - skb_transport_header(skb);
+	bool need_csum, offload_csum, gso_partial, need_ipsec;
 	struct sk_buff *segs = ERR_PTR(-EINVAL);
 	u16 mac_offset = skb->mac_header;
 	__be16 protocol = skb->protocol;
-	bool need_csum, gso_partial;
 	u16 mac_len = skb->mac_len;
 	int gre_offset, outer_hlen;
 

@@ -47,6 +47,11 @@ static struct sk_buff *gre_gso_segment(struct sk_buff *skb,
 	if (need_csum)
 		features &= ~NETIF_F_SCTP_CRC;
 
+	need_ipsec = skb_dst(skb) && dst_xfrm(skb_dst(skb));
+	/* Try to offload checksum if possible */
+	offload_csum = !!(need_csum && !need_ipsec &&
+			  (skb->dev->features & NETIF_F_HW_CSUM));
+
 	/* segment inner packet. */
 	segs = skb_mac_gso_segment(skb, features);
 	if (IS_ERR_OR_NULL(segs)) {

@@ -100,7 +105,13 @@ static struct sk_buff *gre_gso_segment(struct sk_buff *skb,
 		}
 
 		*(pcsum + 1) = 0;
-		*pcsum = gso_make_checksum(skb, 0);
+		if (skb->encapsulation || !offload_csum) {
+			*pcsum = gso_make_checksum(skb, 0);
+		} else {
+			skb->ip_summed = CHECKSUM_PARTIAL;
+			skb->csum_start = skb_transport_header(skb) - skb->head;
+			skb->csum_offset = sizeof(*greh);
+		}
 	} while ((skb = skb->next));
 out:
 	return segs;

-- 
2.1.0

NETIF_F_IP|IPV6_CSUM feature flag indicates UDP and TCP csum offload
while NETIF_F_HW_CSUM feature flag indicates ip generic csum offload
for HW, which includes not only for TCP/UDP csum, but also for other
protocols' csum like GRE's.

However, in skb_csum_hwoffload_help() it only checks features against
NETIF_F_CSUM_MASK(NETIF_F_HW|IP|IPV6_CSUM). So if it's a non TCP/UDP
packet and the features doesn't support NETIF_F_HW_CSUM, but supports
NETIF_F_IP|IPV6_CSUM only, it would still return 0 and leave the HW
to do csum.

This patch is to support ip generic csum processing by checking
NETIF_F_HW_CSUM for all protocols, and check (NETIF_F_IP_CSUM |
NETIF_F_IPV6_CSUM) only for TCP and UDP.

Note that we're using skb->csum_offset to check if it's a TCP/UDP
proctol, this might be fragile. However, as Alex said, for now we
only have a few L4 protocols that are requesting Tx csum offload,
we'd better fix this until a new protocol comes with a same csum
offset.

v1->v2:
  - not extend skb->csum_not_inet, but use skb->csum_offset to tell
    if it's an UDP/TCP csum packet.
v2->v3:
  - add a note in the changelog, as Willem suggested.

Suggested-by: Alexander Duyck <redacted>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/core/dev.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 6df3f1b..aae116d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -3621,7 +3621,18 @@ int skb_csum_hwoffload_help(struct sk_buff *skb,
 		return !!(features & NETIF_F_SCTP_CRC) ? 0 :
 			skb_crc32c_csum_help(skb);
 
-	return !!(features & NETIF_F_CSUM_MASK) ? 0 : skb_checksum_help(skb);
+	if (features & NETIF_F_HW_CSUM)
+		return 0;
+
+	if (features & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)) {
+		switch (skb->csum_offset) {
+		case offsetof(struct tcphdr, check):
+		case offsetof(struct udphdr, check):
+			return 0;
+		}
+	}
+
+	return skb_checksum_help(skb);
 }
 EXPORT_SYMBOL(skb_csum_hwoffload_help);
 

-- 
2.1.0

On Thu, Jan 28, 2021 at 4:29 AM Xin Long [off-list ref] wrote:

Should this check the specific feature flag against skb->protocol? I
don't know if there are actually instances that only support one of
the two flags.

+               switch (skb->csum_offset) {
+               case offsetof(struct tcphdr, check):
+               case offsetof(struct udphdr, check):
+                       return 0;
+               }
+       }
+
+       return skb_checksum_help(skb);
 }
 EXPORT_SYMBOL(skb_csum_hwoffload_help);

--
2.1.0

On Thu, Jan 28, 2021 at 6:07 AM Willem de Bruijn
[off-list ref] wrote:

On Thu, Jan 28, 2021 at 4:29 AM Xin Long [off-list ref] wrote:

quoted

NETIF_F_IP|IPV6_CSUM feature flag indicates UDP and TCP csum offload
while NETIF_F_HW_CSUM feature flag indicates ip generic csum offload
for HW, which includes not only for TCP/UDP csum, but also for other
protocols' csum like GRE's.

However, in skb_csum_hwoffload_help() it only checks features against
NETIF_F_CSUM_MASK(NETIF_F_HW|IP|IPV6_CSUM). So if it's a non TCP/UDP
packet and the features doesn't support NETIF_F_HW_CSUM, but supports
NETIF_F_IP|IPV6_CSUM only, it would still return 0 and leave the HW
to do csum.

This patch is to support ip generic csum processing by checking
NETIF_F_HW_CSUM for all protocols, and check (NETIF_F_IP_CSUM |
NETIF_F_IPV6_CSUM) only for TCP and UDP.

Note that we're using skb->csum_offset to check if it's a TCP/UDP
proctol, this might be fragile. However, as Alex said, for now we
only have a few L4 protocols that are requesting Tx csum offload,
we'd better fix this until a new protocol comes with a same csum
offset.

v1->v2:
  - not extend skb->csum_not_inet, but use skb->csum_offset to tell
    if it's an UDP/TCP csum packet.
v2->v3:
  - add a note in the changelog, as Willem suggested.

Suggested-by: Alexander Duyck <redacted>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/core/dev.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 6df3f1b..aae116d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -3621,7 +3621,18 @@ int skb_csum_hwoffload_help(struct sk_buff *skb,
                return !!(features & NETIF_F_SCTP_CRC) ? 0 :
                        skb_crc32c_csum_help(skb);

-       return !!(features & NETIF_F_CSUM_MASK) ? 0 : skb_checksum_help(skb);
+       if (features & NETIF_F_HW_CSUM)
+               return 0;
+
+       if (features & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)) {

Should this check the specific feature flag against skb->protocol? I
don't know if there are actually instances that only support one of
the two flags.

The issue is at a certain point we start excluding devices that were
previously working.

All this patch is really doing is using the checksum offset to
identify the cases that were previously UDP or TCP offloads and
letting those through with the legacy path, while any offsets that are
not those two, such as the GRE checksum will now have to be explicitly
caught by the NETIF_F_HW_CSUM case and not accepted by the other
cases.

On Thu, Jan 28, 2021 at 2:46 PM Alexander Duyck
[off-list ref] wrote:

On Thu, Jan 28, 2021 at 6:07 AM Willem de Bruijn
[off-list ref] wrote:

quoted

On Thu, Jan 28, 2021 at 4:29 AM Xin Long [off-list ref] wrote:

quoted

NETIF_F_IP|IPV6_CSUM feature flag indicates UDP and TCP csum offload
while NETIF_F_HW_CSUM feature flag indicates ip generic csum offload
for HW, which includes not only for TCP/UDP csum, but also for other
protocols' csum like GRE's.

However, in skb_csum_hwoffload_help() it only checks features against
NETIF_F_CSUM_MASK(NETIF_F_HW|IP|IPV6_CSUM). So if it's a non TCP/UDP
packet and the features doesn't support NETIF_F_HW_CSUM, but supports
NETIF_F_IP|IPV6_CSUM only, it would still return 0 and leave the HW
to do csum.

This patch is to support ip generic csum processing by checking
NETIF_F_HW_CSUM for all protocols, and check (NETIF_F_IP_CSUM |
NETIF_F_IPV6_CSUM) only for TCP and UDP.

Note that we're using skb->csum_offset to check if it's a TCP/UDP
proctol, this might be fragile. However, as Alex said, for now we
only have a few L4 protocols that are requesting Tx csum offload,
we'd better fix this until a new protocol comes with a same csum
offset.

v1->v2:
  - not extend skb->csum_not_inet, but use skb->csum_offset to tell
    if it's an UDP/TCP csum packet.
v2->v3:
  - add a note in the changelog, as Willem suggested.

Suggested-by: Alexander Duyck <redacted>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/core/dev.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 6df3f1b..aae116d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -3621,7 +3621,18 @@ int skb_csum_hwoffload_help(struct sk_buff *skb,
                return !!(features & NETIF_F_SCTP_CRC) ? 0 :
                        skb_crc32c_csum_help(skb);

-       return !!(features & NETIF_F_CSUM_MASK) ? 0 : skb_checksum_help(skb);
+       if (features & NETIF_F_HW_CSUM)
+               return 0;
+
+       if (features & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)) {

Should this check the specific feature flag against skb->protocol? I
don't know if there are actually instances that only support one of
the two flags.

The issue is at a certain point we start excluding devices that were
previously working.

All this patch is really doing is using the checksum offset to
identify the cases that were previously UDP or TCP offloads and
letting those through with the legacy path, while any offsets that are
not those two, such as the GRE checksum will now have to be explicitly
caught by the NETIF_F_HW_CSUM case and not accepted by the other
cases.

I understand. But letting through an IPv6 packet to a nic that
advertises NETIF_F_IP_CSUM, but not NETIF_F_IPV6_CSUM, is still
incorrect, right?

On Thu, Jan 28, 2021 at 12:00 PM Willem de Bruijn
[off-list ref] wrote:

On Thu, Jan 28, 2021 at 2:46 PM Alexander Duyck
[off-list ref] wrote:

quoted

On Thu, Jan 28, 2021 at 6:07 AM Willem de Bruijn
[off-list ref] wrote:

quoted

On Thu, Jan 28, 2021 at 4:29 AM Xin Long [off-list ref] wrote:

quoted

NETIF_F_IP|IPV6_CSUM feature flag indicates UDP and TCP csum offload
while NETIF_F_HW_CSUM feature flag indicates ip generic csum offload
for HW, which includes not only for TCP/UDP csum, but also for other
protocols' csum like GRE's.

However, in skb_csum_hwoffload_help() it only checks features against
NETIF_F_CSUM_MASK(NETIF_F_HW|IP|IPV6_CSUM). So if it's a non TCP/UDP
packet and the features doesn't support NETIF_F_HW_CSUM, but supports
NETIF_F_IP|IPV6_CSUM only, it would still return 0 and leave the HW
to do csum.

This patch is to support ip generic csum processing by checking
NETIF_F_HW_CSUM for all protocols, and check (NETIF_F_IP_CSUM |
NETIF_F_IPV6_CSUM) only for TCP and UDP.

Note that we're using skb->csum_offset to check if it's a TCP/UDP
proctol, this might be fragile. However, as Alex said, for now we
only have a few L4 protocols that are requesting Tx csum offload,
we'd better fix this until a new protocol comes with a same csum
offset.

v1->v2:
  - not extend skb->csum_not_inet, but use skb->csum_offset to tell
    if it's an UDP/TCP csum packet.
v2->v3:
  - add a note in the changelog, as Willem suggested.

Suggested-by: Alexander Duyck <redacted>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/core/dev.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 6df3f1b..aae116d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -3621,7 +3621,18 @@ int skb_csum_hwoffload_help(struct sk_buff *skb,
                return !!(features & NETIF_F_SCTP_CRC) ? 0 :
                        skb_crc32c_csum_help(skb);

-       return !!(features & NETIF_F_CSUM_MASK) ? 0 : skb_checksum_help(skb);
+       if (features & NETIF_F_HW_CSUM)
+               return 0;
+
+       if (features & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)) {

Should this check the specific feature flag against skb->protocol? I
don't know if there are actually instances that only support one of
the two flags.

The issue is at a certain point we start excluding devices that were
previously working.

All this patch is really doing is using the checksum offset to
identify the cases that were previously UDP or TCP offloads and
letting those through with the legacy path, while any offsets that are
not those two, such as the GRE checksum will now have to be explicitly
caught by the NETIF_F_HW_CSUM case and not accepted by the other
cases.

I understand. But letting through an IPv6 packet to a nic that
advertises NETIF_F_IP_CSUM, but not NETIF_F_IPV6_CSUM, is still
incorrect, right?

That all depends. The problem is if we are going to look at protocol
we essentially have to work our way through a number of fields and
sort out if there are tunnels or not and if so what the protocol for
the inner headers are and if that is supported. It might make more
sense in that case to look at incorporating a v4/v6 specific check
into netif_skb_features so we could mask off the bit there.

The question i would have is how has this code been working up until
now without that check? If we are broken outright and need to add it
then maybe this should be deemed more of a fix and pushed for net with
the added protocol bit masking added.

On Thu, Jan 28, 2021 at 4:42 PM Alexander Duyck
[off-list ref] wrote:

On Thu, Jan 28, 2021 at 12:00 PM Willem de Bruijn
[off-list ref] wrote:

quoted

On Thu, Jan 28, 2021 at 2:46 PM Alexander Duyck
[off-list ref] wrote:

quoted

On Thu, Jan 28, 2021 at 6:07 AM Willem de Bruijn
[off-list ref] wrote:

quoted

On Thu, Jan 28, 2021 at 4:29 AM Xin Long [off-list ref] wrote:

quoted

NETIF_F_IP|IPV6_CSUM feature flag indicates UDP and TCP csum offload
while NETIF_F_HW_CSUM feature flag indicates ip generic csum offload
for HW, which includes not only for TCP/UDP csum, but also for other
protocols' csum like GRE's.

However, in skb_csum_hwoffload_help() it only checks features against
NETIF_F_CSUM_MASK(NETIF_F_HW|IP|IPV6_CSUM). So if it's a non TCP/UDP
packet and the features doesn't support NETIF_F_HW_CSUM, but supports
NETIF_F_IP|IPV6_CSUM only, it would still return 0 and leave the HW
to do csum.

This patch is to support ip generic csum processing by checking
NETIF_F_HW_CSUM for all protocols, and check (NETIF_F_IP_CSUM |
NETIF_F_IPV6_CSUM) only for TCP and UDP.

Note that we're using skb->csum_offset to check if it's a TCP/UDP
proctol, this might be fragile. However, as Alex said, for now we
only have a few L4 protocols that are requesting Tx csum offload,
we'd better fix this until a new protocol comes with a same csum
offset.

v1->v2:
  - not extend skb->csum_not_inet, but use skb->csum_offset to tell
    if it's an UDP/TCP csum packet.
v2->v3:
  - add a note in the changelog, as Willem suggested.

Suggested-by: Alexander Duyck <redacted>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/core/dev.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 6df3f1b..aae116d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -3621,7 +3621,18 @@ int skb_csum_hwoffload_help(struct sk_buff *skb,
                return !!(features & NETIF_F_SCTP_CRC) ? 0 :
                        skb_crc32c_csum_help(skb);

-       return !!(features & NETIF_F_CSUM_MASK) ? 0 : skb_checksum_help(skb);
+       if (features & NETIF_F_HW_CSUM)
+               return 0;
+
+       if (features & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)) {

Should this check the specific feature flag against skb->protocol? I
don't know if there are actually instances that only support one of
the two flags.

The issue is at a certain point we start excluding devices that were
previously working.

All this patch is really doing is using the checksum offset to
identify the cases that were previously UDP or TCP offloads and
letting those through with the legacy path, while any offsets that are
not those two, such as the GRE checksum will now have to be explicitly
caught by the NETIF_F_HW_CSUM case and not accepted by the other
cases.

I understand. But letting through an IPv6 packet to a nic that
advertises NETIF_F_IP_CSUM, but not NETIF_F_IPV6_CSUM, is still
incorrect, right?

That all depends. The problem is if we are going to look at protocol
we essentially have to work our way through a number of fields and
sort out if there are tunnels or not and if so what the protocol for
the inner headers are and if that is supported. It might make more
sense in that case to look at incorporating a v4/v6 specific check
into netif_skb_features so we could mask off the bit there.

The question i would have is how has this code been working up until
now without that check? If we are broken outright and need to add it
then maybe this should be deemed more of a fix and pushed for net with
the added protocol bit masking added.

Fair enough. I agree that this patch does not make anything worse, as
it actually tightens the rules.

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Thu, 28 Jan 2021 17:18:30 +0800 you wrote:

This patchset it to add ip generic csum processing first in
skb_csum_hwoffload_help() in Patch 1/2 and then add csum
offload support for GRE header in Patch 2/2.

v1->v2:
  - See each patch's changelog.
v2->v3:
  - See the 1st patch.

[...]

Here is the summary with links:
  - [PATCHv3,net-next,1/2] net: support ip generic csum processing in skb_csum_hwoffload_help
    https://git.kernel.org/netdev/net-next/c/62fafcd63139
  - [PATCHv3,net-next,2/2] ip_gre: add csum offload support for gre header
    https://git.kernel.org/netdev/net-next/c/efa1a65c7e19

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html