[RFC PATCH net-next 0/5] net: improve support for SCTP checksums

When NETIF_F_CSUM_MASK bits are all 0 on netdev features, validate_xmit_skb
uses skb_checksum_help to compute 16-bit, 2-complement checksum on non-GSO
skbs having ip_summed equal to CHECKSUM_PARTIAL.
This results in a systematic corruption of SCTP packets, since they need
to be checksummed with crc32c. Moreover, this is done regardless the value
of NETIF_F_SCTP_CRC, so any chance to offload crc32c computation on the 
NIC is lost. Finally, even when at least one bit in NETIF_F_CSUM_MASK is
set on netdev features, validate_xmit_skb skips checksum computation - but
then most NIC drivers can only call skb_checksum_help if their HW can't
offload the checksum computation. Depending on the driver code, this
results in wrong handling of SCTP, leading to:

- packet being dropped
- packet being transmitted with identically-zero checksum
- packet being transmitted with 2-complement checksum instead of crc32c

This series tries to address the above issue, by providing:
- the possibility to compute crc32c on skbs in Linux net core [patch 1]
- skb_sctp_csum_help, a function sharing common code with the original
  skb_checksum_help, that performs SW checksumming for skbs using crc32c
  [patch 2 and patch 3]
- skb_csum_hwoffload_help, called by validate xmit skb to perform SW
  checksumming using the correct algorithm based on the value of IP
  protocol number and netdev features bitmask [patch 4]
- an update to Linux documentation [patch 5]

Davide Caratti (5):
  skbuff: add stub to help computing crc32c on SCTP packets
  net: split skb_checksum_help
  net: introduce skb_sctp_csum_help
  net: more accurate checksumming in validate_xmit_skb
  Documentation: add description of skb_sctp_csum_help

 Documentation/networking/checksum-offloads.txt |   9 +-
 include/linux/netdevice.h                      |   1 +
 include/linux/skbuff.h                         |   5 +-
 net/core/dev.c                                 | 132 +++++++++++++++++++++----
 net/core/skbuff.c                              |  20 ++++
 net/sctp/offload.c                             |   7 ++
 6 files changed, 151 insertions(+), 23 deletions(-)

-- 
2.7.4

skb_checksum_help is designed to compute the Internet Checksum only. To
avoid duplicating code when other checksumming algorithms (e.g. crc32c)
are used, separate common part from RFC1624-specific part.

Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Davide Caratti <redacted>
---
 net/core/dev.c | 51 +++++++++++++++++++++++++++++++++++----------------
 1 file changed, 35 insertions(+), 16 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index ad5959e..6742160 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -2532,13 +2532,36 @@ static void skb_warn_bad_offload(const struct sk_buff *skb)
 	     skb_shinfo(skb)->gso_type, skb->ip_summed);
 }
 
-/*
- * Invalidate hardware checksum when packet is to be mangled, and
+/* compute 16-bit RFC1624 checksum and store it at skb->data + offset */
+static int skb_rfc1624_csum(struct sk_buff *skb, int offset)
+{
+	__wsum csum;
+	int ret = 0;
+
+	csum = skb_checksum(skb, offset, skb->len - offset, 0);
+
+	offset += skb->csum_offset;
+	BUG_ON(offset + sizeof(__sum16) > skb_headlen(skb));
+
+	if (skb_cloned(skb) &&
+	    !skb_clone_writable(skb, offset + sizeof(__sum16))) {
+		ret = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
+		if (ret)
+			goto out;
+	}
+	*(__sum16 *)(skb->data + offset) = csum_fold(csum) ?: CSUM_MANGLED_0;
+out:
+	return ret;
+}
+
+/* Invalidate hardware checksum when packet is to be mangled, and
  * complete checksum manually on outgoing path.
+ *    @skb - buffer that needs checksum
+ *    @csum_algo(skb, offset) - function used to compute the checksum
  */
-int skb_checksum_help(struct sk_buff *skb)
+static int __skb_checksum_help(struct sk_buff *skb,
+			       int (*csum_algo)(struct sk_buff *, int))
 {
-	__wsum csum;
 	int ret = 0, offset;
 
 	if (skb->ip_summed == CHECKSUM_COMPLETE)

@@ -2560,24 +2583,20 @@ int skb_checksum_help(struct sk_buff *skb)
 
 	offset = skb_checksum_start_offset(skb);
 	BUG_ON(offset >= skb_headlen(skb));
-	csum = skb_checksum(skb, offset, skb->len - offset, 0);
-
-	offset += skb->csum_offset;
-	BUG_ON(offset + sizeof(__sum16) > skb_headlen(skb));
-
-	if (skb_cloned(skb) &&
-	    !skb_clone_writable(skb, offset + sizeof(__sum16))) {
-		ret = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
-		if (ret)
-			goto out;
-	}
 
-	*(__sum16 *)(skb->data + offset) = csum_fold(csum) ?: CSUM_MANGLED_0;
+	ret = csum_algo(skb, offset);
+	if (ret)
+		goto out;
 out_set_summed:
 	skb->ip_summed = CHECKSUM_NONE;
 out:
 	return ret;
 }
+
+int skb_checksum_help(struct sk_buff *skb)
+{
+	return __skb_checksum_help(skb, skb_rfc1624_csum);
+}
 EXPORT_SYMBOL(skb_checksum_help);
 
 __be16 skb_network_protocol(struct sk_buff *skb, int *depth)

-- 
2.7.4

skb_sctp_csum_help is like skb_checksum_help, but it is designed for
checksumming SCTP packets using crc32c (see RFC3309), provided that
sctp.ko has been loaded before. In case sctp.ko is not loaded, invoking
skb_sctp_csum_help() on a skb results in the following printout:

sk_buff: attempt to compute crc32c without sctp.ko

Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Davide Caratti <redacted>
---
 include/linux/netdevice.h |  1 +
 include/linux/skbuff.h    |  3 ++-
 net/core/dev.c            | 29 +++++++++++++++++++++++++++++
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 3868c32..9d72824 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h

@@ -3902,6 +3902,7 @@ void netdev_rss_key_fill(void *buffer, size_t len);
 
 int dev_get_nest_level(struct net_device *dev);
 int skb_checksum_help(struct sk_buff *skb);
+int skb_sctp_csum_help(struct sk_buff *skb);
 struct sk_buff *__skb_gso_segment(struct sk_buff *skb,
 				  netdev_features_t features, bool tx_path);
 struct sk_buff *skb_mac_gso_segment(struct sk_buff *skb,

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 44fc804..91b4e22 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -192,7 +192,8 @@
  *     accordingly. Note the there is no indication in the skbuff that the
  *     CHECKSUM_PARTIAL refers to an SCTP checksum, a driver that supports
  *     both IP checksum offload and SCTP CRC offload must verify which offload
- *     is configured for a packet presumably by inspecting packet headers.
+ *     is configured for a packet presumably by inspecting packet headers; in
+ *     case, skb_sctp_csum_help is provided to compute CRC on SCTP packets.
  *
  *   NETIF_F_FCOE_CRC - This feature indicates that a device is capable of
  *     offloading the FCOE CRC in a packet. To perform this offload the stack

diff --git a/net/core/dev.c b/net/core/dev.c
index 6742160..45cee84 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -2554,6 +2554,29 @@ static int skb_rfc1624_csum(struct sk_buff *skb, int offset)
 	return ret;
 }
 
+/* compute 32-bit RFC3309 checksum and store it at skb->data + offset */
+static int skb_rfc3309_csum(struct sk_buff *skb, int offset)
+{
+	__le32 crc32c_csum;
+	int ret = 0;
+
+	crc32c_csum = cpu_to_le32(~__skb_checksum(skb, offset,
+						  skb->len - offset, ~(__u32)0,
+						  sctp_csum_stub));
+	offset += skb->csum_offset;
+	BUG_ON((offset + sizeof(__le32)) > skb_headlen(skb));
+
+	if (skb_cloned(skb) &&
+	    !skb_clone_writable(skb, offset + sizeof(__le32))) {
+		ret = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
+		if (ret)
+			goto out;
+	}
+	*(__le32 *)(skb->data + offset) = crc32c_csum;
+out:
+	return ret;
+}
+
 /* Invalidate hardware checksum when packet is to be mangled, and
  * complete checksum manually on outgoing path.
  *    @skb - buffer that needs checksum

@@ -2599,6 +2622,12 @@ int skb_checksum_help(struct sk_buff *skb)
 }
 EXPORT_SYMBOL(skb_checksum_help);
 
+int skb_sctp_csum_help(struct sk_buff *skb)
+{
+	return __skb_checksum_help(skb, skb_rfc3309_csum);
+}
+EXPORT_SYMBOL(skb_sctp_csum_help);
+
 __be16 skb_network_protocol(struct sk_buff *skb, int *depth)
 {
 	__be16 type = skb->protocol;

-- 
2.7.4

introduce skb_csum_hwoffload_help and use it as a replacement for
skb_checksum_help in validate_xmit_skb, to compute checksum using crc32c or
2-complement Internet Checksum (or leave the packet unchanged and let the
NIC do the checksum), depending on netdev checksum offloading capabilities
and on presence of IPPROTO_SCTP as protocol number in IPv4/IPv6 header.

Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Davide Caratti <redacted>
---
 net/core/dev.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 45cee84..f8cb3ba 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -140,6 +140,7 @@
 #include <linux/hrtimer.h>
 #include <linux/netfilter_ingress.h>
 #include <linux/crash_dump.h>
+#include <linux/sctp.h>
 
 #include "net-sysfs.h"
 

@@ -2960,6 +2961,54 @@ static struct sk_buff *validate_xmit_vlan(struct sk_buff *skb,
 	return skb;
 }
 
+static int skb_csum_hwoffload_help(struct sk_buff *skb,
+				   netdev_features_t features)
+{
+	bool encap = skb->encapsulation;
+	unsigned int offset = 0;
+	__be16 protocol;
+
+	if (likely((features & (NETIF_F_SCTP_CRC | NETIF_F_CSUM_MASK)) ==
+	    (NETIF_F_SCTP_CRC | NETIF_F_CSUM_MASK)))
+		return 0;
+
+	if (skb->csum_offset != offsetof(struct sctphdr, checksum))
+		goto inet_csum;
+
+	if (encap) {
+		protocol = skb->inner_protocol;
+		if (skb->inner_protocol_type == ENCAP_TYPE_IPPROTO)
+			switch (protocol) {
+			case IPPROTO_IPV6:
+				protocol = ntohs(ETH_P_IPV6);
+				break;
+			case IPPROTO_IP:
+				protocol = ntohs(ETH_P_IP);
+				break;
+			default:
+				goto inet_csum;
+			}
+	} else {
+		protocol = vlan_get_protocol(skb);
+	}
+	switch (protocol) {
+	case ntohs(ETH_P_IP):
+		if ((encap ? inner_ip_hdr(skb) : ip_hdr(skb))->protocol ==
+		    IPPROTO_SCTP)
+			goto sctp_csum;
+		break;
+	case ntohs(ETH_P_IPV6):
+		if (ipv6_find_hdr(skb, &offset, IPPROTO_SCTP, NULL, NULL) ==
+		    IPPROTO_SCTP)
+			goto sctp_csum;
+		break;
+	}
+inet_csum:
+	return !(features & NETIF_F_CSUM_MASK) ? skb_checksum_help(skb) : 0;
+sctp_csum:
+	return !(features & NETIF_F_SCTP_CRC) ? skb_sctp_csum_help(skb) : 0;
+}
+
 static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device *dev)
 {
 	netdev_features_t features;

@@ -2995,8 +3044,7 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device
 			else
 				skb_set_transport_header(skb,
 							 skb_checksum_start_offset(skb));
-			if (!(features & NETIF_F_CSUM_MASK) &&
-			    skb_checksum_help(skb))
+			if (skb_csum_hwoffload_help(skb, features))
 				goto out_kfree_skb;
 		}
 	}

-- 
2.7.4

Add description of skb_sctp_csum_help in networking/checksum-offload.txt;
while at it, remove reference to skb_csum_off_chk* functions, since they
are not present anymore in Linux since commit cf53b1da73bd ('Revert "net:
Add driver helper functions to determine checksum"').

Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Davide Caratti <redacted>
---
 Documentation/networking/checksum-offloads.txt | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/networking/checksum-offloads.txt b/Documentation/networking/checksum-offloads.txt
index 56e3686..cb7a7e5 100644
--- a/Documentation/networking/checksum-offloads.txt
+++ b/Documentation/networking/checksum-offloads.txt

@@ -49,9 +49,9 @@ A driver declares its offload capabilities in netdev->hw_features; see
  and csum_offset given in the SKB; if it tries to deduce these itself in
  hardware (as some NICs do) the driver should check that the values in the
  SKB match those which the hardware will deduce, and if not, fall back to
- checksumming in software instead (with skb_checksum_help or one of the
- skb_csum_off_chk* functions as mentioned in include/linux/skbuff.h).  This
- is a pain, but that's what you get when hardware tries to be clever.
+ checksumming in software instead (with skb_checksum_help or
+ skb_sctp_csum_help functions as mentioned in include/linux/skbuff.h).
+ This is a pain, but that's what you get when hardware tries to be clever.
 
 The stack should, for the most part, assume that checksum offload is
  supported by the underlying device.  The only place that should check is

@@ -60,7 +60,8 @@ The stack should, for the most part, assume that checksum offload is
  may include other offloads besides TX Checksum Offload) and, if they are
  not supported or enabled on the device (determined by netdev->features),
  performs the corresponding offload in software.  In the case of TX
- Checksum Offload, that means calling skb_checksum_help(skb).
+ Checksum Offload, that means calling skb_sctp_csum_help(skb) for SCTP
+ packets, and skb_checksum_help(skb) for other packets.
 
 
 LCO: Local Checksum Offload

-- 
2.7.4

sctp_compute_checksum requires crc32c symbol (provided by libcrc32c), so
it can't be used in net core. Like it has been done previously with other
symbols (e.g. ipv6_dst_lookup), introduce a stub struct skb_checksum_ops
to allow computation of SCTP checksum in net core after sctp.ko (and thus
libcrc32c) has been loaded.

Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Davide Caratti <redacted>
---
 include/linux/skbuff.h |  2 ++
 net/core/skbuff.c      | 20 ++++++++++++++++++++
 net/sctp/offload.c     |  7 +++++++
 3 files changed, 29 insertions(+)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 6f63b7e..44fc804 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -3119,6 +3119,8 @@ struct skb_checksum_ops {
 	__wsum (*combine)(__wsum csum, __wsum csum2, int offset, int len);
 };
 
+extern const struct skb_checksum_ops *sctp_csum_stub __read_mostly;
+
 __wsum __skb_checksum(const struct sk_buff *skb, int offset, int len,
 		      __wsum csum, const struct skb_checksum_ops *ops);
 __wsum skb_checksum(const struct sk_buff *skb, int offset, int len,

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index f8dbe4a..60e9963 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -2235,6 +2235,26 @@ __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
 }
 EXPORT_SYMBOL(skb_copy_and_csum_bits);
 
+static __wsum warn_sctp_csum_update(const void *buff, int len, __wsum sum)
+{
+	net_warn_ratelimited("attempt to compute crc32c without sctp.ko\n");
+	return 0;
+}
+
+static __wsum warn_sctp_csum_combine(__wsum csum, __wsum csum2,
+					 int offset, int len)
+{
+	net_warn_ratelimited("attempt to compute crc32c without sctp.ko\n");
+	return 0;
+}
+
+const struct skb_checksum_ops *sctp_csum_stub __read_mostly =
+	&(struct skb_checksum_ops) {
+	.update  = warn_sctp_csum_update,
+	.combine = warn_sctp_csum_combine,
+};
+EXPORT_SYMBOL(sctp_csum_stub);
+
  /**
  *	skb_zerocopy_headlen - Calculate headroom needed for skb_zerocopy()
  *	@from: source buffer

diff --git a/net/sctp/offload.c b/net/sctp/offload.c
index 7e869d0..1c9c548 100644
--- a/net/sctp/offload.c
+++ b/net/sctp/offload.c

@@ -98,6 +98,12 @@ static const struct net_offload sctp6_offload = {
 	},
 };
 
+static const struct skb_checksum_ops *sctp_csum_ops __read_mostly =
+	&(struct skb_checksum_ops) {
+	.update  = sctp_csum_update,
+	.combine = sctp_csum_combine,
+};
+
 int __init sctp_offload_init(void)
 {
 	int ret;

@@ -110,6 +116,7 @@ int __init sctp_offload_init(void)
 	if (ret)
 		goto ipv4;
 
+	sctp_csum_stub = sctp_csum_ops;
 	return ret;
 
 ipv4:

-- 
2.7.4

On Mon, Jan 23, 2017 at 8:52 AM, Davide Caratti [off-list ref] wrote:

skb_checksum_help is specific to the Internet checksum. For instance,
CHECKSUM_COMPLETE can _only_ refer to Internet checksum calculation
nothing else will work. Checksums and CRCs are very different things
with very different processing. They are not interchangeable, have
very different properties, and hence it is a mistake to try to shoe
horn things so that they use a common infrastructure.

It might make sense to create some CRC helper functions, but last time
I checked there are so few users of CRC in skbufs I'm not even sure
that would make sense.

Tom

From: Tom Herbert

Sent: 23 January 2017 21:00

..

skb_checksum_help is specific to the Internet checksum. For instance,
CHECKSUM_COMPLETE can _only_ refer to Internet checksum calculation
nothing else will work. Checksums and CRCs are very different things
with very different processing. They are not interchangeable, have
very different properties, and hence it is a mistake to try to shoe
horn things so that they use a common infrastructure.

It might make sense to create some CRC helper functions, but last time
I checked there are so few users of CRC in skbufs I'm not even sure
that would make sense.

I can imagine horrid things happening if someone tries to encapsulate
SCTP/IP in UDP (or worse UDP/IP in SCTP).

For UDP in UDP I suspect that CHECKSUM_COMPLETE on an inner UDP packet
allows the outer checksum be calculated by ignoring the inner packet
(since it sums to zero).
This just isn't true if SCTP is involved.
There are tricks to generate a crc of a longer packet, but they'd only
work for SCTP in SCTP.

For non-encapsulated packets it is a different matter.

	David

hello Tom and David,

thank you for the attention.

From: Tom Herbert

quoted

Sent: 23 January 2017 21:00

..

quoted

skb_checksum_help is specific to the Internet checksum. For instance,
CHECKSUM_COMPLETE can _only_ refer to Internet checksum calculation
nothing else will work. Checksums and CRCs are very different things
with very different processing. They are not interchangeable, have
very different properties, and hence it is a mistake to try to shoe
horn things so that they use a common infrastructure.

true, we don't need to test CHECKSUM_COMPLETE on skbs carrying SCTP.
So maybe we can simply replace patches 2/5 and 3/5 with the smaller one at
the bottom of this message.

quoted

It might make sense to create some CRC helper functions, but last time
I checked there are so few users of CRC in skbufs I'm not even sure
that would make sense.

This is exactly the cause of issues I see with SCTP. These packets can be
wrongly checksummed using skb_checksum_help, or simply not checksummed at
all; and in both cases, the packet goes out from the NIC with wrong L4
checksum.

For example: there are scenarios, even the trivial one below, where skb
carrying SCTP packets are wrongly checksummed, because the originating
socket read NETIF_F_SCTP_CRC bit in the underlying device features.
Then, after the kernel forwards the skb, the final transmission
happens on another device where CRC offload is not available: this
typically leads to bad checksums on transmitted SCTP packets.



namespace 1 |                   namespace 2
            |
            |                      br0
            |         +------- Linux bridge -------+
            |         |                            |
            |         V                            V
vethA <-----------> vethB                        eth0
            |
            |

when a socket bound to vethA in namespace 1 generates an INIT packet,
it's not checksummed since veth devices have NETIF_F_SCTP_CRC set [1].
Then, after vethB receives the packet in namespace 2, linux bridge
forwards it to eth0, and (depending on eth0 driver code), it will be
transmitted with wrong CRC32c or simply dropped.

On Tue, 2017-01-24 at 16:35 +0000, David Laight wrote:

I can imagine horrid things happening if someone tries to encapsulate
SCTP/IP in UDP (or worse UDP/IP in SCTP).

For UDP in UDP I suspect that CHECKSUM_COMPLETE on an inner UDP packet
allows the outer checksum be calculated by ignoring the inner packet
(since it sums to zero).
This just isn't true if SCTP is involved.
There are tricks to generate a crc of a longer packet, but they'd only
work for SCTP in SCTP.

For non-encapsulated packets it is a different matter.

If we limit the scope to skbs having ip_summed equal to CHECKSUM_PARTIAL,
like it's done in patch 4, we only need checksumming the packet starting
from csum_start to its end, and copy the computed value to csum_offset.
The difficult thing is discriminating skbs that need CRC32c, namely SCTP,
from the rest of the traffic (that will likely be checksummed by
skb_checksum_help).

Currently, the only way to fix wrong CRCs in the scenario above is to
configure tc filter with "csum" action on eth0 egress, to compensate the
missing capability of eth0 driver to deal with SCTP packets having
ip_summed equal to CHECKSUM_PARTIAL [2].

Patch 4 in the series is an attempt to solve the issue, both for
encapsulated and non-encapsulated skbs, calling skb_csum_hwoffload_help()
inside validate_xmit_skb. In order to look for unchecksummed SCTP packets,
I took inspiration from a Linux-4.4 commit (6ae23ad36253 "net: Add driver
helper functions ...) to implement skb_csum_hwoffload_help, then I called
it in validate_xmit_skb() to fix situations that can't be recovered by the
NIC driver (it's the case where NETIF_F_CSUM_MASK bits are all zero).

Today most NICs can provide at least HW offload for Internet Checksum:
that's why I'm a bit doubtful if it's ok to spend extra CPU cycles in
validate_xmit_skb() to ensure correct CRC in some scenarios. 
Since this issue affects some (not all) NICs, maybe it's better to drop
patch 4, or part of it, and provide a fix for individual drivers that
don't currently handle non-checksummed SCTP packets. But to do that, we
need at least patch 1 and the small code below.

------------------- 8< --------------------------

--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -200,7 +200,8 @@

  *     accordingly. Note the there is no indication in the skbuff that the
  *     CHECKSUM_PARTIAL refers to an FCOE checksum, a driver that supports
  *     both IP checksum offload and FCOE CRC offload must verify which offload
- *     is configured for a packet presumably by inspecting packet headers.
+ *     is configured for a packet presumably by inspecting packet headers; in
+ *     case, skb_sctp_csum_help is provided to compute CRC on SCTP packets.
  *
  * E. Checksumming on output with GSO.
  *

diff --git a/net/core/dev.c b/net/core/dev.c
index ad5959e..fa9be6d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -2580,6 +2580,42 @@ int skb_checksum_help(struct sk_buff *skb)

 }
 EXPORT_SYMBOL(skb_checksum_help);
 
+int skb_sctp_csum_help(struct sk_buff *skb)
+{
+	__le32 crc32c_csum;
+	int ret = 0, offset;
+
+	if (skb->ip_summed != CHECKSUM_PARTIAL)
+		goto out;
+	if (unlikely(skb_is_gso(skb)))
+		goto out;
+	if (skb_has_shared_frag(skb)) {
+		ret = __skb_linearize(skb);
+		if (ret)
+			goto out;
+	}
+
+	offset = skb_checksum_start_offset(skb);
+	crc32c_csum = cpu_to_le32(~__skb_checksum(skb, offset,
+				  skb->len - offset, ~(__u32)0,
+				  sctp_csum_stub));
+
+	offset += offsetof(struct sctphdr, checksum);
+	BUG_ON(offset >= skb_headlen(skb));
+
+	if (skb_cloned(skb) &&
+	    !skb_clone_writable(skb, offset + sizeof(__le32))) {
+		ret = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
+		if (ret)
+			goto out;
+	}
+	*(__le32 *)(skb->data + offset) = crc32c_csum;
+	skb->ip_summed = CHECKSUM_NONE;
+out:
+	return ret;
+}
+EXPORT_SYMBOL(skb_sctp_csum_help);
+
 __be16 skb_network_protocol(struct sk_buff *skb, int *depth)
 {
 	__be16 type = skb->protocol;
-- 
2.7.4
------------------- >8 --------------------------

Thank you again for paying attention to this, and I would appreciate if
you share your opinion.

Notes:

[1] see commit c80fafbbb59e ("veth: sctp: add NETIF_F_SCTP_CRC to device
features")
[2] see commit c008b33f3ef ("net/sched: act_csum: compute crc32c on SCTP
packets").  We could also turn off NETIF_F_SCTP_CRC bit from vethA, but
this would generate useless crc32c calculations if the SCTP server is not
outside the physical node (e.g. it is bound to br0), leading to a
throughput degradation.

From: Davide Caratti

Sent: 02 February 2017 15:07

quoted

From: Tom Herbert

quoted

Sent: 23 January 2017 21:00

..

quoted

skb_checksum_help is specific to the Internet checksum. For instance,
CHECKSUM_COMPLETE can _only_ refer to Internet checksum calculation
nothing else will work. Checksums and CRCs are very different things
with very different processing. They are not interchangeable, have
very different properties, and hence it is a mistake to try to shoe
horn things so that they use a common infrastructure.

true, we don't need to test CHECKSUM_COMPLETE on skbs carrying SCTP.
So maybe we can simply replace patches 2/5 and 3/5 with the smaller one at
the bottom of this message.

I have to admit to not knowing exactly what the CHECKSUM_xxx flags actually mean.
I have a good idea about what the intention is though.

...

On Tue, 2017-01-24 at 16:35 +0000, David Laight wrote:

quoted

I can imagine horrid things happening if someone tries to encapsulate
SCTP/IP in UDP (or worse UDP/IP in SCTP).

For UDP in UDP I suspect that CHECKSUM_COMPLETE on an inner UDP packet
allows the outer checksum be calculated by ignoring the inner packet
(since it sums to zero).
This just isn't true if SCTP is involved.
There are tricks to generate a crc of a longer packet, but they'd only
work for SCTP in SCTP.

For non-encapsulated packets it is a different matter.

If we limit the scope to skbs having ip_summed equal to CHECKSUM_PARTIAL,
like it's done in patch 4, we only need checksumming the packet starting
from csum_start to its end, and copy the computed value to csum_offset.
The difficult thing is discriminating skbs that need CRC32c, namely SCTP,
from the rest of the traffic (that will likely be checksummed by
skb_checksum_help).

...

I'm guessing that the SCTP code only sets CHECKSUM_PARTIAL (and doesn't
perform the checksum) if it somehow knows that the target interface
supports CRC32c checksums.

I'd put the onus on any such interface to perform the checksum (and
set CHECKSUM_COMPLETE (or is it UNNECESSARY?) before passing the 
message onto an interface that doesn't advertise CRC32 support.

You certainly don't want to have to go through all the ethernet drivers!

I don't think you really want to linearize the packet.
...

	David

On Thu, Feb 2, 2017 at 7:07 AM, Davide Caratti [off-list ref] wrote:

hello Tom and David,

thank you for the attention.

quoted

From: Tom Herbert

quoted

Sent: 23 January 2017 21:00

..

quoted

skb_checksum_help is specific to the Internet checksum. For instance,
CHECKSUM_COMPLETE can _only_ refer to Internet checksum calculation
nothing else will work. Checksums and CRCs are very different things
with very different processing. They are not interchangeable, have
very different properties, and hence it is a mistake to try to shoe
horn things so that they use a common infrastructure.

true, we don't need to test CHECKSUM_COMPLETE on skbs carrying SCTP.
So maybe we can simply replace patches 2/5 and 3/5 with the smaller one at
the bottom of this message.

quoted

quoted

It might make sense to create some CRC helper functions, but last time
I checked there are so few users of CRC in skbufs I'm not even sure
that would make sense.

This is exactly the cause of issues I see with SCTP. These packets can be
wrongly checksummed using skb_checksum_help, or simply not checksummed at
all; and in both cases, the packet goes out from the NIC with wrong L4
checksum.

Okay, makes sense. Please consider doing the following:

- Add a bit to skbuf called something like "csum_not_inet". When
ip_summed == CHECKSUM_PARTIAL and this bit is set that means we are
dealing with something other than an Internet checksum.
- At the top of skb_checksum_help (or maybe before the point where the
inet specific checksum start begins do something like:

   if (unlikely(skb->csum_not_inet))
       return skb_checksum_help_not_inet(...);

   The rest of skb_checksum_help should remained unchanged.

- Add a description of the new bit and how skb_checksum_help can work
to the comments for CHECKSUM_PARTIAL in skbuff.h
- Add FCOE to the list of protocol that can set CHECKSUM_UNNECESSARY
for a CRC/csum
- Add a note to CHECKSUM_COMPLETE section that it can only refer to an
Internet checksum

Thanks,
Tom

On Mon, Feb 27, 2017 at 5:39 AM, Davide Caratti [off-list ref] wrote:

On Mon, 2017-01-23 at 12:59 -0800, Tom Herbert wrote:

quoted

quoted

quoted

quoted

It might make sense to create some CRC helper functions, but last time
I checked there are so few users of CRC in skbufs I'm not even sure
that would make sense.

hello Tom and David,

after some (thinking + testing) time, I'm going to re-post this RFC as v2 with
some feedbacks. Thank you in advance for looking at it!

On Thu, 2017-02-02 at 10:08 -0800, Tom Herbert wrote:

quoted

On Thu, 2017-02-02 at 16:07 +0100, Davide Caratti wrote:

quoted

This is exactly the cause of issues I see with SCTP. These packets can be
wrongly checksummed using skb_checksum_help, or simply not checksummed at
all; and in both cases, the packet goes out from the NIC with wrong L4
checksum.

Okay, makes sense. Please consider doing the following:

- Add a bit to skbuf called something like "csum_not_inet". When
ip_summed == CHECKSUM_PARTIAL and this bit is set that means we are
dealing with something other than an Internet checksum.

Ok, done. Another solution would be to extend possible values of
skb->ip_summed, and define a new value suitable for identifying
not-yet-checksummed SCTP packets (something like CRC32C_PARTIAL). Since
skb->ip_summed is 2-bit wide, the overall effect on skb metadata is the
same as adding skb->csum_not_inet [1].

quoted

- At the top of skb_checksum_help (or maybe before the point where the
inet specific checksum start begins do something like:

   if (unlikely(skb->csum_not_inet))
       return skb_checksum_help_not_inet(...);

   The rest of skb_checksum_help should remained unchanged.

According to documentation [2], validate_xmit_skb() is a good place where
the if() statement above can be done, to preserve the possibility of having
the CRC32c computation offloaded by the NIC hardware:

if (unlikely(skb->csum_not_inet && !(features & NETIF_F_SCTP_CRC))
               return skb_checksum_help_not_inet(...);

On Thu, 2017-02-02 at 16:55 +0000, David Laight wrote:

quoted

I'd put the onus on any such interface to perform the checksum (and
set CHECKSUM_COMPLETE (or is it UNNECESSARY?) before passing the
message onto an interface that doesn't advertise CRC32 support.

You certainly don't want to have to go through all the ethernet drivers!

Ideally, a driver not able to offload checksum computation should call
skb_checksum_help() or skb_sctp_csum_help() to resolve CHECKSUM_PARTIAL
and turn it to CHECKSUM_NONE.
But this wouldn't solve all possible setups: there can be scenarios
where the NIC is configured with NETIF_F_SCTP_CRC set and NETIF_F_CSUM_HW
cleared (it's evil, but possible). In this situation, non-GSO SCTP packets
having CHECKSUM_PARTIAL will be systematically corrupted when they are
processed by validate_xmit_skb().

On Thu, 2017-02-02 at 10:08 -0800, Tom Herbert wrote:

quoted

- Add a description of the new bit and how skb_checksum_help can work
to the comments for CHECKSUM_PARTIAL in skbuff.h

Done.

quoted

- Add FCOE to the list of protocol that can set CHECKSUM_UNNECESSARY
for a CRC/csum

Done.

quoted

- Add a note to CHECKSUM_COMPLETE section that it can only refer to an
Internet checksum

Done.

/* references + notes */

[1] ... this recalls to latest comment from David Laight:
On Thu, 2017-02-02 at 16:55 +0000, David Laight wrote:

quoted

I have to admit to not knowing exactly what the CHECKSUM_xxx flags
actually mean. I have a good idea about what the intention is though.

According to domumentation, CHECKSUM_COMPLETE and CHECKSUM_UNNECESSARY are
not used for SCTP (nor in the TX path at all); nevertheless, IPVS snat/dnat
actually set CHECKSUM_UNNECESSARY on SCTP packets after the checksum is
updated (see 97203abe6bc4 "net: ipvs: sctp: do not recalc...).

CHECKSUM_PARTIAL is the preferred mechanism on the transmit path this
defers defers the checksum computation as long as possible.
Unfortunately, if SCTP is encapsulated in UDP we will probably need to
run the SCTP CRC on the host which will be done with your changes to
skb_checksum_help.

I'm not sure if setting CHECKSUM_UNNECESSARY fits my case, because this would
implicitly skip RX validation when using devices like veth or loopback.

CHECKSUM_UNNECESSARY can be used in the transmit path (really the
forwarding path), however this I think this must imply that the
checksum in the packet must be correct. Please see my post about
drivers that are mistakingly using CHECKSUM_UNNECESSARY with LRO since
the checksum in the packet sent into the stack is not correct.

Tom

[2] Documentation/networking/checksum_offloads.txt

regards,
--
davide

On Mon, 2017-01-23 at 12:59 -0800, Tom Herbert wrote:

quoted

quoted

quoted

It might make sense to create some CRC helper functions, but last time
I checked there are so few users of CRC in skbufs I'm not even sure
that would make sense.

hello Tom and David,

after some (thinking + testing) time, I'm going to re-post this RFC as v2 with
some feedbacks. Thank you in advance for looking at it!

On Thu, 2017-02-02 at 10:08 -0800, Tom Herbert wrote:

On Thu, 2017-02-02 at 16:07 +0100, Davide Caratti wrote:

quoted

This is exactly the cause of issues I see with SCTP. These packets can be
wrongly checksummed using skb_checksum_help, or simply not checksummed at
all; and in both cases, the packet goes out from the NIC with wrong L4
checksum.

Okay, makes sense. Please consider doing the following:

- Add a bit to skbuf called something like "csum_not_inet". When
ip_summed == CHECKSUM_PARTIAL and this bit is set that means we are
dealing with something other than an Internet checksum.

Ok, done. Another solution would be to extend possible values of
skb->ip_summed, and define a new value suitable for identifying
not-yet-checksummed SCTP packets (something like CRC32C_PARTIAL). Since
skb->ip_summed is 2-bit wide, the overall effect on skb metadata is the
same as adding skb->csum_not_inet [1].

- At the top of skb_checksum_help (or maybe before the point where the
inet specific checksum start begins do something like:

   if (unlikely(skb->csum_not_inet))
       return skb_checksum_help_not_inet(...);

   The rest of skb_checksum_help should remained unchanged.

According to documentation [2], validate_xmit_skb() is a good place where
the if() statement above can be done, to preserve the possibility of having
the CRC32c computation offloaded by the NIC hardware:

if (unlikely(skb->csum_not_inet && !(features & NETIF_F_SCTP_CRC))
	       return skb_checksum_help_not_inet(...);

On Thu, 2017-02-02 at 16:55 +0000, David Laight wrote:

I'd put the onus on any such interface to perform the checksum (and
set CHECKSUM_COMPLETE (or is it UNNECESSARY?) before passing the 
message onto an interface that doesn't advertise CRC32 support.

You certainly don't want to have to go through all the ethernet drivers!

Ideally, a driver not able to offload checksum computation should call
skb_checksum_help() or skb_sctp_csum_help() to resolve CHECKSUM_PARTIAL
and turn it to CHECKSUM_NONE.
But this wouldn't solve all possible setups: there can be scenarios
where the NIC is configured with NETIF_F_SCTP_CRC set and NETIF_F_CSUM_HW
cleared (it's evil, but possible). In this situation, non-GSO SCTP packets
having CHECKSUM_PARTIAL will be systematically corrupted when they are
processed by validate_xmit_skb().

On Thu, 2017-02-02 at 10:08 -0800, Tom Herbert wrote:

- Add a description of the new bit and how skb_checksum_help can work
to the comments for CHECKSUM_PARTIAL in skbuff.h

Done.

- Add FCOE to the list of protocol that can set CHECKSUM_UNNECESSARY
for a CRC/csum

Done.

- Add a note to CHECKSUM_COMPLETE section that it can only refer to an
Internet checksum

Done.

/* references + notes */

[1] ... this recalls to latest comment from David Laight:
On Thu, 2017-02-02 at 16:55 +0000, David Laight wrote:

I have to admit to not knowing exactly what the CHECKSUM_xxx flags
actually mean. I have a good idea about what the intention is though.

According to domumentation, CHECKSUM_COMPLETE and CHECKSUM_UNNECESSARY are
not used for SCTP (nor in the TX path at all); nevertheless, IPVS snat/dnat
actually set CHECKSUM_UNNECESSARY on SCTP packets after the checksum is
updated (see 97203abe6bc4 "net: ipvs: sctp: do not recalc...).

I'm not sure if setting CHECKSUM_UNNECESSARY fits my case, because this would
implicitly skip RX validation when using devices like veth or loopback.

[2] Documentation/networking/checksum_offloads.txt

regards,

On Mon, 2017-02-27 at 07:11 -0800, Tom Herbert wrote:

CHECKSUM_PARTIAL is the preferred mechanism on the transmit path this
defers defers the checksum computation as long as possible.
Unfortunately, if SCTP is encapsulated in UDP we will probably need to
run the SCTP CRC on the host which will be done with your changes to
skb_checksum_help.

right. Tunnel devices have NETIF_F_SCTP_CRC bit cleared and
NETIF_F_HW_CSUM bit set: so, in this case csum_not_inet can help
recovering non-GSO SCTP packets having ip_summed equal to
CHECKSUM_PARTIAL.

quoted

I'm not sure if setting CHECKSUM_UNNECESSARY fits my case, because this would
implicitly skip RX validation when using devices like veth or loopback.

CHECKSUM_UNNECESSARY can be used in the transmit path (really the
forwarding path), however this I think this must imply that the
checksum in the packet must be correct. Please see my post about
drivers that are mistakingly using CHECKSUM_UNNECESSARY with LRO since
the checksum in the packet sent into the stack is not correct.

Ok, now I'm more convinced to use CHECKSUM_NONE :-)

thank you for the attention!
regards

skb_sctp_csum_help is like skb_checksum_help, but it is designed for
checksumming SCTP packets using crc32c (see RFC3309), provided that
sctp.ko has been loaded before. In case sctp.ko is not loaded, invoking
skb_sctp_csum_help on a skb results in the following printout:

sk_buff: attempt to compute crc32c without sctp.ko

Signed-off-by: Davide Caratti <redacted>
---
 include/linux/netdevice.h |  1 +
 include/linux/skbuff.h    |  3 ++-
 net/core/dev.c            | 40 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index f40f0ab..8c34735 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h

@@ -3918,6 +3918,7 @@ void netdev_rss_key_fill(void *buffer, size_t len);
 
 int dev_get_nest_level(struct net_device *dev);
 int skb_checksum_help(struct sk_buff *skb);
+int skb_sctp_csum_help(struct sk_buff *skb);
 struct sk_buff *__skb_gso_segment(struct sk_buff *skb,
 				  netdev_features_t features, bool tx_path);
 struct sk_buff *skb_mac_gso_segment(struct sk_buff *skb,

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index cab9a32..0671131 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -192,7 +192,8 @@
  *     accordingly. Note the there is no indication in the skbuff that the
  *     CHECKSUM_PARTIAL refers to an SCTP checksum, a driver that supports
  *     both IP checksum offload and SCTP CRC offload must verify which offload
- *     is configured for a packet presumably by inspecting packet headers.
+ *     is configured for a packet presumably by inspecting packet headers; in
+ *     case, skb_sctp_csum_help is provided to compute CRC on SCTP packets.
  *
  *   NETIF_F_FCOE_CRC - This feature indicates that a device is capable of
  *     offloading the FCOE CRC in a packet. To perform this offload the stack

diff --git a/net/core/dev.c b/net/core/dev.c
index 05d19c6..b9fb843 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -140,6 +140,7 @@
 #include <linux/hrtimer.h>
 #include <linux/netfilter_ingress.h>
 #include <linux/crash_dump.h>
+#include <linux/sctp.h>
 
 #include "net-sysfs.h"
 

@@ -2578,6 +2579,45 @@ int skb_checksum_help(struct sk_buff *skb)
 }
 EXPORT_SYMBOL(skb_checksum_help);
 
+int skb_sctp_csum_help(struct sk_buff *skb)
+{
+	__le32 crc32c_csum;
+	int ret = 0, offset;
+
+	if (skb->ip_summed != CHECKSUM_PARTIAL)
+		goto out;
+
+	if (unlikely(skb_is_gso(skb)))
+		goto out;
+
+	/* Before computing a checksum, we should make sure no frag could
+	 * be modified by an external entity : checksum could be wrong.
+	 */
+	if (unlikely(skb_has_shared_frag(skb))) {
+		ret = __skb_linearize(skb);
+		if (ret)
+			goto out;
+	}
+
+	offset = skb_checksum_start_offset(skb);
+	crc32c_csum = cpu_to_le32(~__skb_checksum(skb, offset,
+						  skb->len - offset, ~(__u32)0,
+						  sctp_csum_stub));
+	offset += offsetof(struct sctphdr, checksum);
+	BUG_ON(offset >= skb_headlen(skb));
+
+	if (skb_cloned(skb) &&
+	    !skb_clone_writable(skb, offset + sizeof(__le32))) {
+		ret = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
+		if (ret)
+			goto out;
+	}
+	*(__le32 *)(skb->data + offset) = crc32c_csum;
+	skb->ip_summed = CHECKSUM_NONE;
+out:
+	return ret;
+}
+
 __be16 skb_network_protocol(struct sk_buff *skb, int *depth)
 {
 	__be16 type = skb->protocol;

-- 
2.7.4

sctp_compute_checksum requires crc32c symbol (provided by libcrc32c), so
it can't be used in net core. Like it has been done previously with other
symbols (e.g. ipv6_dst_lookup), introduce a stub struct skb_checksum_ops
to allow computation of SCTP checksum in net core after sctp.ko (and thus
libcrc32c) has been loaded.

Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Davide Caratti <redacted>
---
 include/linux/skbuff.h |  2 ++
 net/core/skbuff.c      | 20 ++++++++++++++++++++
 net/sctp/offload.c     |  7 +++++++
 3 files changed, 29 insertions(+)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 69ccd26..cab9a32 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -3125,6 +3125,8 @@ struct skb_checksum_ops {
 	__wsum (*combine)(__wsum csum, __wsum csum2, int offset, int len);
 };
 
+extern const struct skb_checksum_ops *sctp_csum_stub __read_mostly;
+
 __wsum __skb_checksum(const struct sk_buff *skb, int offset, int len,
 		      __wsum csum, const struct skb_checksum_ops *ops);
 __wsum skb_checksum(const struct sk_buff *skb, int offset, int len,

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index f355795..64fd8fd 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -2242,6 +2242,26 @@ __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
 }
 EXPORT_SYMBOL(skb_copy_and_csum_bits);
 
+static __wsum warn_sctp_csum_update(const void *buff, int len, __wsum sum)
+{
+	net_warn_ratelimited("attempt to compute crc32c without sctp.ko\n");
+	return 0;
+}
+
+static __wsum warn_sctp_csum_combine(__wsum csum, __wsum csum2,
+				     int offset, int len)
+{
+	net_warn_ratelimited("attempt to compute crc32c without sctp.ko\n");
+	return 0;
+}
+
+const struct skb_checksum_ops *sctp_csum_stub __read_mostly =
+	&(struct skb_checksum_ops) {
+	.update  = warn_sctp_csum_update,
+	.combine = warn_sctp_csum_combine,
+};
+EXPORT_SYMBOL(sctp_csum_stub);
+
  /**
  *	skb_zerocopy_headlen - Calculate headroom needed for skb_zerocopy()
  *	@from: source buffer

diff --git a/net/sctp/offload.c b/net/sctp/offload.c
index 4f5a2b5..e9c3db0 100644
--- a/net/sctp/offload.c
+++ b/net/sctp/offload.c

@@ -98,6 +98,12 @@ static const struct net_offload sctp6_offload = {
 	},
 };
 
+static const struct skb_checksum_ops *sctp_csum_ops __read_mostly =
+	&(struct skb_checksum_ops) {
+	.update  = sctp_csum_update,
+	.combine = sctp_csum_combine,
+};
+
 int __init sctp_offload_init(void)
 {
 	int ret;

@@ -110,6 +116,7 @@ int __init sctp_offload_init(void)
 	if (ret)
 		goto ipv4;
 
+	sctp_csum_stub = sctp_csum_ops;
 	return ret;
 
 ipv4:

-- 
2.7.4

Introduce skb->csum_not_inet to identify not-yet-checksummed SCTP packets.
Use this bit in combination with netdev feature bit in validate_xmit_skb,
to discriminate whether skb needs crc32c or 2-complement Internet Checksum
(or none of the two, when the underlying device can do checksum offload).

Signed-off-by: Davide Caratti <redacted>
---
 include/linux/skbuff.h                |  1 +
 net/core/dev.c                        | 14 ++++++++++++--
 net/netfilter/ipvs/ip_vs_proto_sctp.c |  1 +
 net/netfilter/nf_nat_proto_sctp.c     |  1 +
 net/sched/act_csum.c                  |  1 +
 net/sctp/output.c                     |  1 +
 6 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 0671131..236b7d9 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -759,6 +759,7 @@ struct sk_buff {
 	__u8			tc_redirected:1;
 	__u8			tc_from_ingress:1;
 #endif
+	__u8			csum_not_inet:1;
 
 #ifdef CONFIG_NET_SCHED
 	__u16			tc_index;	/* traffic control index */

diff --git a/net/core/dev.c b/net/core/dev.c
index b9fb843..fae3217 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -2614,6 +2614,7 @@ int skb_sctp_csum_help(struct sk_buff *skb)
 	}
 	*(__le32 *)(skb->data + offset) = crc32c_csum;
 	skb->ip_summed = CHECKSUM_NONE;
+	skb->csum_not_inet = 0;
 out:
 	return ret;
 }

@@ -2960,6 +2961,16 @@ static struct sk_buff *validate_xmit_vlan(struct sk_buff *skb,
 	return skb;
 }
 
+static int skb_csum_hwoffload_help(struct sk_buff *skb,
+				   netdev_features_t features)
+{
+	if (unlikely(skb->csum_not_inet))
+		return !(features & NETIF_F_SCTP_CRC) ?
+				skb_sctp_csum_help(skb) : 0;
+
+	return !(features & NETIF_F_CSUM_MASK) ? skb_checksum_help(skb) : 0;
+}
+
 static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device *dev)
 {
 	netdev_features_t features;

@@ -2995,8 +3006,7 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device
 			else
 				skb_set_transport_header(skb,
 							 skb_checksum_start_offset(skb));
-			if (!(features & NETIF_F_CSUM_MASK) &&
-			    skb_checksum_help(skb))
+			if (skb_csum_hwoffload_help(skb, features))
 				goto out_kfree_skb;
 		}
 	}

diff --git a/net/netfilter/ipvs/ip_vs_proto_sctp.c b/net/netfilter/ipvs/ip_vs_proto_sctp.c
index d952d67..4972a60 100644
--- a/net/netfilter/ipvs/ip_vs_proto_sctp.c
+++ b/net/netfilter/ipvs/ip_vs_proto_sctp.c

@@ -81,6 +81,7 @@ static void sctp_nat_csum(struct sk_buff *skb, sctp_sctphdr_t *sctph,
 			  unsigned int sctphoff)
 {
 	sctph->checksum = sctp_compute_cksum(skb, sctphoff);
+	skb->csum_not_inet = 0;
 	skb->ip_summed = CHECKSUM_UNNECESSARY;
 }
 

diff --git a/net/netfilter/nf_nat_proto_sctp.c b/net/netfilter/nf_nat_proto_sctp.c
index 31d3586..9459b88 100644
--- a/net/netfilter/nf_nat_proto_sctp.c
+++ b/net/netfilter/nf_nat_proto_sctp.c

@@ -49,6 +49,7 @@ sctp_manip_pkt(struct sk_buff *skb,
 
 	if (skb->ip_summed != CHECKSUM_PARTIAL) {
 		hdr->checksum = sctp_compute_cksum(skb, hdroff);
+		skb->csum_not_inet = 0;
 		skb->ip_summed = CHECKSUM_NONE;
 	}
 

diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index e978ccd4..85cb150 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c

@@ -337,6 +337,7 @@ static int tcf_csum_sctp(struct sk_buff *skb, unsigned int ihl,
 
 	sctph->checksum = sctp_compute_cksum(skb,
 					     skb_network_offset(skb) + ihl);
+	skb->csum_not_inet = 0;
 	skb->ip_summed = CHECKSUM_NONE;
 
 	return 1;

diff --git a/net/sctp/output.c b/net/sctp/output.c
index 814eac0..0dc227b 100644
--- a/net/sctp/output.c
+++ b/net/sctp/output.c

@@ -528,6 +528,7 @@ static int sctp_packet_pack(struct sctp_packet *packet,
 	} else {
 chksum:
 		head->ip_summed = CHECKSUM_PARTIAL;
+		head->csum_not_inet = 1;
 		head->csum_start = skb_transport_header(head) - head->head;
 		head->csum_offset = offsetof(struct sctphdr, checksum);
 	}

-- 
2.7.4

Add description of skb_sctp_csum_help in networking/checksum-offload.txt,
and document its usage in combination with skb->csum_not_inet. While at
it, remove reference to skb_csum_off_chk* functions, since they have been
removed from Linux source tree since commit cf53b1da73bd ("Revert "net:
Add driver helper functions to determine checksum""), and add missing
explaination of CHECKSUM_UNNECESSARY for FCOE protocol.

Signed-off-by: Davide Caratti <redacted>
---
 Documentation/networking/checksum-offloads.txt |  7 ++++---
 include/linux/skbuff.h                         | 25 ++++++++++++-------------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/Documentation/networking/checksum-offloads.txt b/Documentation/networking/checksum-offloads.txt
index 56e3686..81534e9 100644
--- a/Documentation/networking/checksum-offloads.txt
+++ b/Documentation/networking/checksum-offloads.txt

@@ -49,8 +49,8 @@ A driver declares its offload capabilities in netdev->hw_features; see
  and csum_offset given in the SKB; if it tries to deduce these itself in
  hardware (as some NICs do) the driver should check that the values in the
  SKB match those which the hardware will deduce, and if not, fall back to
- checksumming in software instead (with skb_checksum_help or one of the
- skb_csum_off_chk* functions as mentioned in include/linux/skbuff.h).  This
+ checksumming in software instead (with skb_checksum_help or
+ skb_sctp_csum_help functions as mentioned in include/linux/skbuff.h). This
  is a pain, but that's what you get when hardware tries to be clever.
 
 The stack should, for the most part, assume that checksum offload is

@@ -60,7 +60,8 @@ The stack should, for the most part, assume that checksum offload is
  may include other offloads besides TX Checksum Offload) and, if they are
  not supported or enabled on the device (determined by netdev->features),
  performs the corresponding offload in software.  In the case of TX
- Checksum Offload, that means calling skb_checksum_help(skb).
+ Checksum Offload, that means calling skb_sctp_csum_help(skb) for SCTP
+ packets, and skb_checksum_help(skb) for other packets.
 
 
 LCO: Local Checksum Offload

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 236b7d9..12d3625 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -108,6 +108,7 @@
  *       may perform further validation in this case.
  *     GRE: only if the checksum is present in the header.
  *     SCTP: indicates the CRC in SCTP header has been validated.
+ *     FCOE: indicates the CRC in FC frame has been validated.
  *
  *   skb->csum_level indicates the number of consecutive checksums found in
  *   the packet minus one that have been verified as CHECKSUM_UNNECESSARY.

@@ -161,14 +162,13 @@
  *
  *   NETIF_F_IP_CSUM and NETIF_F_IPV6_CSUM are being deprecated in favor of
  *   NETIF_F_HW_CSUM. New devices should use NETIF_F_HW_CSUM to indicate
- *   checksum offload capability. If a	device has limited checksum capabilities
- *   (for instance can only perform NETIF_F_IP_CSUM or NETIF_F_IPV6_CSUM as
- *   described above) a helper function can be called to resolve
- *   CHECKSUM_PARTIAL. The helper functions are skb_csum_off_chk*. The helper
- *   function takes a spec argument that describes the protocol layer that is
- *   supported for checksum offload and can be called for each packet. If a
- *   packet does not match the specification for offload, skb_checksum_help
- *   is called to resolve the checksum.
+ *   checksum offload capability. If a device has limited checksum capabilities
+ *   (for instance it can't perform NETIF_F_IP_CSUM or NETIF_F_IPV6_CSUM as
+ *   described above) a helper function (namely skb_csum_hwoffload_help) can
+ *   be called to resolve CHECKSUM_PARTIAL. This function uses netdev_features_t
+ *   to have the Internet Checksum computed by HW, in case any feature belonging
+ *   to NETIF_F_CSUM_MASK is set, or by software using skb_checksum_help().
+ *   See also Section D.
  *
  * CHECKSUM_NONE:
  *

@@ -189,11 +189,10 @@
  *   NETIF_F_SCTP_CRC - This feature indicates that a device is capable of
  *     offloading the SCTP CRC in a packet. To perform this offload the stack
  *     will set ip_summed to CHECKSUM_PARTIAL and set csum_start and csum_offset
- *     accordingly. Note the there is no indication in the skbuff that the
- *     CHECKSUM_PARTIAL refers to an SCTP checksum, a driver that supports
- *     both IP checksum offload and SCTP CRC offload must verify which offload
- *     is configured for a packet presumably by inspecting packet headers; in
- *     case, skb_sctp_csum_help is provided to compute CRC on SCTP packets.
+ *     accordingly. skb->csum_not_inet is an indication in the skbuff that the
+ *     CHECKSUM_PARTIAL refers to an SCTP checksum: a driver can use it to
+ *     decide whether skb_checksum_help() or skb_sctp_csum_help() have to be
+ *     called on a sk_buff having ip_summed set to CHECKSUM_PARTIAL.
  *
  *   NETIF_F_FCOE_CRC - This feature indicates that a device is capable of
  *     offloading the FCOE CRC in a packet. To perform this offload the stack

-- 
2.7.4

On Tue, Feb 28, 2017 at 2:32 AM, Davide Caratti [off-list ref] wrote:

Unfortunately this potentially pushes the skbuf flags over 32 bits if
I count correctly. I suggest that you rename csum_bad to
csum_not_inet. Looks like csum_bad is only set by a grand total of one
driver and I don't believe that is enough to justify its existence.
It's probably a good time to remove it.

Return value looks complex. Maybe we should just change
skb_csum_*_help to return bool, true of checksum was handled false if
not.

On Tue, Feb 28, 2017 at 2:32 AM, Davide Caratti [off-list ref] wrote:

sctp_compute_checksum requires crc32c symbol (provided by libcrc32c), so
it can't be used in net core. Like it has been done previously with other
symbols (e.g. ipv6_dst_lookup), introduce a stub struct skb_checksum_ops
to allow computation of SCTP checksum in net core after sctp.ko (and thus
libcrc32c) has been loaded.

At a minimum the name really needs to change.  SCTP does not do
checksums.  It does a CRC, and a CRC is a very different thing.  The
fact that somebody decided that offloading a CRC could use the same
framework is very unfortunate, and your patch descriptions in this
whole set are calling out a CRC as checksums which it is not.

I don't want to see anything "checksum" or "csum" related in the
naming when it comes to dealing with SCTP unless we absolutely have to
have it.  So any function names or structures with sctp in the name
should call out "crc32" or "crc", please don't use checksum.

On Tue, Feb 28, 2017 at 2:46 PM, Alexander Duyck
[off-list ref] wrote:

On Tue, Feb 28, 2017 at 2:32 AM, Davide Caratti [off-list ref] wrote:

quoted

sctp_compute_checksum requires crc32c symbol (provided by libcrc32c), so
it can't be used in net core. Like it has been done previously with other
symbols (e.g. ipv6_dst_lookup), introduce a stub struct skb_checksum_ops
to allow computation of SCTP checksum in net core after sctp.ko (and thus
libcrc32c) has been loaded.

At a minimum the name really needs to change.  SCTP does not do
checksums.  It does a CRC, and a CRC is a very different thing.  The
fact that somebody decided that offloading a CRC could use the same
framework is very unfortunate, and your patch descriptions in this
whole set are calling out a CRC as checksums which it is not.

I don't want to see anything "checksum" or "csum" related in the
naming when it comes to dealing with SCTP unless we absolutely have to
have it.  So any function names or structures with sctp in the name
should call out "crc32" or "crc", please don't use checksum.

Alexander,

I agree that internal functions to sctp should not refer to checksum,
but I think we need to take care to be consistent with any external
API (even if somebody made a mistake defining it this way :-) ). As
you know the checksum interface must be very precisely defined, there
is no leeway for ambiguity. Many places in the stack use csum and
CHECKSUM_* to refer to the API not the actual algorithm, others don't
(e.g. CHECKSUM_UNNECESSARY can apply to SCTP checksum,
CHECKSUM_COMPLETE must be an Internet checksum).

For instance, in that light skb_sctp_csum_help is appropriately named
I think because this is being called from skb_csum_help and refers to
the interface to resolve a checksum.

Tom

quoted

Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Davide Caratti <redacted>
---
 include/linux/skbuff.h |  2 ++
 net/core/skbuff.c      | 20 ++++++++++++++++++++
 net/sctp/offload.c     |  7 +++++++
 3 files changed, 29 insertions(+)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 69ccd26..cab9a32 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -3125,6 +3125,8 @@ struct skb_checksum_ops {
        __wsum (*combine)(__wsum csum, __wsum csum2, int offset, int len);
 };

+extern const struct skb_checksum_ops *sctp_csum_stub __read_mostly;
+
 __wsum __skb_checksum(const struct sk_buff *skb, int offset, int len,
                      __wsum csum, const struct skb_checksum_ops *ops);
 __wsum skb_checksum(const struct sk_buff *skb, int offset, int len,

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index f355795..64fd8fd 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -2242,6 +2242,26 @@ __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
 }
 EXPORT_SYMBOL(skb_copy_and_csum_bits);

+static __wsum warn_sctp_csum_update(const void *buff, int len, __wsum sum)
+{
+       net_warn_ratelimited("attempt to compute crc32c without sctp.ko\n");
+       return 0;
+}
+
+static __wsum warn_sctp_csum_combine(__wsum csum, __wsum csum2,
+                                    int offset, int len)
+{
+       net_warn_ratelimited("attempt to compute crc32c without sctp.ko\n");
+       return 0;
+}
+
+const struct skb_checksum_ops *sctp_csum_stub __read_mostly =
+       &(struct skb_checksum_ops) {
+       .update  = warn_sctp_csum_update,
+       .combine = warn_sctp_csum_combine,
+};
+EXPORT_SYMBOL(sctp_csum_stub);
+
  /**
  *     skb_zerocopy_headlen - Calculate headroom needed for skb_zerocopy()
  *     @from: source buffer

diff --git a/net/sctp/offload.c b/net/sctp/offload.c
index 4f5a2b5..e9c3db0 100644
--- a/net/sctp/offload.c
+++ b/net/sctp/offload.c

@@ -98,6 +98,12 @@ static const struct net_offload sctp6_offload = {
        },
 };

+static const struct skb_checksum_ops *sctp_csum_ops __read_mostly =
+       &(struct skb_checksum_ops) {
+       .update  = sctp_csum_update,
+       .combine = sctp_csum_combine,
+};
+
 int __init sctp_offload_init(void)
 {
        int ret;

@@ -110,6 +116,7 @@ int __init sctp_offload_init(void)
        if (ret)
                goto ipv4;

+       sctp_csum_stub = sctp_csum_ops;
        return ret;

 ipv4:
--

2.7.4