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/[email protected]/
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
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 <[email protected]>
---
.../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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
.../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
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 <[email protected]>
---
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
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 <[email protected]>
---
.../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