netdev - Re: [PATCH net-next] net/mlx4_en: don't set CHECKSUM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1497891850.2600.9.camel@redhat.com>
Date:   Mon, 19 Jun 2017 19:04:10 +0200
From:   Davide Caratti <dcaratti@...hat.com>
To:     Tariq Toukan <tariqt@...lanox.com>, netdev@...r.kernel.org
Cc:     "David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH net-next] net/mlx4_en: don't set CHECKSUM_COMPLETE on
 SCTP packets

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