[PATCH net v2] net/mlx4_en: don't set CHECKSUM_COMPLETE on SCTP packets

[PATCH net v2] net/mlx4_en: don't set CHECKSUM_COMPLETE on SCTP packets

[Davide Caratti]

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.

V2: don't test MLX4_CQE_STATUS_IPV6 if MLX4_CQE_STATUS_IPV4 is set

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 | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
index 436f768..bf16380 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
@@ -574,16 +574,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 (unlikely(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)
@@ -594,17 +599,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));
@@ -627,11 +635,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;
 }
-- 
2.9.4

Re: [PATCH net v2] net/mlx4_en: don't set CHECKSUM_COMPLETE on SCTP packets

[David Miller]

From: Davide Caratti <dcaratti@redhat.com>
Date: Thu,  3 Aug 2017 22:54:48 +0200

> 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.
> 
> V2: don't test MLX4_CQE_STATUS_IPV6 if MLX4_CQE_STATUS_IPV4 is set
> 
> 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>

Can I get reviews from some Mellanox folks please?

Re: [PATCH net v2] net/mlx4_en: don't set CHECKSUM_COMPLETE on SCTP packets

[Saeed Mahameed]

On Thu, Aug 3, 2017 at 11:54 PM, Davide Caratti <dcaratti@redhat.com> 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.
>
> V2: don't test MLX4_CQE_STATUS_IPV6 if MLX4_CQE_STATUS_IPV4 is set
>
> 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 | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> index 436f768..bf16380 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> @@ -574,16 +574,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 (unlikely(ipproto == IPPROTO_SCTP))
> +               return -1;
>
Hi Davide

If you got to here then it means this is a non UDP/TCP ipv4 packet and
the HW failed to validate it's checksum but
you get from the connectx3 HW a 1's complement 16-bit sum of IP
Payload + IP pseudo-header.
so if you return -1 here the driver will report checksum none for this
packet (and you will abandon any checsum offload/help from HW).

I am not an SCTP expert but it seems that you decided here that
connectX3 hw checksum (described above) can't be used to calculate
SCTP packet checksum
is that correct?

If so, then i am ok with this patch.

>         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)
> @@ -594,17 +599,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));
> @@ -627,11 +635,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;
>  }
> --
> 2.9.4
>

Re: [PATCH net v2] net/mlx4_en: don't set CHECKSUM_COMPLETE on SCTP packets

[Davide Caratti]

On Tue, 2017-08-08 at 18:07 +0300, Saeed Mahameed wrote:
> >   {
> >          __u16 length_for_csum = 0;
> >          __wsum csum_pseudo_header = 0;
> > +       __u8 ipproto = iph->protocol;
> > +
> > +       if (unlikely(ipproto == IPPROTO_SCTP))
> > +               return -1;
> > 
> 
> Hi Davide
> 

hi Saeed,

thank you for looking at this!

> If you got to here then it means this is a non UDP/TCP ipv4 packet and
> the HW failed to validate it's checksum but
> you get from the connectx3 HW a 1's complement 16-bit sum of IP
> Payload + IP pseudo-header.
> so if you return -1 here the driver will report checksum none for this
> packet (and you will abandon any checsum offload/help from HW).
> 
> I am not an SCTP expert but it seems that you decided here that
> connectX3 hw checksum (described above) can't be used to calculate
> SCTP packet checksum
> is that correct?
> 
Yes, that's correct. SCTP uses CRC32c, not 1's complement (and does not use
pseudo-headers): therefore, the checksum computed by the NIC hardware can't
be used.

The issue I observed is skb->ip_summed set to CHECKSUM_COMPLETE, that made
CRC32c validation fail in my setup (that was a netfilter REJECT rule, matching
SCTP packets). AFAIK, CHECKSUM_COMPLETE is valid only for the Internet Checksum;
setting CHECKSUM_NONE on rx packets carrying IPPROTO_SCTP fixed my test scenario.

> If so, then i am ok with this patch.

I planned to post this some weeks ago, after agreeing v2 with Tariq
(https://www.spinics.net/lists/netdev/msg441231.html), but took some time to
find a ConnectX-3 (from what I saw the issue is not present on ConnectX-3 Pro,
since it has MLX4_RX_CSUM_MODE_VAL_NON_TCP_UDP bit set to 0).

regards,

Re: [PATCH net v2] net/mlx4_en: don't set CHECKSUM_COMPLETE on SCTP packets

[Saeed Mahameed]

On Tue, Aug 8, 2017 at 7:06 PM, Davide Caratti <dcaratti@redhat.com> wrote:
> On Tue, 2017-08-08 at 18:07 +0300, Saeed Mahameed wrote:
>> >   {
>> >          __u16 length_for_csum = 0;
>> >          __wsum csum_pseudo_header = 0;
>> > +       __u8 ipproto = iph->protocol;
>> > +
>> > +       if (unlikely(ipproto == IPPROTO_SCTP))
>> > +               return -1;
>> >
>>
>> Hi Davide
>>
>
> hi Saeed,
>
> thank you for looking at this!
>
>> If you got to here then it means this is a non UDP/TCP ipv4 packet and
>> the HW failed to validate it's checksum but
>> you get from the connectx3 HW a 1's complement 16-bit sum of IP
>> Payload + IP pseudo-header.
>> so if you return -1 here the driver will report checksum none for this
>> packet (and you will abandon any checsum offload/help from HW).
>>
>> I am not an SCTP expert but it seems that you decided here that
>> connectX3 hw checksum (described above) can't be used to calculate
>> SCTP packet checksum
>> is that correct?
>>
> Yes, that's correct. SCTP uses CRC32c, not 1's complement (and does not use
> pseudo-headers): therefore, the checksum computed by the NIC hardware can't
> be used.
>
> The issue I observed is skb->ip_summed set to CHECKSUM_COMPLETE, that made
> CRC32c validation fail in my setup (that was a netfilter REJECT rule, matching
> SCTP packets). AFAIK, CHECKSUM_COMPLETE is valid only for the Internet Checksum;
> setting CHECKSUM_NONE on rx packets carrying IPPROTO_SCTP fixed my test scenario.
>
>> If so, then i am ok with this patch.
>
> I planned to post this some weeks ago, after agreeing v2 with Tariq
> (https://www.spinics.net/lists/netdev/msg441231.html), but took some time to
> find a ConnectX-3 (from what I saw the issue is not present on ConnectX-3 Pro,
> since it has MLX4_RX_CSUM_MODE_VAL_NON_TCP_UDP bit set to 0).
>

Thanks for the clarification, I will ack the patch.

> regards,
> --
> davide
>

Re: [PATCH net v2] net/mlx4_en: don't set CHECKSUM_COMPLETE on SCTP packets

[Saeed Mahameed]

On Thu, Aug 3, 2017 at 11:54 PM, Davide Caratti <dcaratti@redhat.com> 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.
>
> V2: don't test MLX4_CQE_STATUS_IPV6 if MLX4_CQE_STATUS_IPV4 is set
>
> 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>

Acked-by: Saeed Mahameed <saeedm@mellanox.com>

Re: [PATCH net v2] net/mlx4_en: don't set CHECKSUM_COMPLETE on SCTP packets

[David Miller]

From: Saeed Mahameed <saeedm@dev.mellanox.co.il>
Date: Tue, 8 Aug 2017 19:16:52 +0300

> On Thu, Aug 3, 2017 at 11:54 PM, Davide Caratti <dcaratti@redhat.com> 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.
>>
>> V2: don't test MLX4_CQE_STATUS_IPV6 if MLX4_CQE_STATUS_IPV4 is set
>>
>> 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>
> 
> Acked-by: Saeed Mahameed <saeedm@mellanox.com>

Applied and queued up for -stable.