[PATCH net-next v2 0/7] drivers: Fix drivers doing TX csum offload with EH

[PATCH net-next v2 0/7] drivers: Fix drivers doing TX csum offload with EH

[Tom Herbert]

Several NICs would seem to support protocol specific TX checksum offload
and allow for cases where an IPv6 packet contains extension headers.
When deciding whether to offload a packet, ipv6_skip_exthdr is called
to skip extension headers. The problem is that if a packet contains an
IPv6 Routing Header then protocol specific checksum offload can't work,
the destination IP address in the IPv6 header is not the same one that
is used in the pseudo header for TCP or UDP. The correct address is
derived from the last segment in the routing list (which itself might
be obfuscated so that a device could even read it).

This patch set adds a new function ipv6_skip_exthdr_no_rthdr to be
called in lieu of ipv6_skip_exthdr. If a routing header is present in
a packet then ipv6_skip_exthdr_no_rthdr returns a value less than
zero, this is an indication to the driver that TX checksum offload
is not viable and it should call skb_checksum_help instead of
offloading the checksum.

The i40e, iavf, ice, idpf, hinic, and fm10k are updated accordingly
to call ipv6_skip_exthdr_no_rthdr.

Testing: The code compiles, but is otherwise untested due to lack of
NIC hardware. It would be appreciated if someone with access to the
hardware could test.

v2: Fixed uninitialized variable in exthdrs_core.c

Tom Herbert (7):
  ipv6: Add ipv6_skip_exthdr_no_rthdr
  i40e: Don't do TX csum offload with routing header present
  iavf: Don't do TX csum offload with routing header present
  ice: Don't do TX csum offload with routing header present
  idpf: Don't do TX csum offload with routing header present
  hinic: Don't do TX csum offload with routing header present
  fm10k: Don't do TX csum offload with routing header present

 drivers/net/ethernet/huawei/hinic/hinic_tx.c  | 23 +++++++++++----
 drivers/net/ethernet/intel/fm10k/fm10k_main.c |  9 ++++--
 drivers/net/ethernet/intel/i40e/i40e_txrx.c   | 22 ++++++---------
 drivers/net/ethernet/intel/iavf/iavf_txrx.c   | 20 ++++++-------
 drivers/net/ethernet/intel/ice/ice_txrx.c     | 22 ++++++---------
 .../ethernet/intel/idpf/idpf_singleq_txrx.c   | 28 +++++++++----------
 include/net/ipv6.h                            | 17 +++++++++--
 net/ipv6/exthdrs_core.c                       | 25 ++++++++++++-----
 8 files changed, 98 insertions(+), 68 deletions(-)

-- 
2.34.1

[PATCH net-next v2 1/7] ipv6: Add ipv6_skip_exthdr_no_rthdr

[Tom Herbert]

ipv6_skip_exthdr_no_rthdr will be called by drivers that support
protocol specific transmit checksum offload with extension headers.
Protocol specific checksum offload doesn't work with routing headers
since the destination address in the IPv6 header is not the one used
in the pseduo checksum for TCP or UDP. This is not a problem with
protocol agnostic checksum offload.

If a routing header is present then ipv6_skip_exthdr_no_rthdr returns
a value less than zero, this is an indication that the driver should
call skb_checksum_help instead of offloading the checksum which  would
be doomed to cause a packet drop at the receiver due to a bad checksum.

Signed-off-by: Tom Herbert <tom@herbertland.com>
---
 include/net/ipv6.h      | 17 +++++++++++++++--
 net/ipv6/exthdrs_core.c | 25 ++++++++++++++++++-------
 2 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 88a8e554f7a1..6581fabd1e1e 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -1157,8 +1157,21 @@ void ipv6_push_nfrag_opts(struct sk_buff *skb, struct ipv6_txoptions *opt,
 void ipv6_push_frag_opts(struct sk_buff *skb, struct ipv6_txoptions *opt,
 			 u8 *proto);
 
-int ipv6_skip_exthdr(const struct sk_buff *, int start, u8 *nexthdrp,
-		     __be16 *frag_offp);
+int __ipv6_skip_exthdr(const struct sk_buff *skb, int start, u8 *nexthdrp,
+		       __be16 *frag_offp, bool no_rthdr);
+
+static inline int ipv6_skip_exthdr(const struct sk_buff *skb, int start,
+				   u8 *nexthdrp, __be16 *frag_offp)
+{
+	return __ipv6_skip_exthdr(skb, start, nexthdrp, frag_offp, false);
+}
+
+static inline int ipv6_skip_exthdr_no_rthdr(const struct sk_buff *skb,
+					    int start, u8 *nexthdrp,
+					    __be16 *frag_offp)
+{
+	return __ipv6_skip_exthdr(skb, start, nexthdrp, frag_offp, true);
+}
 
 bool ipv6_ext_hdr(u8 nexthdr);
 
diff --git a/net/ipv6/exthdrs_core.c b/net/ipv6/exthdrs_core.c
index 49e31e4ae7b7..25501777ae05 100644
--- a/net/ipv6/exthdrs_core.c
+++ b/net/ipv6/exthdrs_core.c
@@ -69,8 +69,8 @@ EXPORT_SYMBOL(ipv6_ext_hdr);
  * --ANK (980726)
  */
 
-int ipv6_skip_exthdr(const struct sk_buff *skb, int start, u8 *nexthdrp,
-		     __be16 *frag_offp)
+int __ipv6_skip_exthdr(const struct sk_buff *skb, int start, u8 *nexthdrp,
+		       __be16 *frag_offp, bool no_rthdr)
 {
 	u8 nexthdr = *nexthdrp;
 
@@ -85,7 +85,8 @@ int ipv6_skip_exthdr(const struct sk_buff *skb, int start, u8 *nexthdrp,
 		hp = skb_header_pointer(skb, start, sizeof(_hdr), &_hdr);
 		if (!hp)
 			return -1;
-		if (nexthdr == NEXTHDR_FRAGMENT) {
+		switch (nexthdr) {
+		case NEXTHDR_FRAGMENT: {
 			__be16 _frag_off, *fp;
 			fp = skb_header_pointer(skb,
 						start+offsetof(struct frag_hdr,
@@ -97,21 +98,31 @@ int ipv6_skip_exthdr(const struct sk_buff *skb, int start, u8 *nexthdrp,
 
 			*frag_offp = *fp;
 			if (ntohs(*frag_offp) & ~0x7)
-				break;
+				goto out;
 			hdrlen = 8;
-		} else if (nexthdr == NEXTHDR_AUTH)
+			break;
+		}
+		case NEXTHDR_AUTH:
 			hdrlen = ipv6_authlen(hp);
-		else
+			break;
+		case NEXTHDR_ROUTING:
+			if (no_rthdr)
+				return -1;
+			fallthrough;
+		default:
 			hdrlen = ipv6_optlen(hp);
+			break;
+		}
 
 		nexthdr = hp->nexthdr;
 		start += hdrlen;
 	}
 
+out:
 	*nexthdrp = nexthdr;
 	return start;
 }
-EXPORT_SYMBOL(ipv6_skip_exthdr);
+EXPORT_SYMBOL(__ipv6_skip_exthdr);
 
 int ipv6_find_tlv(const struct sk_buff *skb, int offset, int type)
 {
-- 
2.34.1

[PATCH net-next v2 2/7] i40e: Don't do TX csum offload with routing header present

[Tom Herbert]

When determining if the L4 checksum in an IPv6 packet can be offloaded
on transmit, call ipv6_skip_exthdr_no_rthdr to check for the presence
of a routing header. If a routing header is present, that is the
function return less than zero, then don't offload checksum and call
skb_checksum_help instead.

Signed-off-by: Tom Herbert <tom@herbertland.com>
---
 drivers/net/ethernet/intel/i40e/i40e_txrx.c | 22 +++++++++------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index c006f716a3bd..b89761e3be7f 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -3296,16 +3296,13 @@ static int i40e_tx_enable_csum(struct sk_buff *skb, u32 *tx_flags,
 
 			l4_proto = ip.v4->protocol;
 		} else if (*tx_flags & I40E_TX_FLAGS_IPV6) {
-			int ret;
-
 			tunnel |= I40E_TX_CTX_EXT_IP_IPV6;
 
 			exthdr = ip.hdr + sizeof(*ip.v6);
 			l4_proto = ip.v6->nexthdr;
-			ret = ipv6_skip_exthdr(skb, exthdr - skb->data,
-					       &l4_proto, &frag_off);
-			if (ret < 0)
-				return -1;
+			if (ipv6_skip_exthdr_no_rthdr(skb, exthdr - skb->data,
+						      &l4_proto, &frag_off) < 0)
+				goto no_csum_offload;
 		}
 
 		/* define outer transport */
@@ -3324,6 +3321,7 @@ static int i40e_tx_enable_csum(struct sk_buff *skb, u32 *tx_flags,
 			l4.hdr = skb_inner_network_header(skb);
 			break;
 		default:
+no_csum_offload:
 			if (*tx_flags & I40E_TX_FLAGS_TSO)
 				return -1;
 
@@ -3377,9 +3375,10 @@ static int i40e_tx_enable_csum(struct sk_buff *skb, u32 *tx_flags,
 
 		exthdr = ip.hdr + sizeof(*ip.v6);
 		l4_proto = ip.v6->nexthdr;
-		if (l4.hdr != exthdr)
-			ipv6_skip_exthdr(skb, exthdr - skb->data,
-					 &l4_proto, &frag_off);
+		if (l4.hdr != exthdr &&
+		    ipv6_skip_exthdr_no_rthdr(skb, exthdr - skb->data,
+					      &l4_proto, &frag_off) < 0)
+			goto no_csum_offload;
 	}
 
 	/* compute inner L3 header size */
@@ -3405,10 +3404,7 @@ static int i40e_tx_enable_csum(struct sk_buff *skb, u32 *tx_flags,
 			  I40E_TX_DESC_LENGTH_L4_FC_LEN_SHIFT;
 		break;
 	default:
-		if (*tx_flags & I40E_TX_FLAGS_TSO)
-			return -1;
-		skb_checksum_help(skb);
-		return 0;
+		goto no_csum_offload;
 	}
 
 	*td_cmd |= cmd;
-- 
2.34.1

[PATCH net-next v2 3/7] iavf: Don't do TX csum offload with routing header present

[Tom Herbert]

When determining if the L4 checksum in an IPv6 packet can be offloaded
on transmit, call ipv6_skip_exthdr_no_rthdr to check for the presence
of a routing header. If a routing header is present, that is the
function return less than zero, then don't offload checksum and call
skb_checksum_help instead.

Signed-off-by: Tom Herbert <tom@herbertland.com>
---
 drivers/net/ethernet/intel/iavf/iavf_txrx.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_txrx.c b/drivers/net/ethernet/intel/iavf/iavf_txrx.c
index 26b424fd6718..44cad541bed4 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_txrx.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_txrx.c
@@ -1651,9 +1651,10 @@ static int iavf_tx_enable_csum(struct sk_buff *skb, u32 *tx_flags,
 
 			exthdr = ip.hdr + sizeof(*ip.v6);
 			l4_proto = ip.v6->nexthdr;
-			if (l4.hdr != exthdr)
-				ipv6_skip_exthdr(skb, exthdr - skb->data,
-						 &l4_proto, &frag_off);
+			if (l4.hdr != exthdr &&
+			    ipv6_skip_exthdr_no_rthdr(skb, exthdr - skb->data,
+						      &l4_proto, &frag_off) < 0)
+				goto no_csum_offload;
 		}
 
 		/* define outer transport */
@@ -1672,6 +1673,7 @@ static int iavf_tx_enable_csum(struct sk_buff *skb, u32 *tx_flags,
 			l4.hdr = skb_inner_network_header(skb);
 			break;
 		default:
+no_csum_offload:
 			if (*tx_flags & IAVF_TX_FLAGS_TSO)
 				return -1;
 
@@ -1725,9 +1727,10 @@ static int iavf_tx_enable_csum(struct sk_buff *skb, u32 *tx_flags,
 
 		exthdr = ip.hdr + sizeof(*ip.v6);
 		l4_proto = ip.v6->nexthdr;
-		if (l4.hdr != exthdr)
-			ipv6_skip_exthdr(skb, exthdr - skb->data,
-					 &l4_proto, &frag_off);
+		if (l4.hdr != exthdr &&
+		    ipv6_skip_exthdr_no_rthdr(skb, exthdr - skb->data,
+					      &l4_proto, &frag_off) < 0)
+			goto no_csum_offload;
 	}
 
 	/* compute inner L3 header size */
@@ -1753,10 +1756,7 @@ static int iavf_tx_enable_csum(struct sk_buff *skb, u32 *tx_flags,
 			  IAVF_TX_DESC_LENGTH_L4_FC_LEN_SHIFT;
 		break;
 	default:
-		if (*tx_flags & IAVF_TX_FLAGS_TSO)
-			return -1;
-		skb_checksum_help(skb);
-		return 0;
+		goto no_csum_offload;
 	}
 
 	*td_cmd |= cmd;
-- 
2.34.1

[PATCH net-next v2 4/7] ice: Don't do TX csum offload with routing header present

[Tom Herbert]

When determining if the L4 checksum in an IPv6 packet can be offloaded
on transmit, call ipv6_skip_exthdr_no_rthdr to check for the presence
of a routing header. If a routing header is present, that is the
function return less than zero, then don't offload checksum and call
skb_checksum_help instead.

Signed-off-by: Tom Herbert <tom@herbertland.com>
---
 drivers/net/ethernet/intel/ice/ice_txrx.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
index 8bb743f78fcb..fd57ac52191e 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx.c
+++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
@@ -1842,15 +1842,12 @@ int ice_tx_csum(struct ice_tx_buf *first, struct ice_tx_offload_params *off)
 				  ICE_TX_CTX_EIPT_IPV4_NO_CSUM;
 			l4_proto = ip.v4->protocol;
 		} else if (first->tx_flags & ICE_TX_FLAGS_IPV6) {
-			int ret;
-
 			tunnel |= ICE_TX_CTX_EIPT_IPV6;
 			exthdr = ip.hdr + sizeof(*ip.v6);
 			l4_proto = ip.v6->nexthdr;
-			ret = ipv6_skip_exthdr(skb, exthdr - skb->data,
-					       &l4_proto, &frag_off);
-			if (ret < 0)
-				return -1;
+			if (ipv6_skip_exthdr_no_rthdr(skb, exthdr - skb->data,
+						      &l4_proto, &frag_off) < 0)
+				goto no_csum_offload;
 		}
 
 		/* define outer transport */
@@ -1869,6 +1866,7 @@ int ice_tx_csum(struct ice_tx_buf *first, struct ice_tx_offload_params *off)
 			l4.hdr = skb_inner_network_header(skb);
 			break;
 		default:
+no_csum_offload:
 			if (first->tx_flags & ICE_TX_FLAGS_TSO)
 				return -1;
 
@@ -1928,9 +1926,10 @@ int ice_tx_csum(struct ice_tx_buf *first, struct ice_tx_offload_params *off)
 		cmd |= ICE_TX_DESC_CMD_IIPT_IPV6;
 		exthdr = ip.hdr + sizeof(*ip.v6);
 		l4_proto = ip.v6->nexthdr;
-		if (l4.hdr != exthdr)
-			ipv6_skip_exthdr(skb, exthdr - skb->data, &l4_proto,
-					 &frag_off);
+		if (l4.hdr != exthdr &&
+		    ipv6_skip_exthdr_no_rthdr(skb, exthdr - skb->data,
+					      &l4_proto, &frag_off) < 0)
+			goto no_csum_offload;
 	} else {
 		return -1;
 	}
@@ -1961,10 +1960,7 @@ int ice_tx_csum(struct ice_tx_buf *first, struct ice_tx_offload_params *off)
 		break;
 
 	default:
-		if (first->tx_flags & ICE_TX_FLAGS_TSO)
-			return -1;
-		skb_checksum_help(skb);
-		return 0;
+		goto no_csum_offload;
 	}
 
 	off->td_cmd |= cmd;
-- 
2.34.1

[PATCH net-next v2 5/7] idpf: Don't do TX csum offload with routing header present

[Tom Herbert]

When determining if the L4 checksum in an IPv6 packet can be offloaded
on transmit, call ipv6_skip_exthdr_no_rthdr to check for the presence
of a routing header. If a routing header is present, that is the
function return less than zero, then don't offload checksum and call
skb_checksum_help instead.

Signed-off-by: Tom Herbert <tom@herbertland.com>
---
 .../ethernet/intel/idpf/idpf_singleq_txrx.c   | 28 +++++++++----------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
index 27b93592c4ba..3b0bc4d7d691 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
@@ -57,10 +57,12 @@ static int idpf_tx_singleq_csum(struct sk_buff *skb,
 			tunnel |= IDPF_TX_CTX_EXT_IP_IPV6;
 
 			l4_proto = ip.v6->nexthdr;
-			if (ipv6_ext_hdr(l4_proto))
-				ipv6_skip_exthdr(skb, skb_network_offset(skb) +
-						 sizeof(*ip.v6),
-						 &l4_proto, &frag_off);
+			if (ipv6_ext_hdr(l4_proto) &&
+			    ipv6_skip_exthdr_no_rthdr(skb,
+						      skb_network_offset(skb) +
+						      sizeof(*ip.v6), &l4_proto,
+						      &frag_off) < 0)
+				goto no_csum_offload;
 		}
 
 		/* define outer transport */
@@ -76,6 +78,7 @@ static int idpf_tx_singleq_csum(struct sk_buff *skb,
 			l4.hdr = skb_inner_network_header(skb);
 			break;
 		default:
+no_csum_offload:
 			if (is_tso)
 				return -1;
 
@@ -131,10 +134,12 @@ static int idpf_tx_singleq_csum(struct sk_buff *skb,
 	} else if (off->tx_flags & IDPF_TX_FLAGS_IPV6) {
 		cmd |= IDPF_TX_DESC_CMD_IIPT_IPV6;
 		l4_proto = ip.v6->nexthdr;
-		if (ipv6_ext_hdr(l4_proto))
-			ipv6_skip_exthdr(skb, skb_network_offset(skb) +
-					 sizeof(*ip.v6), &l4_proto,
-					 &frag_off);
+		if (ipv6_ext_hdr(l4_proto) &&
+		    ipv6_skip_exthdr_no_rthdr(skb,
+					      skb_network_offset(skb) +
+					      sizeof(*ip.v6), &l4_proto,
+					      &frag_off) < 0)
+			goto no_csum_offload;
 	} else {
 		return -1;
 	}
@@ -161,12 +166,7 @@ static int idpf_tx_singleq_csum(struct sk_buff *skb,
 		l4_len = sizeof(struct sctphdr) >> 2;
 		break;
 	default:
-		if (is_tso)
-			return -1;
-
-		skb_checksum_help(skb);
-
-		return 0;
+		goto no_csum_offload;
 	}
 
 	offset |= l4_len << IDPF_TX_DESC_LEN_L4_LEN_S;
-- 
2.34.1

[PATCH net-next v2 6/7] hinic: Don't do TX csum offload with routing header present

[Tom Herbert]

When determining if the L4 checksum in an IPv6 packet can be offloaded
on transmit, call ipv6_skip_exthdr_no_rthdr to check for the presence
of a routing header. If a routing header is present, that is the
function return less than zero, then don't offload checksum and call
skb_checksum_help instead.

Signed-off-by: Tom Herbert <tom@herbertland.com>
---
 drivers/net/ethernet/huawei/hinic/hinic_tx.c | 23 +++++++++++++++-----
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/huawei/hinic/hinic_tx.c b/drivers/net/ethernet/huawei/hinic/hinic_tx.c
index 9b60966736db..ba6b7481b6fa 100644
--- a/drivers/net/ethernet/huawei/hinic/hinic_tx.c
+++ b/drivers/net/ethernet/huawei/hinic/hinic_tx.c
@@ -357,8 +357,10 @@ static int offload_csum(struct hinic_sq_task *task, u32 *queue_info,
 	u32 offset, l4_len, network_hdr_len;
 	enum hinic_l3_offload_type l3_type;
 	u32 tunnel_type = NOT_TUNNEL;
+	unsigned char *exthdr;
 	union hinic_l3 ip;
 	union hinic_l4 l4;
+	__be16 frag_off;
 	u8 l4_proto;
 
 	if (skb->ip_summed != CHECKSUM_PARTIAL)
@@ -374,17 +376,15 @@ static int offload_csum(struct hinic_sq_task *task, u32 *queue_info,
 			l3_type = IPV4_PKT_NO_CHKSUM_OFFLOAD;
 			l4_proto = ip.v4->protocol;
 		} else if (ip.v4->version == 6) {
-			unsigned char *exthdr;
-			__be16 frag_off;
-
 			l3_type = IPV6_PKT;
 			tunnel_type = TUNNEL_UDP_CSUM;
 			exthdr = ip.hdr + sizeof(*ip.v6);
 			l4_proto = ip.v6->nexthdr;
 			l4.hdr = skb_transport_header(skb);
-			if (l4.hdr != exthdr)
-				ipv6_skip_exthdr(skb, exthdr - skb->data,
-						 &l4_proto, &frag_off);
+			if (l4.hdr != exthdr &&
+			    ipv6_skip_exthdr_no_rthdr(skb, exthdr - skb->data,
+						      &l4_proto, &frag_off) < 0)
+				goto no_csum_offload;
 		} else {
 			l3_type = L3TYPE_UNKNOWN;
 			l4_proto = IPPROTO_RAW;
@@ -411,6 +411,7 @@ static int offload_csum(struct hinic_sq_task *task, u32 *queue_info,
 			network_hdr_len = skb_network_header_len(skb);
 			break;
 		default:
+no_csum_offload:
 			/* Unsupported tunnel packet, disable csum offload */
 			skb_checksum_help(skb);
 			return 0;
@@ -421,6 +422,16 @@ static int offload_csum(struct hinic_sq_task *task, u32 *queue_info,
 		ip.hdr = skb_network_header(skb);
 		l4.hdr = skb_transport_header(skb);
 		network_hdr_len = skb_network_header_len(skb);
+
+		if (ip.v4->version == 6) {
+			exthdr = ip.hdr + sizeof(*ip.v6);
+			l4_proto = ip.v6->nexthdr;
+			l4.hdr = skb_transport_header(skb);
+			if (l4.hdr != exthdr &&
+			    ipv6_skip_exthdr_no_rthdr(skb, exthdr - skb->data,
+						      &l4_proto, &frag_off) < 0)
+				goto no_csum_offload;
+		}
 	}
 
 	get_inner_l3_l4_type(skb, &ip, &l4, TX_OFFLOAD_CSUM, &l3_type,
-- 
2.34.1

[PATCH net-next v2 7/7] fm10k: Don't do TX csum offload with routing header present

[Tom Herbert]

When determining if the L4 checksum in an IPv6 packet can be offloaded
on transmit, call ipv6_skip_exthdr_no_rthdr to check for the presence
of a routing header. If a routing header is present, that is the
function return less than zero, then don't offload checksum and call
skb_checksum_help instead.

Signed-off-by: Tom Herbert <tom@herbertland.com>
---
 drivers/net/ethernet/intel/fm10k/fm10k_main.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_main.c b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
index fc373472e4e1..b422fe7be427 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
@@ -832,9 +832,11 @@ static void fm10k_tx_csum(struct fm10k_ring *tx_ring,
 		if (likely((transport_hdr - network_hdr.raw) ==
 			   sizeof(struct ipv6hdr)))
 			break;
-		ipv6_skip_exthdr(skb, network_hdr.raw - skb->data +
-				      sizeof(struct ipv6hdr),
-				 &l4_hdr, &frag_off);
+		if (ipv6_skip_exthdr_no_rthdr(skb, network_hdr.raw - skb->data +
+					      sizeof(struct ipv6hdr),
+					      &l4_hdr, &frag_off) < 0)
+			goto no_csum_offload;
+
 		if (unlikely(frag_off))
 			l4_hdr = NEXTHDR_FRAGMENT;
 		break;
@@ -851,6 +853,7 @@ static void fm10k_tx_csum(struct fm10k_ring *tx_ring,
 			break;
 		fallthrough;
 	default:
+no_csum_offload:
 		if (unlikely(net_ratelimit())) {
 			dev_warn(tx_ring->dev,
 				 "partial checksum, version=%d l4 proto=%x\n",
-- 
2.34.1

Re: [PATCH net-next v2 0/7] drivers: Fix drivers doing TX csum offload with EH

[Przemek Kitszel]

On 7/1/24 21:55, Tom Herbert wrote:
> Several NICs would seem to support protocol specific TX checksum offload
> and allow for cases where an IPv6 packet contains extension headers.
> When deciding whether to offload a packet, ipv6_skip_exthdr is called
> to skip extension headers. The problem is that if a packet contains an
> IPv6 Routing Header then protocol specific checksum offload can't work,
> the destination IP address in the IPv6 header is not the same one that
> is used in the pseudo header for TCP or UDP. The correct address is
> derived from the last segment in the routing list (which itself might
> be obfuscated so that a device could even read it).

feels like there is a missing "not" after "could" - with it added, reads
fine (not a request to change, just being verbose about assumptions)

> 
> This patch set adds a new function ipv6_skip_exthdr_no_rthdr to be
> called in lieu of ipv6_skip_exthdr. If a routing header is present in
> a packet then ipv6_skip_exthdr_no_rthdr returns a value less than
> zero, this is an indication to the driver that TX checksum offload
> is not viable and it should call skb_checksum_help instead of
> offloading the checksum.
> 
> The i40e, iavf, ice, idpf, hinic, and fm10k are updated accordingly
> to call ipv6_skip_exthdr_no_rthdr.
> 
> Testing: The code compiles, but is otherwise untested due to lack of
> NIC hardware. It would be appreciated if someone with access to the
> hardware could test.

we could test intel ones (except fm10k) via @Tony's tree

> 
> v2: Fixed uninitialized variable in exthdrs_core.c
> 
> Tom Herbert (7):
>    ipv6: Add ipv6_skip_exthdr_no_rthdr
>    i40e: Don't do TX csum offload with routing header present
>    iavf: Don't do TX csum offload with routing header present
>    ice: Don't do TX csum offload with routing header present

sidenote:
our HW is supporting (among others) a GCO check-summing mode described
as: "Checksum 16bit (TCP/UDP) with no pseudo Header", but we have not
yet provided patches for that, and I don't even know if this mode
will be used (CC @Paul)

>    idpf: Don't do TX csum offload with routing header present
>    hinic: Don't do TX csum offload with routing header present
>    fm10k: Don't do TX csum offload with routing header present
> 
>   drivers/net/ethernet/huawei/hinic/hinic_tx.c  | 23 +++++++++++----
>   drivers/net/ethernet/intel/fm10k/fm10k_main.c |  9 ++++--
>   drivers/net/ethernet/intel/i40e/i40e_txrx.c   | 22 ++++++---------
>   drivers/net/ethernet/intel/iavf/iavf_txrx.c   | 20 ++++++-------
>   drivers/net/ethernet/intel/ice/ice_txrx.c     | 22 ++++++---------
>   .../ethernet/intel/idpf/idpf_singleq_txrx.c   | 28 +++++++++----------
>   include/net/ipv6.h                            | 17 +++++++++--
>   net/ipv6/exthdrs_core.c                       | 25 ++++++++++++-----
>   8 files changed, 98 insertions(+), 68 deletions(-)
> 

I have reviewed the patches and they conform to commit message/intent,
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
(for the series)

Re: [PATCH net-next v2 0/7] drivers: Fix drivers doing TX csum offload with EH

[Jakub Kicinski]

On Mon,  1 Jul 2024 12:55:00 -0700 Tom Herbert wrote:
> Testing: The code compiles, but is otherwise untested due to lack of
> NIC hardware. It would be appreciated if someone with access to the
> hardware could test.

Could you pop a script under tools/testing/selftests/drivers/net/
that'd exercise this?

This will hopefully guarantee good coverage soon, due to:
https://netdev.bots.linux.dev/devices.html

Re: [PATCH net-next v2 0/7] drivers: Fix drivers doing TX csum offload with EH

[Greenwalt, Paul]

On 7/2/2024 3:31 AM, Przemek Kitszel wrote:
> On 7/1/24 21:55, Tom Herbert wrote:
>> Several NICs would seem to support protocol specific TX checksum offload
>> and allow for cases where an IPv6 packet contains extension headers.
>> When deciding whether to offload a packet, ipv6_skip_exthdr is called
>> to skip extension headers. The problem is that if a packet contains an
>> IPv6 Routing Header then protocol specific checksum offload can't work,
>> the destination IP address in the IPv6 header is not the same one that
>> is used in the pseudo header for TCP or UDP. The correct address is
>> derived from the last segment in the routing list (which itself might
>> be obfuscated so that a device could even read it).
> 
> feels like there is a missing "not" after "could" - with it added, reads
> fine (not a request to change, just being verbose about assumptions)
> 
>>
>> This patch set adds a new function ipv6_skip_exthdr_no_rthdr to be
>> called in lieu of ipv6_skip_exthdr. If a routing header is present in
>> a packet then ipv6_skip_exthdr_no_rthdr returns a value less than
>> zero, this is an indication to the driver that TX checksum offload
>> is not viable and it should call skb_checksum_help instead of
>> offloading the checksum.
>>
>> The i40e, iavf, ice, idpf, hinic, and fm10k are updated accordingly
>> to call ipv6_skip_exthdr_no_rthdr.
>>
>> Testing: The code compiles, but is otherwise untested due to lack of
>> NIC hardware. It would be appreciated if someone with access to the
>> hardware could test.
> 
> we could test intel ones (except fm10k) via @Tony's tree
> 
>>
>> v2: Fixed uninitialized variable in exthdrs_core.c
>>
>> Tom Herbert (7):
>>    ipv6: Add ipv6_skip_exthdr_no_rthdr
>>    i40e: Don't do TX csum offload with routing header present
>>    iavf: Don't do TX csum offload with routing header present
>>    ice: Don't do TX csum offload with routing header present
> 
> sidenote:
> our HW is supporting (among others) a GCO check-summing mode described
> as: "Checksum 16bit (TCP/UDP) with no pseudo Header", but we have not
> yet provided patches for that, and I don't even know if this mode
> will be used (CC @Paul)
> 

We will be adding support for GCO "Checksum 16 with pseudo Headers" to
the ice driver. It will be off by default.

>>    idpf: Don't do TX csum offload with routing header present
>>    hinic: Don't do TX csum offload with routing header present
>>    fm10k: Don't do TX csum offload with routing header present
>>
>>   drivers/net/ethernet/huawei/hinic/hinic_tx.c  | 23 +++++++++++----
>>   drivers/net/ethernet/intel/fm10k/fm10k_main.c |  9 ++++--
>>   drivers/net/ethernet/intel/i40e/i40e_txrx.c   | 22 ++++++---------
>>   drivers/net/ethernet/intel/iavf/iavf_txrx.c   | 20 ++++++-------
>>   drivers/net/ethernet/intel/ice/ice_txrx.c     | 22 ++++++---------
>>   .../ethernet/intel/idpf/idpf_singleq_txrx.c   | 28 +++++++++----------
>>   include/net/ipv6.h                            | 17 +++++++++--
>>   net/ipv6/exthdrs_core.c                       | 25 ++++++++++++-----
>>   8 files changed, 98 insertions(+), 68 deletions(-)
>>
> 
> I have reviewed the patches and they conform to commit message/intent,
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> (for the series)

Re: [PATCH net-next v2 0/7] drivers: Fix drivers doing TX csum offload with EH

[Tom Herbert]

On Tue, Jul 2, 2024 at 6:46 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon,  1 Jul 2024 12:55:00 -0700 Tom Herbert wrote:
> > Testing: The code compiles, but is otherwise untested due to lack of
> > NIC hardware. It would be appreciated if someone with access to the
> > hardware could test.
>
> Could you pop a script under tools/testing/selftests/drivers/net/
> that'd exercise this?
>
> This will hopefully guarantee good coverage soon, due to:
> https://netdev.bots.linux.dev/devices.html

Sure. Thanks for the pointer.

Tom

Re: [PATCH net-next v2 0/7] drivers: Fix drivers doing TX csum offload with EH

[Przemek Kitszel]

On 7/3/24 16:38, Tom Herbert wrote:
> 
> 
> On Wed, Jul 3, 2024, 7:20 AM Greenwalt, Paul <paul.greenwalt@intel.com 
> <mailto:paul.greenwalt@intel.com>> wrote:
> 
> 
> 
>     On 7/2/2024 3:31 AM, Przemek Kitszel wrote:
>      > On 7/1/24 21:55, Tom Herbert wrote:
>      >> Several NICs would seem to support protocol specific TX checksum
>     offload
>      >> and allow for cases where an IPv6 packet contains extension headers.
>      >> When deciding whether to offload a packet, ipv6_skip_exthdr is
>     called
>      >> to skip extension headers. The problem is that if a packet
>     contains an
>      >> IPv6 Routing Header then protocol specific checksum offload
>     can't work,
>      >> the destination IP address in the IPv6 header is not the same
>     one that
>      >> is used in the pseudo header for TCP or UDP. The correct address is
>      >> derived from the last segment in the routing list (which itself
>     might
>      >> be obfuscated so that a device could even read it).
>      >
>      > feels like there is a missing "not" after "could" - with it
>     added, reads
>      > fine (not a request to change, just being verbose about assumptions)
>      >
>      >>
>      >> This patch set adds a new function ipv6_skip_exthdr_no_rthdr to be
>      >> called in lieu of ipv6_skip_exthdr. If a routing header is
>     present in
>      >> a packet then ipv6_skip_exthdr_no_rthdr returns a value less than
>      >> zero, this is an indication to the driver that TX checksum offload
>      >> is not viable and it should call skb_checksum_help instead of
>      >> offloading the checksum.
>      >>
>      >> The i40e, iavf, ice, idpf, hinic, and fm10k are updated accordingly
>      >> to call ipv6_skip_exthdr_no_rthdr.
>      >>
>      >> Testing: The code compiles, but is otherwise untested due to lack of
>      >> NIC hardware. It would be appreciated if someone with access to the
>      >> hardware could test.
>      >
>      > we could test intel ones (except fm10k) via @Tony's tree
> 
> 
> Awesome! If you need any help let me know.
> 
>      >
>      >>
>      >> v2: Fixed uninitialized variable in exthdrs_core.c
>      >>
>      >> Tom Herbert (7):
>      >>    ipv6: Add ipv6_skip_exthdr_no_rthdr
>      >>    i40e: Don't do TX csum offload with routing header present
>      >>    iavf: Don't do TX csum offload with routing header present
>      >>    ice: Don't do TX csum offload with routing header present
>      >
>      > sidenote:
>      > our HW is supporting (among others) a GCO check-summing mode
>     described
>      > as: "Checksum 16bit (TCP/UDP) with no pseudo Header", but we have not
>      > yet provided patches for that, and I don't even know if this mode
>      > will be used (CC @Paul)
>      >
> 
>     We will be adding support for GCO "Checksum 16 with pseudo Headers" to
>     the ice driver. It will be off by default.
> 
> 
> I'm not sure what that means. 

IPv6 Routing Headers render (simple?) HW-offloaded checksumming wrong,
but not for the "no pseudo Header"-checksum. I have no idea how such
checksum will be useful, and we don't have plans to implement it, so
this is not much relevant. But that is just one mode that you could 
config our (new) HW.

> Can ICE just provide checksum-complete? 
> It's by far the simplest, most robust method with the most flexibility 
> for users. :-)

sorry, could you please elaborate?

Paul will implement GCO for ice and otherwise my understanding was that
our checksum is fine. Is there a room for improvement?

> 
> Tom
> 
> 
>      >>    idpf: Don't do TX csum offload with routing header present
>      >>    hinic: Don't do TX csum offload with routing header present
>      >>    fm10k: Don't do TX csum offload with routing header present
>      >>
>      >>   drivers/net/ethernet/huawei/hinic/hinic_tx.c  | 23 +++++++++++----
>      >>   drivers/net/ethernet/intel/fm10k/fm10k_main.c |  9 ++++--
>      >>   drivers/net/ethernet/intel/i40e/i40e_txrx.c   | 22 ++++++---------
>      >>   drivers/net/ethernet/intel/iavf/iavf_txrx.c   | 20 ++++++-------
>      >>   drivers/net/ethernet/intel/ice/ice_txrx.c     | 22 ++++++---------
>      >>   .../ethernet/intel/idpf/idpf_singleq_txrx.c   | 28
>     +++++++++----------
>      >>   include/net/ipv6.h                            | 17 +++++++++--
>      >>   net/ipv6/exthdrs_core.c                       | 25
>     ++++++++++++-----
>      >>   8 files changed, 98 insertions(+), 68 deletions(-)
>      >>
>      >
>      > I have reviewed the patches and they conform to commit
>     message/intent,
>      > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com
>     <mailto:przemyslaw.kitszel@intel.com>>
>      > (for the series)
> 

Re: [PATCH net-next v2 0/7] drivers: Fix drivers doing TX csum offload with EH

[Tom Herbert]

On Wed, Jul 3, 2024 at 8:03 AM Przemek Kitszel
<przemyslaw.kitszel@intel.com> wrote:
>
> On 7/3/24 16:38, Tom Herbert wrote:
> >
> >
> > On Wed, Jul 3, 2024, 7:20 AM Greenwalt, Paul <paul.greenwalt@intel.com
> > <mailto:paul.greenwalt@intel.com>> wrote:
> >
> >
> >
> >     On 7/2/2024 3:31 AM, Przemek Kitszel wrote:
> >      > On 7/1/24 21:55, Tom Herbert wrote:
> >      >> Several NICs would seem to support protocol specific TX checksum
> >     offload
> >      >> and allow for cases where an IPv6 packet contains extension headers.
> >      >> When deciding whether to offload a packet, ipv6_skip_exthdr is
> >     called
> >      >> to skip extension headers. The problem is that if a packet
> >     contains an
> >      >> IPv6 Routing Header then protocol specific checksum offload
> >     can't work,
> >      >> the destination IP address in the IPv6 header is not the same
> >     one that
> >      >> is used in the pseudo header for TCP or UDP. The correct address is
> >      >> derived from the last segment in the routing list (which itself
> >     might
> >      >> be obfuscated so that a device could even read it).
> >      >
> >      > feels like there is a missing "not" after "could" - with it
> >     added, reads
> >      > fine (not a request to change, just being verbose about assumptions)
> >      >
> >      >>
> >      >> This patch set adds a new function ipv6_skip_exthdr_no_rthdr to be
> >      >> called in lieu of ipv6_skip_exthdr. If a routing header is
> >     present in
> >      >> a packet then ipv6_skip_exthdr_no_rthdr returns a value less than
> >      >> zero, this is an indication to the driver that TX checksum offload
> >      >> is not viable and it should call skb_checksum_help instead of
> >      >> offloading the checksum.
> >      >>
> >      >> The i40e, iavf, ice, idpf, hinic, and fm10k are updated accordingly
> >      >> to call ipv6_skip_exthdr_no_rthdr.
> >      >>
> >      >> Testing: The code compiles, but is otherwise untested due to lack of
> >      >> NIC hardware. It would be appreciated if someone with access to the
> >      >> hardware could test.
> >      >
> >      > we could test intel ones (except fm10k) via @Tony's tree
> >
> >
> > Awesome! If you need any help let me know.
> >
> >      >
> >      >>
> >      >> v2: Fixed uninitialized variable in exthdrs_core.c
> >      >>
> >      >> Tom Herbert (7):
> >      >>    ipv6: Add ipv6_skip_exthdr_no_rthdr
> >      >>    i40e: Don't do TX csum offload with routing header present
> >      >>    iavf: Don't do TX csum offload with routing header present
> >      >>    ice: Don't do TX csum offload with routing header present
> >      >
> >      > sidenote:
> >      > our HW is supporting (among others) a GCO check-summing mode
> >     described
> >      > as: "Checksum 16bit (TCP/UDP) with no pseudo Header", but we have not
> >      > yet provided patches for that, and I don't even know if this mode
> >      > will be used (CC @Paul)
> >      >
> >
> >     We will be adding support for GCO "Checksum 16 with pseudo Headers" to
> >     the ice driver. It will be off by default.
> >
> >
> > I'm not sure what that means.
>
> IPv6 Routing Headers render (simple?) HW-offloaded checksumming wrong,
> but not for the "no pseudo Header"-checksum. I have no idea how such
> checksum will be useful, and we don't have plans to implement it, so
> this is not much relevant. But that is just one mode that you could
> config our (new) HW.
>
> > Can ICE just provide checksum-complete?
> > It's by far the simplest, most robust method with the most flexibility
> > for users. :-)
>
> sorry, could you please elaborate?
>
> Paul will implement GCO for ice and otherwise my understanding was that
> our checksum is fine. Is there a room for improvement?

Przemek,

No, there's plenty of room for improvement :-). This is protocol
specific checksum offload versus protocol agnostic checksum offload,
and the opinion of the community has been clear for a long time:
protocol agnostic checksum offload is the preferred method and
protocol specific checksum offload is deprecated. This is true for
both transmit and receive checksum offload. For background see Dave
Miller's presentation on this from 2016:
https://www.youtube.com/watch?v=6VgmazGwL_Y.

Protocol agnostic checksum offload isn't just a to have, it addresses
many bugs in protocol specific checksum offload. This patch set
addresses one obvious bug, but there are others. For instance, in IETF
there is a proposal in spring WG to do SRv6 without a routing header
that would make the checksum incorrect on the wire. This will break
protocol specific TX checksum offload and there's nothing to key on in
the packet like an RH  so that a driver would know the offload will
fail. I'm really not sure how we could fix this without major surgery
in the stack. Use protocol agnostic checksum offload and it "just
works" (the proposal to purposely send a bad checksum on the wire
without a RH is a bad idea in general, but I'm not sure we'll be able
to stop it in IETF).

And not to pick on the ICE driver, but please take a look at the
function ice_tx_csum. This function is called on every packet just to
evaluate whether the device is going to be able to offload the packet.
Basically, it parses the packet on transmit to make sure that the
device will be able to parse the packet (this is where we need to call
ipv6_skip_exthdr_no_rthdr). This function is 180 LOC! If the device
properly supports protocol agnostic checksum offload all that is
needed is to write the start offset and checksum offset into the
receive descriptor. Maybe there's some checks on the offset values,
but I can't see that needing more than ten LOC--  it's much less
susceptible to bugs, more robust, and works with a much wider set of
protocol combinations.

BTW, this patch set is the first in a series to formally deprecate and
remove protocol specific checksum offloads from the core kernel. IMO,
we've waited long enough! My intent is to remove CHECKSUM_UNNECESSARY,
NETIF_F_IP_CSUM, and NETIF_F_IPV6_CSUM. (note comment in skbuff.h "New
devices should use %NETIF_F_HW_CSUM to indicate checksum offload
capability."). Helper functions will be provided to support legacy
devices.

Tom

>
> >
> > Tom
> >
> >
> >      >>    idpf: Don't do TX csum offload with routing header present
> >      >>    hinic: Don't do TX csum offload with routing header present
> >      >>    fm10k: Don't do TX csum offload with routing header present
> >      >>
> >      >>   drivers/net/ethernet/huawei/hinic/hinic_tx.c  | 23 +++++++++++----
> >      >>   drivers/net/ethernet/intel/fm10k/fm10k_main.c |  9 ++++--
> >      >>   drivers/net/ethernet/intel/i40e/i40e_txrx.c   | 22 ++++++---------
> >      >>   drivers/net/ethernet/intel/iavf/iavf_txrx.c   | 20 ++++++-------
> >      >>   drivers/net/ethernet/intel/ice/ice_txrx.c     | 22 ++++++---------
> >      >>   .../ethernet/intel/idpf/idpf_singleq_txrx.c   | 28
> >     +++++++++----------
> >      >>   include/net/ipv6.h                            | 17 +++++++++--
> >      >>   net/ipv6/exthdrs_core.c                       | 25
> >     ++++++++++++-----
> >      >>   8 files changed, 98 insertions(+), 68 deletions(-)
> >      >>
> >      >
> >      > I have reviewed the patches and they conform to commit
> >     message/intent,
> >      > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com
> >     <mailto:przemyslaw.kitszel@intel.com>>
> >      > (for the series)
> >
>