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

[David Miller]

From: Saeed Mahameed 
Date: Tue, 8 Aug 2017 19:16:52 +0300

> On Thu, Aug 3, 2017 at 11:54 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.
>>
>> V2: don't test MLX4_CQE_STATUS_IPV6 if MLX4_CQE_STATUS_IPV4 is set
>>
>> Reported-by: Shuang Li 
>> Fixes: f8c6455bb04b ("net/mlx4_en: Extend checksum offloading by CHECKSUM 
>> COMPLETE")
>> Signed-off-by: Davide Caratti 
> 
> Acked-by: Saeed Mahameed 

Applied and queued up for -stable.

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  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 
> Fixes: f8c6455bb04b ("net/mlx4_en: Extend checksum offloading by CHECKSUM 
> COMPLETE")
> Signed-off-by: Davide Caratti 

Acked-by: Saeed Mahameed 

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  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

[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,
--
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  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 
> Fixes: f8c6455bb04b ("net/mlx4_en: Extend checksum offloading by CHECKSUM 
> COMPLETE")
> Signed-off-by: Davide Caratti 
> ---
>  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(>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 
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 
> Fixes: f8c6455bb04b ("net/mlx4_en: Extend checksum offloading by CHECKSUM 
> COMPLETE")
> Signed-off-by: Davide Caratti 

Can I get reviews from some Mellanox folks please?