diff mbox series

net: fix the way how L4 checksum choice is tested

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

Checks

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

Commit Message

Ivan Malov May 29, 2019, 5:33 p.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. As GCC version 7.4.0 is not susceptible
to the said false positive assessment, this patch
maintains a compiler barrier for earlier versions.

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>
---
 lib/librte_net/rte_net.h | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

Comments

Ananyev, Konstantin June 24, 2019, 11:52 a.m. UTC | #1 
> 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. As GCC version 7.4.0 is not susceptible
> to the said false positive assessment, this patch
> maintains a compiler barrier for earlier versions.
> 
> 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>
> ---
>  lib/librte_net/rte_net.h | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
> index 7088584..fb09431 100644
> --- a/lib/librte_net/rte_net.h
> +++ b/lib/librte_net/rte_net.h
> @@ -151,7 +151,19 @@ uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
>  			ipv4_hdr->hdr_checksum = 0;
>  	}
> 

As I remember, saw something similar before...
Probably the eaiser way to overcome it, is just to always initialize ipv4_hdr above,
something like:

+ipv4_hdr = NULL;
if (ol_flags & PKT_TX_IPV4) {
                ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct rte_ipv4_hdr *,
                                inner_l3_offset);

                if (ol_flags & PKT_TX_IP_CKSUM)
                        ipv4_hdr->hdr_checksum = 0;
 }


> -	if ((ol_flags & PKT_TX_UDP_CKSUM) == PKT_TX_UDP_CKSUM) {
> +#ifdef GCC_VERSION
> +#if GCC_VERSION < 70400
> +	/*
> +	 * Earlier versions of GCC suspect access to possibly
> +	 * uninitialised ipv4_hdr in the code below, although
> +	 * the said variable is properly initialised above.
> +	 * Use compiler barrier to cope with the problem.
> +	 */
> +	rte_compiler_barrier();
> +#endif
> +#endif
> +
> +	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);
> @@ -167,7 +179,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 */
> --
> 1.8.3.1
Ananyev, Konstantin June 24, 2019, 12:01 p.m. UTC | #2 
> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Monday, June 24, 2019 12:52 PM
> To: 'Ivan Malov' <ivan.malov@oktetlabs.ru>; Olivier Matz <olivier.matz@6wind.com>
> Cc: dev@dpdk.org; Kulasek, TomaszX <tomaszx.kulasek@intel.com>; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] 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. As GCC version 7.4.0 is not susceptible
> > to the said false positive assessment, this patch
> > maintains a compiler barrier for earlier versions.
> >
> > 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>
> > ---
> >  lib/librte_net/rte_net.h | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
> > index 7088584..fb09431 100644
> > --- a/lib/librte_net/rte_net.h
> > +++ b/lib/librte_net/rte_net.h
> > @@ -151,7 +151,19 @@ uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
> >  			ipv4_hdr->hdr_checksum = 0;
> >  	}
> >
> 
> As I remember, saw something similar before...
> Probably the eaiser way to overcome it, is just to always initialize ipv4_hdr above,
> something like:
> 
> +ipv4_hdr = NULL;
> if (ol_flags & PKT_TX_IPV4) {
>                 ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct rte_ipv4_hdr *,
>                                 inner_l3_offset);
> 
>                 if (ol_flags & PKT_TX_IP_CKSUM)
>                         ipv4_hdr->hdr_checksum = 0;
>  }

As another possible option  always initialisze both, and then use either one or another,
depending on flags:

ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct rte_ipv4_hdr *, inner_l3_offset);
ipv6_hdr = rte_pktmbuf_mtod_offset(m, struct rte_ipv6_hdr *, inner_l3_offset);
....

> 
> 
> > -	if ((ol_flags & PKT_TX_UDP_CKSUM) == PKT_TX_UDP_CKSUM) {
> > +#ifdef GCC_VERSION
> > +#if GCC_VERSION < 70400
> > +	/*
> > +	 * Earlier versions of GCC suspect access to possibly
> > +	 * uninitialised ipv4_hdr in the code below, although
> > +	 * the said variable is properly initialised above.
> > +	 * Use compiler barrier to cope with the problem.
> > +	 */
> > +	rte_compiler_barrier();
> > +#endif
> > +#endif
> > +
> > +	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);
> > @@ -167,7 +179,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 */
> > --
> > 1.8.3.1
Andrew Rybchenko June 24, 2019, 12:16 p.m. UTC | #3 
On 6/24/19 3:01 PM, Ananyev, Konstantin wrote:
>
>> -----Original Message-----
>> From: Ananyev, Konstantin
>> Sent: Monday, June 24, 2019 12:52 PM
>> To: 'Ivan Malov' <ivan.malov@oktetlabs.ru>; Olivier Matz <olivier.matz@6wind.com>
>> Cc: dev@dpdk.org; Kulasek, TomaszX <tomaszx.kulasek@intel.com>; stable@dpdk.org
>> Subject: RE: [dpdk-dev] [PATCH] 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. As GCC version 7.4.0 is not susceptible
>>> to the said false positive assessment, this patch
>>> maintains a compiler barrier for earlier versions.
>>>
>>> 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>
>>> ---
>>>   lib/librte_net/rte_net.h | 16 ++++++++++++++--
>>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
>>> index 7088584..fb09431 100644
>>> --- a/lib/librte_net/rte_net.h
>>> +++ b/lib/librte_net/rte_net.h
>>> @@ -151,7 +151,19 @@ uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
>>>   			ipv4_hdr->hdr_checksum = 0;
>>>   	}
>>>
>> As I remember, saw something similar before...
>> Probably the eaiser way to overcome it, is just to always initialize ipv4_hdr above,
>> something like:
>>
>> +ipv4_hdr = NULL;
>> if (ol_flags & PKT_TX_IPV4) {
>>                  ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct rte_ipv4_hdr *,
>>                                  inner_l3_offset);
>>
>>                  if (ol_flags & PKT_TX_IP_CKSUM)
>>                          ipv4_hdr->hdr_checksum = 0;
>>   }
> As another possible option  always initialisze both, and then use either one or another,
> depending on flags:
>
> ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct rte_ipv4_hdr *, inner_l3_offset);
> ipv6_hdr = rte_pktmbuf_mtod_offset(m, struct rte_ipv6_hdr *, inner_l3_offset);
> ....

We have discussed the solution locally and rejected it since it just kills
the compiler help. It makes it possible to use invalid header and
compiler will not save you.

Suggested solution looks ugly, but at least it does not kill help from
compiler.

>>> -	if ((ol_flags & PKT_TX_UDP_CKSUM) == PKT_TX_UDP_CKSUM) {
>>> +#ifdef GCC_VERSION
>>> +#if GCC_VERSION < 70400
>>> +	/*
>>> +	 * Earlier versions of GCC suspect access to possibly
>>> +	 * uninitialised ipv4_hdr in the code below, although
>>> +	 * the said variable is properly initialised above.
>>> +	 * Use compiler barrier to cope with the problem.
>>> +	 */
>>> +	rte_compiler_barrier();
>>> +#endif
>>> +#endif
>>> +
>>> +	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);
>>> @@ -167,7 +179,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 */
>>> --
>>> 1.8.3.1
Ananyev, Konstantin June 27, 2019, 1:26 p.m. UTC | #4 
From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
Sent: Monday, June 24, 2019 1:17 PM
To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Ivan Malov <ivan.malov@oktetlabs.ru>; Olivier Matz <olivier.matz@6wind.com>
Cc: dev@dpdk.org; Kulasek, TomaszX <tomaszx.kulasek@intel.com>; stable@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] net: fix the way how L4 checksum choice is tested

On 6/24/19 3:01 PM, Ananyev, Konstantin wrote:





-----Original Message-----

From: Ananyev, Konstantin

Sent: Monday, June 24, 2019 12:52 PM

To: 'Ivan Malov' <ivan.malov@oktetlabs.ru><mailto:ivan.malov@oktetlabs.ru>; Olivier Matz <olivier.matz@6wind.com><mailto:olivier.matz@6wind.com>

Cc: dev@dpdk.org<mailto:dev@dpdk.org>; Kulasek, TomaszX <tomaszx.kulasek@intel.com><mailto:tomaszx.kulasek@intel.com>; stable@dpdk.org<mailto:stable@dpdk.org>

Subject: RE: [dpdk-dev] [PATCH] 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. As GCC version 7.4.0 is not susceptible

to the said false positive assessment, this patch

maintains a compiler barrier for earlier versions.



Fixes: 4fb7e803eb1a ("ethdev: add Tx preparation")

Cc: Tomasz Kulasek <tomaszx.kulasek@intel.com><mailto:tomaszx.kulasek@intel.com>

Cc: stable@dpdk.org<mailto:stable@dpdk.org>



Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru><mailto:ivan.malov@oktetlabs.ru>

---

 lib/librte_net/rte_net.h | 16 ++++++++++++++--

 1 file changed, 14 insertions(+), 2 deletions(-)



diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h

index 7088584..fb09431 100644

--- a/lib/librte_net/rte_net.h

+++ b/lib/librte_net/rte_net.h

@@ -151,7 +151,19 @@ uint32_t rte_net_get_ptype(const struct rte_mbuf *m,

                   ipv4_hdr->hdr_checksum = 0;

    }





As I remember, saw something similar before...

Probably the eaiser way to overcome it, is just to always initialize ipv4_hdr above,

something like:



+ipv4_hdr = NULL;

if (ol_flags & PKT_TX_IPV4) {

                ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct rte_ipv4_hdr *,

                                inner_l3_offset);



                if (ol_flags & PKT_TX_IP_CKSUM)

                        ipv4_hdr->hdr_checksum = 0;

 }



As another possible option  always initialisze both, and then use either one or another,

depending on flags:



ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct rte_ipv4_hdr *, inner_l3_offset);

ipv6_hdr = rte_pktmbuf_mtod_offset(m, struct rte_ipv6_hdr *, inner_l3_offset);

....

We have discussed the solution locally and rejected it since it just kills
the compiler help. It makes it possible to use invalid header and
compiler will not save you.

Suggested solution looks ugly, but at least it does not kill help from
compiler.

[KA] Well for me ugliness of the fix outweigh the hypothetical risk of using wrong pointer.
After all that’s why we have code reviews and UT to prevent such things to happen.
Another alternative to reorder the code a bit:

If (if (ol_flags & PKT_TX_IPV4) {
           if (ol_flags & PKT_TX_IP_CKSUM)
                        ipv4_hdr->hdr_checksum = 0;
             if (UDP) {…} else if (TCP) {…}
} else { /*IPv6*/
             If (UDP) {…} else if (TCP) {…}
}

-   if ((ol_flags & PKT_TX_UDP_CKSUM) == PKT_TX_UDP_CKSUM) {

+#ifdef GCC_VERSION

+#if GCC_VERSION < 70400

+   /*

+    * Earlier versions of GCC suspect access to possibly

+    * uninitialised ipv4_hdr in the code below, although

+    * the said variable is properly initialised above.

+    * Use compiler barrier to cope with the problem.

+    */

+   rte_compiler_barrier();

+#endif

+#endif

+

+   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);

@@ -167,7 +179,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 */

--

1.8.3.1
diff mbox series

Patch

diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
index 7088584..fb09431 100644
--- a/lib/librte_net/rte_net.h
+++ b/lib/librte_net/rte_net.h
@@ -151,7 +151,19 @@  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) {
+#ifdef GCC_VERSION
+#if GCC_VERSION < 70400
+	/*
+	 * Earlier versions of GCC suspect access to possibly
+	 * uninitialised ipv4_hdr in the code below, although
+	 * the said variable is properly initialised above.
+	 * Use compiler barrier to cope with the problem.
+	 */
+	rte_compiler_barrier();
+#endif
+#endif
+
+	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);
@@ -167,7 +179,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 */