All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next 0/8] net: qualcomm: rmnet: MAPv4 download checksum cleanup, part 2
@ 2021-06-12 14:37 Alex Elder
  2021-06-12 14:37 ` [PATCH net-next 1/8] net: qualcomm: rmnet: remove some local variables Alex Elder
                   ` (7 more replies)
  0 siblings, 8 replies; 9+ messages in thread
From: Alex Elder @ 2021-06-12 14:37 UTC (permalink / raw)
  To: subashab, stranche, davem, kuba; +Cc: bjorn.andersson, netdev, linux-kernel

This is part 2 of a large series that reworks some code that handles
downloaded packets when MAPv4 checksum offload is enabled.  The
first part, which includes an overview, is here:
  https://lore.kernel.org/netdev/20210611190529.3085813-1-elder@linaro.org/

This second part of the series completes the simplification of this
handling code, removing unnecessary byte swaps and bitwise inversions
of checksum values, and along the way avoids the need for almost all
of the forced type casts.  The checksum field in an RMNet download
trailer is given __sum16_t type to accurately reflect the meaning of
that field.

					-Alex

Alex Elder (8):
  net: qualcomm: rmnet: remove some local variables
  net: qualcomm: rmnet: rearrange some NOTs
  net: qualcomm: rmnet: show that an intermediate sum is zero
  net: qualcomm: rmnet: return earlier for bad checksum
  net: qualcomm: rmnet: remove unneeded code
  net: qualcomm: rmnet: trailer value is a checksum
  net: qualcomm: rmnet: drop some unary NOTs
  net: qualcomm: rmnet: IPv6 payload length is simple

 .../ethernet/qualcomm/rmnet/rmnet_map_data.c  | 115 ++++++------------
 include/linux/if_rmnet.h                      |   2 +-
 2 files changed, 41 insertions(+), 76 deletions(-)

-- 
2.27.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH net-next 1/8] net: qualcomm: rmnet: remove some local variables
  2021-06-12 14:37 [PATCH net-next 0/8] net: qualcomm: rmnet: MAPv4 download checksum cleanup, part 2 Alex Elder
@ 2021-06-12 14:37 ` Alex Elder
  2021-06-12 14:37 ` [PATCH net-next 2/8] net: qualcomm: rmnet: rearrange some NOTs Alex Elder
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Alex Elder @ 2021-06-12 14:37 UTC (permalink / raw)
  To: subashab, stranche, davem, kuba; +Cc: bjorn.andersson, netdev, linux-kernel

In rmnet_map_ipv4_dl_csum_trailer(), remove the "csum_temp" and
"addend" local variables, and simplify a few lines of code.

Remove the "csum_temp", "csum_value", "ip6_hdr_csum", and "addend"
local variables in rmnet_map_ipv6_dl_csum_trailer(), and simplify a
few lines of code there as well.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 .../ethernet/qualcomm/rmnet/rmnet_map_data.c  | 37 +++++++------------
 1 file changed, 13 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index d4d23ab446ef5..3e6feef0fd252 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -35,10 +35,9 @@ rmnet_map_ipv4_dl_csum_trailer(struct sk_buff *skb,
 {
 	struct iphdr *ip4h = (struct iphdr *)skb->data;
 	void *txporthdr = skb->data + ip4h->ihl * 4;
-	__sum16 *csum_field, csum_temp, pseudo_csum;
+	__sum16 *csum_field, pseudo_csum;
 	__sum16 ip_payload_csum;
-	u16 csum_value_final;
-	__be16 addend;
+	__sum16 csum_value_final;
 
 	/* Computing the checksum over just the IPv4 header--including its
 	 * checksum field--should yield 0.  If it doesn't, the IP header
@@ -83,14 +82,11 @@ rmnet_map_ipv4_dl_csum_trailer(struct sk_buff *skb,
 	pseudo_csum = ~csum_tcpudp_magic(ip4h->saddr, ip4h->daddr,
 					 ntohs(ip4h->tot_len) - ip4h->ihl * 4,
 					 ip4h->protocol, 0);
-	addend = (__force __be16)pseudo_csum;
-	pseudo_csum = csum16_add(ip_payload_csum, addend);
+	pseudo_csum = csum16_add(ip_payload_csum, (__force __be16)pseudo_csum);
 
-	addend = (__force __be16)*csum_field;
-	csum_temp = ~csum16_sub(pseudo_csum, addend);
-	csum_value_final = (__force u16)csum_temp;
+	csum_value_final = ~csum16_sub(pseudo_csum, (__force __be16)*csum_field);
 
-	if (unlikely(csum_value_final == 0)) {
+	if (unlikely(!csum_value_final)) {
 		switch (ip4h->protocol) {
 		case IPPROTO_UDP:
 			/* RFC 768 - DL4 1's complement rule for UDP csum 0 */
@@ -105,7 +101,7 @@ rmnet_map_ipv4_dl_csum_trailer(struct sk_buff *skb,
 		}
 	}
 
-	if (csum_value_final == (__force u16)*csum_field) {
+	if (csum_value_final == *csum_field) {
 		priv->stats.csum_ok++;
 		return 0;
 	} else {
@@ -122,12 +118,10 @@ rmnet_map_ipv6_dl_csum_trailer(struct sk_buff *skb,
 {
 	struct ipv6hdr *ip6h = (struct ipv6hdr *)skb->data;
 	void *txporthdr = skb->data + sizeof(*ip6h);
-	__sum16 *csum_field, pseudo_csum, csum_temp;
-	__be16 ip6_hdr_csum, addend;
+	__sum16 *csum_field, pseudo_csum;
 	__sum16 ip6_payload_csum;
 	__be16 ip_header_csum;
-	u16 csum_value_final;
-	__be16 csum_value;
+	__sum16 csum_value_final;
 	u32 length;
 
 	/* Checksum offload is only supported for UDP and TCP protocols;
@@ -145,23 +139,18 @@ rmnet_map_ipv6_dl_csum_trailer(struct sk_buff *skb,
 	 * of the IP header from the trailer checksum.  We then add the
 	 * checksum computed over the pseudo header.
 	 */
-	csum_value = ~csum_trailer->csum_value;
 	ip_header_csum = (__force __be16)ip_fast_csum(ip6h, sizeof(*ip6h) / 4);
-	ip6_hdr_csum = (__force __be16)~ip_header_csum;
-	ip6_payload_csum = csum16_sub((__force __sum16)csum_value,
-				      ip6_hdr_csum);
+	ip6_payload_csum = csum16_sub((__force __sum16)~csum_trailer->csum_value,
+				      ~ip_header_csum);
 
 	length = (ip6h->nexthdr == IPPROTO_UDP) ?
 		 ntohs(((struct udphdr *)txporthdr)->len) :
 		 ntohs(ip6h->payload_len);
 	pseudo_csum = ~csum_ipv6_magic(&ip6h->saddr, &ip6h->daddr,
 				       length, ip6h->nexthdr, 0);
-	addend = (__force __be16)pseudo_csum;
-	pseudo_csum = csum16_add(ip6_payload_csum, addend);
+	pseudo_csum = csum16_add(ip6_payload_csum, (__force __be16)pseudo_csum);
 
-	addend = (__force __be16)*csum_field;
-	csum_temp = ~csum16_sub(pseudo_csum, addend);
-	csum_value_final = (__force u16)csum_temp;
+	csum_value_final = ~csum16_sub(pseudo_csum, (__force __be16)*csum_field);
 
 	if (unlikely(csum_value_final == 0)) {
 		switch (ip6h->nexthdr) {
@@ -180,7 +169,7 @@ rmnet_map_ipv6_dl_csum_trailer(struct sk_buff *skb,
 		}
 	}
 
-	if (csum_value_final == (__force u16)*csum_field) {
+	if (csum_value_final == *csum_field) {
 		priv->stats.csum_ok++;
 		return 0;
 	} else {
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH net-next 2/8] net: qualcomm: rmnet: rearrange some NOTs
  2021-06-12 14:37 [PATCH net-next 0/8] net: qualcomm: rmnet: MAPv4 download checksum cleanup, part 2 Alex Elder
  2021-06-12 14:37 ` [PATCH net-next 1/8] net: qualcomm: rmnet: remove some local variables Alex Elder
@ 2021-06-12 14:37 ` Alex Elder
  2021-06-12 14:37 ` [PATCH net-next 3/8] net: qualcomm: rmnet: show that an intermediate sum is zero Alex Elder
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Alex Elder @ 2021-06-12 14:37 UTC (permalink / raw)
  To: subashab, stranche, davem, kuba; +Cc: bjorn.andersson, netdev, linux-kernel

With the ones' complement arithmetic, the sum of two negated values
is equal to the negation of the sum of the two original values [1].
Rearrange the calculation ip6_payload_sum using this property.

[1] https://tools.ietf.org/html/rfc1071

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index 3e6feef0fd252..1b170e9189d8a 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -140,8 +140,8 @@ rmnet_map_ipv6_dl_csum_trailer(struct sk_buff *skb,
 	 * checksum computed over the pseudo header.
 	 */
 	ip_header_csum = (__force __be16)ip_fast_csum(ip6h, sizeof(*ip6h) / 4);
-	ip6_payload_csum = csum16_sub((__force __sum16)~csum_trailer->csum_value,
-				      ~ip_header_csum);
+	ip6_payload_csum = ~csum16_sub((__force __sum16)csum_trailer->csum_value,
+				       ip_header_csum);
 
 	length = (ip6h->nexthdr == IPPROTO_UDP) ?
 		 ntohs(((struct udphdr *)txporthdr)->len) :
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH net-next 3/8] net: qualcomm: rmnet: show that an intermediate sum is zero
  2021-06-12 14:37 [PATCH net-next 0/8] net: qualcomm: rmnet: MAPv4 download checksum cleanup, part 2 Alex Elder
  2021-06-12 14:37 ` [PATCH net-next 1/8] net: qualcomm: rmnet: remove some local variables Alex Elder
  2021-06-12 14:37 ` [PATCH net-next 2/8] net: qualcomm: rmnet: rearrange some NOTs Alex Elder
@ 2021-06-12 14:37 ` Alex Elder
  2021-06-12 14:37 ` [PATCH net-next 4/8] net: qualcomm: rmnet: return earlier for bad checksum Alex Elder
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Alex Elder @ 2021-06-12 14:37 UTC (permalink / raw)
  To: subashab, stranche, davem, kuba; +Cc: bjorn.andersson, netdev, linux-kernel

This patch simply demonstrates that a checksum value computed when
verifying an offloaded transport checksum value for both IPv4 and
IPv6 is (normally) 0.  It can be squashed into the next patch.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index 1b170e9189d8a..51909b8fa8a80 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -84,6 +84,11 @@ rmnet_map_ipv4_dl_csum_trailer(struct sk_buff *skb,
 					 ip4h->protocol, 0);
 	pseudo_csum = csum16_add(ip_payload_csum, (__force __be16)pseudo_csum);
 
+	/* The trailer checksum *includes* the checksum in the transport
+	 * header.  Adding that to the pseudo checksum will yield 0xffff
+	 * ("negative 0") if the message arrived intact.
+	 */
+	WARN_ON((__sum16)~pseudo_csum);
 	csum_value_final = ~csum16_sub(pseudo_csum, (__force __be16)*csum_field);
 
 	if (unlikely(!csum_value_final)) {
@@ -150,6 +155,10 @@ rmnet_map_ipv6_dl_csum_trailer(struct sk_buff *skb,
 				       length, ip6h->nexthdr, 0);
 	pseudo_csum = csum16_add(ip6_payload_csum, (__force __be16)pseudo_csum);
 
+	/* Adding the payload checksum to the pseudo checksum yields 0xffff
+	 * ("negative 0") if the message arrived intact.
+	 */
+	WARN_ON((__sum16)~pseudo_csum);
 	csum_value_final = ~csum16_sub(pseudo_csum, (__force __be16)*csum_field);
 
 	if (unlikely(csum_value_final == 0)) {
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH net-next 4/8] net: qualcomm: rmnet: return earlier for bad checksum
  2021-06-12 14:37 [PATCH net-next 0/8] net: qualcomm: rmnet: MAPv4 download checksum cleanup, part 2 Alex Elder
                   ` (2 preceding siblings ...)
  2021-06-12 14:37 ` [PATCH net-next 3/8] net: qualcomm: rmnet: show that an intermediate sum is zero Alex Elder
@ 2021-06-12 14:37 ` Alex Elder
  2021-06-12 14:37 ` [PATCH net-next 5/8] net: qualcomm: rmnet: remove unneeded code Alex Elder
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Alex Elder @ 2021-06-12 14:37 UTC (permalink / raw)
  To: subashab, stranche, davem, kuba; +Cc: bjorn.andersson, netdev, linux-kernel

In rmnet_map_ipv4_dl_csum_trailer(), if the sum of the trailer
checksum and the pseudo checksum is non-zero, checksum validation
has failed.  We can return an error as soon as we know that.

We can do the same thing in rmnet_map_ipv6_dl_csum_trailer().

Add some comments that explain where we're headed.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 .../ethernet/qualcomm/rmnet/rmnet_map_data.c  | 36 ++++++++++++++-----
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index 51909b8fa8a80..a05124eb8602e 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -76,6 +76,17 @@ rmnet_map_ipv4_dl_csum_trailer(struct sk_buff *skb,
 	 * We verified above that the IP header contributes zero to the
 	 * trailer checksum.  Therefore the checksum in the trailer is
 	 * just the checksum computed over the IP payload.
+
+	 * If the IP payload arrives intact, adding the pseudo header
+	 * checksum to the IP payload checksum will yield 0xffff (negative
+	 * zero).  This means the trailer checksum and the pseudo checksum
+	 * are additive inverses of each other.  Put another way, the
+	 * message passes the checksum test if the trailer checksum value
+	 * is the negated pseudo header checksum.
+	 *
+	 * Knowing this, we don't even need to examine the transport
+	 * header checksum value; it is already accounted for in the
+	 * checksum value found in the trailer.
 	 */
 	ip_payload_csum = (__force __sum16)~csum_trailer->csum_value;
 
@@ -84,11 +95,11 @@ rmnet_map_ipv4_dl_csum_trailer(struct sk_buff *skb,
 					 ip4h->protocol, 0);
 	pseudo_csum = csum16_add(ip_payload_csum, (__force __be16)pseudo_csum);
 
-	/* The trailer checksum *includes* the checksum in the transport
-	 * header.  Adding that to the pseudo checksum will yield 0xffff
-	 * ("negative 0") if the message arrived intact.
-	 */
-	WARN_ON((__sum16)~pseudo_csum);
+	/* The cast is required to ensure only the low 16 bits are examined */
+	if ((__sum16)~pseudo_csum) {
+		priv->stats.csum_validation_failed++;
+		return -EINVAL;
+	}
 	csum_value_final = ~csum16_sub(pseudo_csum, (__force __be16)*csum_field);
 
 	if (unlikely(!csum_value_final)) {
@@ -143,6 +154,11 @@ rmnet_map_ipv6_dl_csum_trailer(struct sk_buff *skb,
 	 * transport checksum from this, we first subract the contribution
 	 * of the IP header from the trailer checksum.  We then add the
 	 * checksum computed over the pseudo header.
+	 *
+	 * It's sufficient to compare the IP payload checksum with the
+	 * negated pseudo checksum to determine whether the packet
+	 * checksum was good.  (See further explanation in comments
+	 * in rmnet_map_ipv4_dl_csum_trailer()).
 	 */
 	ip_header_csum = (__force __be16)ip_fast_csum(ip6h, sizeof(*ip6h) / 4);
 	ip6_payload_csum = ~csum16_sub((__force __sum16)csum_trailer->csum_value,
@@ -155,10 +171,12 @@ rmnet_map_ipv6_dl_csum_trailer(struct sk_buff *skb,
 				       length, ip6h->nexthdr, 0);
 	pseudo_csum = csum16_add(ip6_payload_csum, (__force __be16)pseudo_csum);
 
-	/* Adding the payload checksum to the pseudo checksum yields 0xffff
-	 * ("negative 0") if the message arrived intact.
-	 */
-	WARN_ON((__sum16)~pseudo_csum);
+	/* The cast is required to ensure only the low 16 bits are examined */
+	if ((__sum16)~pseudo_csum) {
+		priv->stats.csum_validation_failed++;
+		return -EINVAL;
+	}
+
 	csum_value_final = ~csum16_sub(pseudo_csum, (__force __be16)*csum_field);
 
 	if (unlikely(csum_value_final == 0)) {
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH net-next 5/8] net: qualcomm: rmnet: remove unneeded code
  2021-06-12 14:37 [PATCH net-next 0/8] net: qualcomm: rmnet: MAPv4 download checksum cleanup, part 2 Alex Elder
                   ` (3 preceding siblings ...)
  2021-06-12 14:37 ` [PATCH net-next 4/8] net: qualcomm: rmnet: return earlier for bad checksum Alex Elder
@ 2021-06-12 14:37 ` Alex Elder
  2021-06-12 14:37 ` [PATCH net-next 6/8] net: qualcomm: rmnet: trailer value is a checksum Alex Elder
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Alex Elder @ 2021-06-12 14:37 UTC (permalink / raw)
  To: subashab, stranche, davem, kuba; +Cc: bjorn.andersson, netdev, linux-kernel

The previous patch makes rmnet_map_ipv4_dl_csum_trailer() return
early with an error if it is determined that the computed checksum
for the IP payload does not match what was expected.

If the computed checksum *does* match the expected value, the IP
payload (i.e., the transport message), can be considered good.
There is no need to do any further processing of the message.

This means a big block of code is unnecessary for validating the
transport checksum value, and can be removed.

Make comparable changes in rmnet_map_ipv6_dl_csum_trailer().

Signed-off-by: Alex Elder <elder@linaro.org>
---
 .../ethernet/qualcomm/rmnet/rmnet_map_data.c  | 75 ++++---------------
 1 file changed, 14 insertions(+), 61 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index a05124eb8602e..033b8ad3d7356 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -37,7 +37,6 @@ rmnet_map_ipv4_dl_csum_trailer(struct sk_buff *skb,
 	void *txporthdr = skb->data + ip4h->ihl * 4;
 	__sum16 *csum_field, pseudo_csum;
 	__sum16 ip_payload_csum;
-	__sum16 csum_value_final;
 
 	/* Computing the checksum over just the IPv4 header--including its
 	 * checksum field--should yield 0.  If it doesn't, the IP header
@@ -93,37 +92,15 @@ rmnet_map_ipv4_dl_csum_trailer(struct sk_buff *skb,
 	pseudo_csum = ~csum_tcpudp_magic(ip4h->saddr, ip4h->daddr,
 					 ntohs(ip4h->tot_len) - ip4h->ihl * 4,
 					 ip4h->protocol, 0);
-	pseudo_csum = csum16_add(ip_payload_csum, (__force __be16)pseudo_csum);
 
 	/* The cast is required to ensure only the low 16 bits are examined */
-	if ((__sum16)~pseudo_csum) {
+	if (ip_payload_csum != (__sum16)~pseudo_csum) {
 		priv->stats.csum_validation_failed++;
 		return -EINVAL;
 	}
-	csum_value_final = ~csum16_sub(pseudo_csum, (__force __be16)*csum_field);
 
-	if (unlikely(!csum_value_final)) {
-		switch (ip4h->protocol) {
-		case IPPROTO_UDP:
-			/* RFC 768 - DL4 1's complement rule for UDP csum 0 */
-			csum_value_final = ~csum_value_final;
-			break;
-
-		case IPPROTO_TCP:
-			/* DL4 Non-RFC compliant TCP checksum found */
-			if (*csum_field == (__force __sum16)0xFFFF)
-				csum_value_final = ~csum_value_final;
-			break;
-		}
-	}
-
-	if (csum_value_final == *csum_field) {
-		priv->stats.csum_ok++;
-		return 0;
-	} else {
-		priv->stats.csum_validation_failed++;
-		return -EINVAL;
-	}
+	priv->stats.csum_ok++;
+	return 0;
 }
 
 #if IS_ENABLED(CONFIG_IPV6)
@@ -137,7 +114,6 @@ rmnet_map_ipv6_dl_csum_trailer(struct sk_buff *skb,
 	__sum16 *csum_field, pseudo_csum;
 	__sum16 ip6_payload_csum;
 	__be16 ip_header_csum;
-	__sum16 csum_value_final;
 	u32 length;
 
 	/* Checksum offload is only supported for UDP and TCP protocols;
@@ -154,11 +130,6 @@ rmnet_map_ipv6_dl_csum_trailer(struct sk_buff *skb,
 	 * transport checksum from this, we first subract the contribution
 	 * of the IP header from the trailer checksum.  We then add the
 	 * checksum computed over the pseudo header.
-	 *
-	 * It's sufficient to compare the IP payload checksum with the
-	 * negated pseudo checksum to determine whether the packet
-	 * checksum was good.  (See further explanation in comments
-	 * in rmnet_map_ipv4_dl_csum_trailer()).
 	 */
 	ip_header_csum = (__force __be16)ip_fast_csum(ip6h, sizeof(*ip6h) / 4);
 	ip6_payload_csum = ~csum16_sub((__force __sum16)csum_trailer->csum_value,
@@ -169,40 +140,22 @@ rmnet_map_ipv6_dl_csum_trailer(struct sk_buff *skb,
 		 ntohs(ip6h->payload_len);
 	pseudo_csum = ~csum_ipv6_magic(&ip6h->saddr, &ip6h->daddr,
 				       length, ip6h->nexthdr, 0);
-	pseudo_csum = csum16_add(ip6_payload_csum, (__force __be16)pseudo_csum);
 
-	/* The cast is required to ensure only the low 16 bits are examined */
-	if ((__sum16)~pseudo_csum) {
+	/* It's sufficient to compare the IP payload checksum with the
+	 * negated pseudo checksum to determine whether the packet
+	 * checksum was good.  (See further explanation in comments
+	 * in rmnet_map_ipv4_dl_csum_trailer()).
+	 *
+	 * The cast is required to ensure only the low 16 bits are
+	 * examined.
+	 */
+	if (ip6_payload_csum != (__sum16)~pseudo_csum) {
 		priv->stats.csum_validation_failed++;
 		return -EINVAL;
 	}
 
-	csum_value_final = ~csum16_sub(pseudo_csum, (__force __be16)*csum_field);
-
-	if (unlikely(csum_value_final == 0)) {
-		switch (ip6h->nexthdr) {
-		case IPPROTO_UDP:
-			/* RFC 2460 section 8.1
-			 * DL6 One's complement rule for UDP checksum 0
-			 */
-			csum_value_final = ~csum_value_final;
-			break;
-
-		case IPPROTO_TCP:
-			/* DL6 Non-RFC compliant TCP checksum found */
-			if (*csum_field == (__force __sum16)0xFFFF)
-				csum_value_final = ~csum_value_final;
-			break;
-		}
-	}
-
-	if (csum_value_final == *csum_field) {
-		priv->stats.csum_ok++;
-		return 0;
-	} else {
-		priv->stats.csum_validation_failed++;
-		return -EINVAL;
-	}
+	priv->stats.csum_ok++;
+	return 0;
 }
 #endif
 
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH net-next 6/8] net: qualcomm: rmnet: trailer value is a checksum
  2021-06-12 14:37 [PATCH net-next 0/8] net: qualcomm: rmnet: MAPv4 download checksum cleanup, part 2 Alex Elder
                   ` (4 preceding siblings ...)
  2021-06-12 14:37 ` [PATCH net-next 5/8] net: qualcomm: rmnet: remove unneeded code Alex Elder
@ 2021-06-12 14:37 ` Alex Elder
  2021-06-12 14:37 ` [PATCH net-next 7/8] net: qualcomm: rmnet: drop some unary NOTs Alex Elder
  2021-06-12 14:37 ` [PATCH net-next 8/8] net: qualcomm: rmnet: IPv6 payload length is simple Alex Elder
  7 siblings, 0 replies; 9+ messages in thread
From: Alex Elder @ 2021-06-12 14:37 UTC (permalink / raw)
  To: subashab, stranche, davem, kuba; +Cc: bjorn.andersson, netdev, linux-kernel

The csum_value field in the rmnet_map_dl_csum_trailer structure is a
"real" Internet checksum.  It is a 16 bit value, in big endian format,
which represents an inverted ones' complement sum over pairs of bytes.

Make that clear by changing its type to __sum16.

This makes a typecast in rmnet_map_ipv4_dl_csum_trailer() and
another in rmnet_map_ipv6_dl_csum_trailer() unnecessary.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c | 5 ++---
 include/linux/if_rmnet.h                             | 2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index 033b8ad3d7356..610c8b5a8f46a 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -87,7 +87,7 @@ rmnet_map_ipv4_dl_csum_trailer(struct sk_buff *skb,
 	 * header checksum value; it is already accounted for in the
 	 * checksum value found in the trailer.
 	 */
-	ip_payload_csum = (__force __sum16)~csum_trailer->csum_value;
+	ip_payload_csum = ~csum_trailer->csum_value;
 
 	pseudo_csum = ~csum_tcpudp_magic(ip4h->saddr, ip4h->daddr,
 					 ntohs(ip4h->tot_len) - ip4h->ihl * 4,
@@ -132,8 +132,7 @@ rmnet_map_ipv6_dl_csum_trailer(struct sk_buff *skb,
 	 * checksum computed over the pseudo header.
 	 */
 	ip_header_csum = (__force __be16)ip_fast_csum(ip6h, sizeof(*ip6h) / 4);
-	ip6_payload_csum = ~csum16_sub((__force __sum16)csum_trailer->csum_value,
-				       ip_header_csum);
+	ip6_payload_csum = ~csum16_sub(csum_trailer->csum_value, ip_header_csum);
 
 	length = (ip6h->nexthdr == IPPROTO_UDP) ?
 		 ntohs(((struct udphdr *)txporthdr)->len) :
diff --git a/include/linux/if_rmnet.h b/include/linux/if_rmnet.h
index be17610a981ef..10e7521ecb6cd 100644
--- a/include/linux/if_rmnet.h
+++ b/include/linux/if_rmnet.h
@@ -25,7 +25,7 @@ struct rmnet_map_dl_csum_trailer {
 	u8 flags;			/* MAP_CSUM_DL_VALID_FLAG */
 	__be16 csum_start_offset;
 	__be16 csum_length;
-	__be16 csum_value;
+	__sum16 csum_value;
 } __aligned(1);
 
 /* rmnet_map_dl_csum_trailer flags field:
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH net-next 7/8] net: qualcomm: rmnet: drop some unary NOTs
  2021-06-12 14:37 [PATCH net-next 0/8] net: qualcomm: rmnet: MAPv4 download checksum cleanup, part 2 Alex Elder
                   ` (5 preceding siblings ...)
  2021-06-12 14:37 ` [PATCH net-next 6/8] net: qualcomm: rmnet: trailer value is a checksum Alex Elder
@ 2021-06-12 14:37 ` Alex Elder
  2021-06-12 14:37 ` [PATCH net-next 8/8] net: qualcomm: rmnet: IPv6 payload length is simple Alex Elder
  7 siblings, 0 replies; 9+ messages in thread
From: Alex Elder @ 2021-06-12 14:37 UTC (permalink / raw)
  To: subashab, stranche, davem, kuba; +Cc: bjorn.andersson, netdev, linux-kernel

We compare a payload checksum with a pseudo checksum value for
equality in rmnet_map_ipv4_dl_csum_trailer().  Both of those values
are computed with a unary NOT (~) operation.  The result of the
comparison is the same if we omit that NOT for both values.

Remove these operations in rmnet_map_ipv6_dl_csum_trailer() also.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 .../net/ethernet/qualcomm/rmnet/rmnet_map_data.c   | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index 610c8b5a8f46a..ed4737d0043d6 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -87,11 +87,11 @@ rmnet_map_ipv4_dl_csum_trailer(struct sk_buff *skb,
 	 * header checksum value; it is already accounted for in the
 	 * checksum value found in the trailer.
 	 */
-	ip_payload_csum = ~csum_trailer->csum_value;
+	ip_payload_csum = csum_trailer->csum_value;
 
-	pseudo_csum = ~csum_tcpudp_magic(ip4h->saddr, ip4h->daddr,
-					 ntohs(ip4h->tot_len) - ip4h->ihl * 4,
-					 ip4h->protocol, 0);
+	pseudo_csum = csum_tcpudp_magic(ip4h->saddr, ip4h->daddr,
+					ntohs(ip4h->tot_len) - ip4h->ihl * 4,
+					ip4h->protocol, 0);
 
 	/* The cast is required to ensure only the low 16 bits are examined */
 	if (ip_payload_csum != (__sum16)~pseudo_csum) {
@@ -132,13 +132,13 @@ rmnet_map_ipv6_dl_csum_trailer(struct sk_buff *skb,
 	 * checksum computed over the pseudo header.
 	 */
 	ip_header_csum = (__force __be16)ip_fast_csum(ip6h, sizeof(*ip6h) / 4);
-	ip6_payload_csum = ~csum16_sub(csum_trailer->csum_value, ip_header_csum);
+	ip6_payload_csum = csum16_sub(csum_trailer->csum_value, ip_header_csum);
 
 	length = (ip6h->nexthdr == IPPROTO_UDP) ?
 		 ntohs(((struct udphdr *)txporthdr)->len) :
 		 ntohs(ip6h->payload_len);
-	pseudo_csum = ~csum_ipv6_magic(&ip6h->saddr, &ip6h->daddr,
-				       length, ip6h->nexthdr, 0);
+	pseudo_csum = csum_ipv6_magic(&ip6h->saddr, &ip6h->daddr,
+				      length, ip6h->nexthdr, 0);
 
 	/* It's sufficient to compare the IP payload checksum with the
 	 * negated pseudo checksum to determine whether the packet
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH net-next 8/8] net: qualcomm: rmnet: IPv6 payload length is simple
  2021-06-12 14:37 [PATCH net-next 0/8] net: qualcomm: rmnet: MAPv4 download checksum cleanup, part 2 Alex Elder
                   ` (6 preceding siblings ...)
  2021-06-12 14:37 ` [PATCH net-next 7/8] net: qualcomm: rmnet: drop some unary NOTs Alex Elder
@ 2021-06-12 14:37 ` Alex Elder
  7 siblings, 0 replies; 9+ messages in thread
From: Alex Elder @ 2021-06-12 14:37 UTC (permalink / raw)
  To: subashab, stranche, davem, kuba; +Cc: bjorn.andersson, netdev, linux-kernel

We don't support any extension headers for IPv6 packets.  Extension
headers therefore contribute 0 bytes to the payload length.  As a
result we can just use the IPv6 payload length as the length used to
compute the pseudo header checksum for both UDP and TCP messages.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index ed4737d0043d6..a6ce22f60a00c 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -114,7 +114,6 @@ rmnet_map_ipv6_dl_csum_trailer(struct sk_buff *skb,
 	__sum16 *csum_field, pseudo_csum;
 	__sum16 ip6_payload_csum;
 	__be16 ip_header_csum;
-	u32 length;
 
 	/* Checksum offload is only supported for UDP and TCP protocols;
 	 * the packet cannot include any IPv6 extension headers
@@ -134,11 +133,9 @@ rmnet_map_ipv6_dl_csum_trailer(struct sk_buff *skb,
 	ip_header_csum = (__force __be16)ip_fast_csum(ip6h, sizeof(*ip6h) / 4);
 	ip6_payload_csum = csum16_sub(csum_trailer->csum_value, ip_header_csum);
 
-	length = (ip6h->nexthdr == IPPROTO_UDP) ?
-		 ntohs(((struct udphdr *)txporthdr)->len) :
-		 ntohs(ip6h->payload_len);
 	pseudo_csum = csum_ipv6_magic(&ip6h->saddr, &ip6h->daddr,
-				      length, ip6h->nexthdr, 0);
+				      ntohs(ip6h->payload_len),
+				      ip6h->nexthdr, 0);
 
 	/* It's sufficient to compare the IP payload checksum with the
 	 * negated pseudo checksum to determine whether the packet
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-06-12 14:39 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-12 14:37 [PATCH net-next 0/8] net: qualcomm: rmnet: MAPv4 download checksum cleanup, part 2 Alex Elder
2021-06-12 14:37 ` [PATCH net-next 1/8] net: qualcomm: rmnet: remove some local variables Alex Elder
2021-06-12 14:37 ` [PATCH net-next 2/8] net: qualcomm: rmnet: rearrange some NOTs Alex Elder
2021-06-12 14:37 ` [PATCH net-next 3/8] net: qualcomm: rmnet: show that an intermediate sum is zero Alex Elder
2021-06-12 14:37 ` [PATCH net-next 4/8] net: qualcomm: rmnet: return earlier for bad checksum Alex Elder
2021-06-12 14:37 ` [PATCH net-next 5/8] net: qualcomm: rmnet: remove unneeded code Alex Elder
2021-06-12 14:37 ` [PATCH net-next 6/8] net: qualcomm: rmnet: trailer value is a checksum Alex Elder
2021-06-12 14:37 ` [PATCH net-next 7/8] net: qualcomm: rmnet: drop some unary NOTs Alex Elder
2021-06-12 14:37 ` [PATCH net-next 8/8] net: qualcomm: rmnet: IPv6 payload length is simple Alex Elder

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.