* [PATCH net-next] net/mlx4_en: don't set CHECKSUM_COMPLETE on SCTP packets
@ 2017-06-16 14:01 Davide Caratti
2017-06-18 11:10 ` Tariq Toukan
0 siblings, 1 reply; 4+ messages in thread
From: Davide Caratti @ 2017-06-16 14:01 UTC (permalink / raw)
To: netdev; +Cc: Tariq Toukan, David S . Miller
if the NIC fails to validate the checksum on TCP/UDP, and validation of IP
checksum is successful, the driver subtracts the pseudo-header checksum
from the value obtained by the hardware and sets CHECKSUM_COMPLETE. Don't
do that if protocol is IPPROTO_SCTP, otherwise CRC32c validation fails.
Reported-by: Shuang Li <shuali@redhat.com>
Fixes: f8c6455bb04b ("net/mlx4_en: Extend checksum offloading by CHECKSUM COMPLETE")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
drivers/net/ethernet/mellanox/mlx4/en_rx.c | 33 +++++++++++++++++++-----------
1 file changed, 21 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
index 77abd18..d9293e4 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
@@ -572,16 +572,21 @@ static inline __wsum get_fixed_vlan_csum(__wsum hw_checksum,
* header, the HW adds it. To address that, we are subtracting the pseudo
* header checksum from the checksum value provided by the HW.
*/
-static void get_fixed_ipv4_csum(__wsum hw_checksum, struct sk_buff *skb,
- struct iphdr *iph)
+static int get_fixed_ipv4_csum(__wsum hw_checksum, struct sk_buff *skb,
+ struct iphdr *iph)
{
__u16 length_for_csum = 0;
__wsum csum_pseudo_header = 0;
+ __u8 ipproto = iph->protocol;
+
+ if (ipproto == IPPROTO_SCTP)
+ return -1;
length_for_csum = (be16_to_cpu(iph->tot_len) - (iph->ihl << 2));
csum_pseudo_header = csum_tcpudp_nofold(iph->saddr, iph->daddr,
- length_for_csum, iph->protocol, 0);
+ length_for_csum, ipproto, 0);
skb->csum = csum_sub(hw_checksum, csum_pseudo_header);
+ return 0;
}
#if IS_ENABLED(CONFIG_IPV6)
@@ -592,17 +597,20 @@ static void get_fixed_ipv4_csum(__wsum hw_checksum, struct sk_buff *skb,
static int get_fixed_ipv6_csum(__wsum hw_checksum, struct sk_buff *skb,
struct ipv6hdr *ipv6h)
{
+ __u8 nexthdr = ipv6h->nexthdr;
__wsum csum_pseudo_hdr = 0;
- if (unlikely(ipv6h->nexthdr == IPPROTO_FRAGMENT ||
- ipv6h->nexthdr == IPPROTO_HOPOPTS))
+ if (unlikely(nexthdr == IPPROTO_FRAGMENT ||
+ nexthdr == IPPROTO_HOPOPTS ||
+ nexthdr == IPPROTO_SCTP))
return -1;
- hw_checksum = csum_add(hw_checksum, (__force __wsum)htons(ipv6h->nexthdr));
+ hw_checksum = csum_add(hw_checksum, (__force __wsum)htons(nexthdr));
csum_pseudo_hdr = csum_partial(&ipv6h->saddr,
sizeof(ipv6h->saddr) + sizeof(ipv6h->daddr), 0);
csum_pseudo_hdr = csum_add(csum_pseudo_hdr, (__force __wsum)ipv6h->payload_len);
- csum_pseudo_hdr = csum_add(csum_pseudo_hdr, (__force __wsum)ntohs(ipv6h->nexthdr));
+ csum_pseudo_hdr = csum_add(csum_pseudo_hdr,
+ (__force __wsum)htons(nexthdr));
skb->csum = csum_sub(hw_checksum, csum_pseudo_hdr);
skb->csum = csum_add(skb->csum, csum_partial(ipv6h, sizeof(struct ipv6hdr), 0));
@@ -624,12 +632,13 @@ static int check_csum(struct mlx4_cqe *cqe, struct sk_buff *skb, void *va,
hdr += sizeof(struct vlan_hdr);
}
- if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV4))
- get_fixed_ipv4_csum(hw_checksum, skb, hdr);
+ if ((cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV4)) &&
+ (unlikely(get_fixed_ipv4_csum(hw_checksum, skb, hdr))))
+ return -1;
#if IS_ENABLED(CONFIG_IPV6)
- else if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV6))
- if (unlikely(get_fixed_ipv6_csum(hw_checksum, skb, hdr)))
- return -1;
+ else if ((cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV6)) &&
+ (unlikely(get_fixed_ipv6_csum(hw_checksum, skb, hdr))))
+ return -1;
#endif
return 0;
}
--
2.9.4
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH net-next] net/mlx4_en: don't set CHECKSUM_COMPLETE on SCTP packets
2017-06-16 14:01 [PATCH net-next] net/mlx4_en: don't set CHECKSUM_COMPLETE on SCTP packets Davide Caratti
@ 2017-06-18 11:10 ` Tariq Toukan
2017-06-19 17:04 ` Davide Caratti
0 siblings, 1 reply; 4+ messages in thread
From: Tariq Toukan @ 2017-06-18 11:10 UTC (permalink / raw)
To: Davide Caratti, netdev; +Cc: Tariq Toukan, David S . Miller
On 16/06/2017 5:01 PM, Davide Caratti wrote:
> if the NIC fails to validate the checksum on TCP/UDP, and validation of IP
> checksum is successful, the driver subtracts the pseudo-header checksum
> from the value obtained by the hardware and sets CHECKSUM_COMPLETE. Don't
> do that if protocol is IPPROTO_SCTP, otherwise CRC32c validation fails.
>
> Reported-by: Shuang Li <shuali@redhat.com>
> Fixes: f8c6455bb04b ("net/mlx4_en: Extend checksum offloading by CHECKSUM COMPLETE")
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
> drivers/net/ethernet/mellanox/mlx4/en_rx.c | 33 +++++++++++++++++++-----------
> 1 file changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> index 77abd18..d9293e4 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> @@ -572,16 +572,21 @@ static inline __wsum get_fixed_vlan_csum(__wsum hw_checksum,
> * header, the HW adds it. To address that, we are subtracting the pseudo
> * header checksum from the checksum value provided by the HW.
> */
> -static void get_fixed_ipv4_csum(__wsum hw_checksum, struct sk_buff *skb,
> - struct iphdr *iph)
> +static int get_fixed_ipv4_csum(__wsum hw_checksum, struct sk_buff *skb,
> + struct iphdr *iph)
> {
> __u16 length_for_csum = 0;
> __wsum csum_pseudo_header = 0;
> + __u8 ipproto = iph->protocol;
> +
> + if (ipproto == IPPROTO_SCTP)
This is unlikely().
> + return -1;
>
> length_for_csum = (be16_to_cpu(iph->tot_len) - (iph->ihl << 2));
> csum_pseudo_header = csum_tcpudp_nofold(iph->saddr, iph->daddr,
> - length_for_csum, iph->protocol, 0);
> + length_for_csum, ipproto, 0);
> skb->csum = csum_sub(hw_checksum, csum_pseudo_header);
> + return 0;
> }
>
> #if IS_ENABLED(CONFIG_IPV6)
> @@ -592,17 +597,20 @@ static void get_fixed_ipv4_csum(__wsum hw_checksum, struct sk_buff *skb,
> static int get_fixed_ipv6_csum(__wsum hw_checksum, struct sk_buff *skb,
> struct ipv6hdr *ipv6h)
> {
> + __u8 nexthdr = ipv6h->nexthdr;
> __wsum csum_pseudo_hdr = 0;
>
> - if (unlikely(ipv6h->nexthdr == IPPROTO_FRAGMENT ||
> - ipv6h->nexthdr == IPPROTO_HOPOPTS))
> + if (unlikely(nexthdr == IPPROTO_FRAGMENT ||
> + nexthdr == IPPROTO_HOPOPTS ||
> + nexthdr == IPPROTO_SCTP))
> return -1;
> - hw_checksum = csum_add(hw_checksum, (__force __wsum)htons(ipv6h->nexthdr));
> + hw_checksum = csum_add(hw_checksum, (__force __wsum)htons(nexthdr));
>
> csum_pseudo_hdr = csum_partial(&ipv6h->saddr,
> sizeof(ipv6h->saddr) + sizeof(ipv6h->daddr), 0);
> csum_pseudo_hdr = csum_add(csum_pseudo_hdr, (__force __wsum)ipv6h->payload_len);
> - csum_pseudo_hdr = csum_add(csum_pseudo_hdr, (__force __wsum)ntohs(ipv6h->nexthdr));
> + csum_pseudo_hdr = csum_add(csum_pseudo_hdr,
> + (__force __wsum)htons(nexthdr));
>
> skb->csum = csum_sub(hw_checksum, csum_pseudo_hdr);
> skb->csum = csum_add(skb->csum, csum_partial(ipv6h, sizeof(struct ipv6hdr), 0));
> @@ -624,12 +632,13 @@ static int check_csum(struct mlx4_cqe *cqe, struct sk_buff *skb, void *va,
> hdr += sizeof(struct vlan_hdr);
> }
>
> - if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV4))
> - get_fixed_ipv4_csum(hw_checksum, skb, hdr);
> + if ((cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV4)) &&
> + (unlikely(get_fixed_ipv4_csum(hw_checksum, skb, hdr))))
No! The lazy evaluation trick is wrong here.
This way you'll end up going almost always to the else (ipv6) for the
wrong reason.
> + return -1;
> #if IS_ENABLED(CONFIG_IPV6)
> - else if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV6))
> - if (unlikely(get_fixed_ipv6_csum(hw_checksum, skb, hdr)))
> - return -1;
> + else if ((cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV6)) &&
> + (unlikely(get_fixed_ipv6_csum(hw_checksum, skb, hdr))))
> + return -1;
Let's not change this, might cause future bugs, similarly to the one above.
> #endif
> return 0;
> }
>
Regards,
Tariq
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net-next] net/mlx4_en: don't set CHECKSUM_COMPLETE on SCTP packets
2017-06-18 11:10 ` Tariq Toukan
@ 2017-06-19 17:04 ` Davide Caratti
2017-06-20 7:52 ` Tariq Toukan
0 siblings, 1 reply; 4+ messages in thread
From: Davide Caratti @ 2017-06-19 17:04 UTC (permalink / raw)
To: Tariq Toukan, netdev; +Cc: David S . Miller
hello Tariq,
On Sun, 2017-06-18 at 14:10 +0300, Tariq Toukan wrote:
> > @@ -624,12 +632,13 @@ static int check_csum(struct mlx4_cqe *cqe, struct sk_buff *skb, void *va,
> > hdr += sizeof(struct vlan_hdr);
> > }
> >
> > - if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV4))
> > - get_fixed_ipv4_csum(hw_checksum, skb, hdr);
> > + if ((cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV4)) &&
> > + (unlikely(get_fixed_ipv4_csum(hw_checksum, skb, hdr))))
>
> No! The lazy evaluation trick is wrong here.
> This way you'll end up going almost always to the else (ipv6) for the
> wrong reason.
you are right! thanks for spotting this.
> > + return -1;
> > #if IS_ENABLED(CONFIG_IPV6)
> > - else if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV6))
> > - if (unlikely(get_fixed_ipv6_csum(hw_checksum, skb, hdr)))
> > - return -1;
> > + else if ((cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV6)) &&
> > + (unlikely(get_fixed_ipv6_csum(hw_checksum, skb, hdr))))
> > + return -1;
>
> Let's not change this, might cause future bugs, similarly to the one above.
> > #endif
> > return 0;
> > }
maybe we can avoid adding braces, remove that 'else' keyword and the nested 'if',
thus saving one line, given that check_csum() returns the same set of values as
get_fixed_ipv{4,6}_checksum(), with the same meaning (-1 => go with CHECKSUM_NONE,
0 => go with CHECKSUM_COMPLETE).
---- >8 ----
@@ -625,11 +633,10 @@ static int check_csum(struct mlx4_cqe *cqe, struct sk_buff *skb, void *va,
}
if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV4))
- get_fixed_ipv4_csum(hw_checksum, skb, hdr);
+ return get_fixed_ipv4_csum(hw_checksum, skb, hdr);
#if IS_ENABLED(CONFIG_IPV6)
- else if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV6))
- if (unlikely(get_fixed_ipv6_csum(hw_checksum, skb, hdr)))
- return -1;
+ if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV6))
+ return get_fixed_ipv6_csum(hw_checksum, skb, hdr);
#endif
return 0;
}
---- 8< ----
I will test and repost a v2 with this modification, unless you have any
objections. Thank you in advance!
regards
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net-next] net/mlx4_en: don't set CHECKSUM_COMPLETE on SCTP packets
2017-06-19 17:04 ` Davide Caratti
@ 2017-06-20 7:52 ` Tariq Toukan
0 siblings, 0 replies; 4+ messages in thread
From: Tariq Toukan @ 2017-06-20 7:52 UTC (permalink / raw)
To: Davide Caratti, Tariq Toukan, netdev; +Cc: David S . Miller
On 19/06/2017 8:04 PM, Davide Caratti wrote:
> hello Tariq,
> On Sun, 2017-06-18 at 14:10 +0300, Tariq Toukan wrote:
>>> @@ -624,12 +632,13 @@ static int check_csum(struct mlx4_cqe *cqe, struct sk_buff *skb, void *va,
>>> hdr += sizeof(struct vlan_hdr);
>>> }
>>>
>>> - if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV4))
>>> - get_fixed_ipv4_csum(hw_checksum, skb, hdr);
>>> + if ((cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV4)) &&
>>> + (unlikely(get_fixed_ipv4_csum(hw_checksum, skb, hdr))))
>>
>> No! The lazy evaluation trick is wrong here.
>> This way you'll end up going almost always to the else (ipv6) for the
>> wrong reason.
>
> you are right! thanks for spotting this.
>
>>> + return -1;
>>> #if IS_ENABLED(CONFIG_IPV6)
>>> - else if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV6))
>>> - if (unlikely(get_fixed_ipv6_csum(hw_checksum, skb, hdr)))
>>> - return -1;
>>> + else if ((cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV6)) &&
>>> + (unlikely(get_fixed_ipv6_csum(hw_checksum, skb, hdr))))
>>> + return -1;
>>
>> Let's not change this, might cause future bugs, similarly to the one above.
>>> #endif
>>> return 0;
>>> }
>
> maybe we can avoid adding braces, remove that 'else' keyword and the nested 'if',
> thus saving one line, given that check_csum() returns the same set of values as
> get_fixed_ipv{4,6}_checksum(), with the same meaning (-1 => go with CHECKSUM_NONE,
> 0 => go with CHECKSUM_COMPLETE).
>
Yeah this sounds good.
> ---- >8 ----
> @@ -625,11 +633,10 @@ static int check_csum(struct mlx4_cqe *cqe, struct sk_buff *skb, void *va,
> }
>
> if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV4))
> - get_fixed_ipv4_csum(hw_checksum, skb, hdr);
> + return get_fixed_ipv4_csum(hw_checksum, skb, hdr);
> #if IS_ENABLED(CONFIG_IPV6)
> - else if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV6))
> - if (unlikely(get_fixed_ipv6_csum(hw_checksum, skb, hdr)))
> - return -1;
> + if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV6))
> + return get_fixed_ipv6_csum(hw_checksum, skb, hdr);
> #endif
> return 0;
> }
> ---- 8< ----
Looks good to me.
>
> I will test and repost a v2 with this modification, unless you have any
> objections. Thank you in advance!
> regards
> --
> davide
>
>
>
Thank you Davide.
Regards,
Tariq
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-06-20 7:52 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-06-16 14:01 [PATCH net-next] net/mlx4_en: don't set CHECKSUM_COMPLETE on SCTP packets Davide Caratti
2017-06-18 11:10 ` Tariq Toukan
2017-06-19 17:04 ` Davide Caratti
2017-06-20 7:52 ` Tariq Toukan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).