[v3] net: fix the way how L4 checksum choice is tested

Message ID 20190628031309.24215-1-ivan.malov@oktetlabs.ru (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series [v3] net: fix the way how L4 checksum choice is tested |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/intel-Performance-Testing success Performance Testing PASS
ci/mellanox-Performance-Testing success Performance Testing PASS

Commit Message

Ivan Malov June 28, 2019, 3:13 a.m. UTC
  The API to prepare checksum offloads mistreats L4
checksum type enum values as self-contained flags.

Turning these flag checks into enum checks causes
warnings by GCC about possibly uninitialised IPv4
header pointer. The issue was found to show up in
the case of GCC versions 4.8.5 and 5.4.0, however,
it might be the case for a wider variety of other
versions. Initialise the pointer upon declaration.
and explain the reason behind this in the comment.

Fixes: 4fb7e803eb1a ("ethdev: add Tx preparation")
Cc: Tomasz Kulasek <tomaszx.kulasek@intel.com>
Cc: stable@dpdk.org

Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 lib/librte_net/rte_net.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
  

Comments

Stephen Hemminger June 28, 2019, 4:26 a.m. UTC | #1
On Fri, 28 Jun 2019 06:13:09 +0300
Ivan Malov <ivan.malov@oktetlabs.ru> wrote:

> The API to prepare checksum offloads mistreats L4
> checksum type enum values as self-contained flags.
> 
> Turning these flag checks into enum checks causes
> warnings by GCC about possibly uninitialised IPv4
> header pointer. The issue was found to show up in
> the case of GCC versions 4.8.5 and 5.4.0, however,
> it might be the case for a wider variety of other
> versions. Initialise the pointer upon declaration.
> and explain the reason behind this in the comment.
> 
> Fixes: 4fb7e803eb1a ("ethdev: add Tx preparation")
> Cc: Tomasz Kulasek <tomaszx.kulasek@intel.com>
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
>  lib/librte_net/rte_net.h | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
> index 7be69f8..d240206 100644
> --- a/lib/librte_net/rte_net.h
> +++ b/lib/librte_net/rte_net.h
> @@ -112,7 +112,8 @@ uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
>  static inline int
>  rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
>  {
> -	struct rte_ipv4_hdr *ipv4_hdr;
> +	/* Initialise ipv4_hdr to avoid false positive compiler warnings. */
> +	struct rte_ipv4_hdr *ipv4_hdr = NULL;
>  	struct rte_ipv6_hdr *ipv6_hdr;

Looks good, a smart compiler will just drop it.

Acked-by: Stephen Hemminger <stephen@networkplumber.org>
  
Ananyev, Konstantin June 28, 2019, 10:47 a.m. UTC | #2
> -----Original Message-----
> From: Ivan Malov [mailto:ivan.malov@oktetlabs.ru]
> Sent: Friday, June 28, 2019 4:13 AM
> To: Olivier Matz <olivier.matz@6wind.com>
> Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>; Stephen
> Hemminger <stephen@networkplumber.org>; Kulasek, TomaszX <tomaszx.kulasek@intel.com>; stable@dpdk.org
> Subject: [PATCH v3] net: fix the way how L4 checksum choice is tested
> 
> The API to prepare checksum offloads mistreats L4
> checksum type enum values as self-contained flags.
> 
> Turning these flag checks into enum checks causes
> warnings by GCC about possibly uninitialised IPv4
> header pointer. The issue was found to show up in
> the case of GCC versions 4.8.5 and 5.4.0, however,
> it might be the case for a wider variety of other
> versions. Initialise the pointer upon declaration.
> and explain the reason behind this in the comment.
> 
> Fixes: 4fb7e803eb1a ("ethdev: add Tx preparation")
> Cc: Tomasz Kulasek <tomaszx.kulasek@intel.com>
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
>  lib/librte_net/rte_net.h | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
> index 7be69f8..d240206 100644
> --- a/lib/librte_net/rte_net.h
> +++ b/lib/librte_net/rte_net.h
> @@ -112,7 +112,8 @@ uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
>  static inline int
>  rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
>  {
> -	struct rte_ipv4_hdr *ipv4_hdr;
> +	/* Initialise ipv4_hdr to avoid false positive compiler warnings. */
> +	struct rte_ipv4_hdr *ipv4_hdr = NULL;
>  	struct rte_ipv6_hdr *ipv6_hdr;
>  	struct rte_tcp_hdr *tcp_hdr;
>  	struct rte_udp_hdr *udp_hdr;
> @@ -150,7 +151,7 @@ uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
>  			ipv4_hdr->hdr_checksum = 0;
>  	}
> 
> -	if ((ol_flags & PKT_TX_UDP_CKSUM) == PKT_TX_UDP_CKSUM) {
> +	if ((ol_flags & PKT_TX_L4_MASK) == PKT_TX_UDP_CKSUM) {
>  		if (ol_flags & PKT_TX_IPV4) {
>  			udp_hdr = (struct rte_udp_hdr *)((char *)ipv4_hdr +
>  					m->l3_len);
> @@ -166,7 +167,7 @@ uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
>  			udp_hdr->dgram_cksum = rte_ipv6_phdr_cksum(ipv6_hdr,
>  					ol_flags);
>  		}
> -	} else if ((ol_flags & PKT_TX_TCP_CKSUM) ||
> +	} else if ((ol_flags & PKT_TX_L4_MASK) == PKT_TX_TCP_CKSUM ||
>  			(ol_flags & PKT_TX_TCP_SEG)) {
>  		if (ol_flags & PKT_TX_IPV4) {
>  			/* non-TSO tcp or TSO */
> --

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 1.8.3.1
  
Ferruh Yigit June 28, 2019, 4:24 p.m. UTC | #3
On 6/28/2019 11:47 AM, Ananyev, Konstantin wrote:
> 
> 
>> -----Original Message-----
>> From: Ivan Malov [mailto:ivan.malov@oktetlabs.ru]
>> Sent: Friday, June 28, 2019 4:13 AM
>> To: Olivier Matz <olivier.matz@6wind.com>
>> Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>; Stephen
>> Hemminger <stephen@networkplumber.org>; Kulasek, TomaszX <tomaszx.kulasek@intel.com>; stable@dpdk.org
>> Subject: [PATCH v3] net: fix the way how L4 checksum choice is tested
>>
>> The API to prepare checksum offloads mistreats L4
>> checksum type enum values as self-contained flags.
>>
>> Turning these flag checks into enum checks causes
>> warnings by GCC about possibly uninitialised IPv4
>> header pointer. The issue was found to show up in
>> the case of GCC versions 4.8.5 and 5.4.0, however,
>> it might be the case for a wider variety of other
>> versions. Initialise the pointer upon declaration.
>> and explain the reason behind this in the comment.
>>
>> Fixes: 4fb7e803eb1a ("ethdev: add Tx preparation")
>> Cc: Tomasz Kulasek <tomaszx.kulasek@intel.com>
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
>> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> 

Applied to dpdk-next-net/master, thanks.
  

Patch

diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
index 7be69f8..d240206 100644
--- a/lib/librte_net/rte_net.h
+++ b/lib/librte_net/rte_net.h
@@ -112,7 +112,8 @@  uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
 static inline int
 rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
 {
-	struct rte_ipv4_hdr *ipv4_hdr;
+	/* Initialise ipv4_hdr to avoid false positive compiler warnings. */
+	struct rte_ipv4_hdr *ipv4_hdr = NULL;
 	struct rte_ipv6_hdr *ipv6_hdr;
 	struct rte_tcp_hdr *tcp_hdr;
 	struct rte_udp_hdr *udp_hdr;
@@ -150,7 +151,7 @@  uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
 			ipv4_hdr->hdr_checksum = 0;
 	}
 
-	if ((ol_flags & PKT_TX_UDP_CKSUM) == PKT_TX_UDP_CKSUM) {
+	if ((ol_flags & PKT_TX_L4_MASK) == PKT_TX_UDP_CKSUM) {
 		if (ol_flags & PKT_TX_IPV4) {
 			udp_hdr = (struct rte_udp_hdr *)((char *)ipv4_hdr +
 					m->l3_len);
@@ -166,7 +167,7 @@  uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
 			udp_hdr->dgram_cksum = rte_ipv6_phdr_cksum(ipv6_hdr,
 					ol_flags);
 		}
-	} else if ((ol_flags & PKT_TX_TCP_CKSUM) ||
+	} else if ((ol_flags & PKT_TX_L4_MASK) == PKT_TX_TCP_CKSUM ||
 			(ol_flags & PKT_TX_TCP_SEG)) {
 		if (ol_flags & PKT_TX_IPV4) {
 			/* non-TSO tcp or TSO */