* Re: [dpdk-dev] [PATCH] net: fix the way how L4 checksum choice is tested
2019-05-29 17:33 [dpdk-dev] [PATCH] net: fix the way how L4 checksum choice is tested Ivan Malov
@ 2019-06-24 11:52 ` Ananyev, Konstantin
2019-06-24 12:01 ` Ananyev, Konstantin
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: Ananyev, Konstantin @ 2019-06-24 11:52 UTC (permalink / raw)
To: Ivan Malov, Olivier Matz; +Cc: dev, Kulasek, TomaszX, stable
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [dpdk-dev] [PATCH] net: fix the way how L4 checksum choice is tested
2019-05-29 17:33 [dpdk-dev] [PATCH] net: fix the way how L4 checksum choice is tested Ivan Malov
2019-06-24 11:52 ` Ananyev, Konstantin
@ 2019-06-24 12:01 ` Ananyev, Konstantin
2019-06-24 12:16 ` Andrew Rybchenko
2019-06-27 21:52 ` [dpdk-dev] [PATCH v2] " Ivan Malov
2019-06-28 3:13 ` [dpdk-dev] [PATCH v3] " Ivan Malov
3 siblings, 1 reply; 11+ messages in thread
From: Ananyev, Konstantin @ 2019-06-24 12:01 UTC (permalink / raw)
To: Ivan Malov, Olivier Matz; +Cc: dev, Kulasek, TomaszX, stable
> -----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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [dpdk-dev] [PATCH] net: fix the way how L4 checksum choice is tested
2019-06-24 12:01 ` Ananyev, Konstantin
@ 2019-06-24 12:16 ` Andrew Rybchenko
2019-06-27 13:26 ` Ananyev, Konstantin
0 siblings, 1 reply; 11+ messages in thread
From: Andrew Rybchenko @ 2019-06-24 12:16 UTC (permalink / raw)
To: Ananyev, Konstantin, Ivan Malov, Olivier Matz
Cc: dev, Kulasek, TomaszX, stable
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [dpdk-dev] [PATCH] net: fix the way how L4 checksum choice is tested
2019-06-24 12:16 ` Andrew Rybchenko
@ 2019-06-27 13:26 ` Ananyev, Konstantin
0 siblings, 0 replies; 11+ messages in thread
From: Ananyev, Konstantin @ 2019-06-27 13:26 UTC (permalink / raw)
To: Andrew Rybchenko, Ivan Malov, Olivier Matz; +Cc: dev, Kulasek, TomaszX, stable
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [dpdk-dev] [PATCH v2] net: fix the way how L4 checksum choice is tested
2019-05-29 17:33 [dpdk-dev] [PATCH] net: fix the way how L4 checksum choice is tested Ivan Malov
2019-06-24 11:52 ` Ananyev, Konstantin
2019-06-24 12:01 ` Ananyev, Konstantin
@ 2019-06-27 21:52 ` Ivan Malov
2019-06-28 0:10 ` Stephen Hemminger
2019-06-28 3:13 ` [dpdk-dev] [PATCH v3] " Ivan Malov
3 siblings, 1 reply; 11+ messages in thread
From: Ivan Malov @ 2019-06-27 21:52 UTC (permalink / raw)
To: Olivier Matz
Cc: dev, Ananyev, Konstantin, Andrew Rybchenko, Tomasz Kulasek, stable
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 | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
index 7be69f8..6eab230 100644
--- a/lib/librte_net/rte_net.h
+++ b/lib/librte_net/rte_net.h
@@ -112,7 +112,13 @@ 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 beforehand to mute false positive
+ * compiler warnings about this variable being possibly used
+ * uninitialised in L4 parsing branches below albeit it is clearly
+ * initialised in IPv4 parsing branch prior L4-related code.
+ */
+ 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 +156,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 +172,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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [dpdk-dev] [PATCH v2] net: fix the way how L4 checksum choice is tested
2019-06-27 21:52 ` [dpdk-dev] [PATCH v2] " Ivan Malov
@ 2019-06-28 0:10 ` Stephen Hemminger
0 siblings, 0 replies; 11+ messages in thread
From: Stephen Hemminger @ 2019-06-28 0:10 UTC (permalink / raw)
To: Ivan Malov
Cc: Olivier Matz, dev, Ananyev, Konstantin, Andrew Rybchenko,
Tomasz Kulasek, stable
On Fri, 28 Jun 2019 00:52:02 +0300
Ivan Malov <ivan.malov@oktetlabs.ru> wrote:
> + /*
> + * Initialise ipv4_hdr beforehand to mute false positive
> + * compiler warnings about this variable being possibly used
> + * uninitialised in L4 parsing branches below albeit it is clearly
> + * initialised in IPv4 parsing branch prior L4-related code.
> + */
Really, keep the verbosity in the commit log not the code.
One line commit is enough.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [dpdk-dev] [PATCH v3] net: fix the way how L4 checksum choice is tested
2019-05-29 17:33 [dpdk-dev] [PATCH] net: fix the way how L4 checksum choice is tested Ivan Malov
` (2 preceding siblings ...)
2019-06-27 21:52 ` [dpdk-dev] [PATCH v2] " Ivan Malov
@ 2019-06-28 3:13 ` Ivan Malov
2019-06-28 4:26 ` Stephen Hemminger
2019-06-28 10:47 ` Ananyev, Konstantin
3 siblings, 2 replies; 11+ messages in thread
From: Ivan Malov @ 2019-06-28 3:13 UTC (permalink / raw)
To: Olivier Matz
Cc: dev, Ananyev, Konstantin, Andrew Rybchenko, Stephen Hemminger,
Tomasz Kulasek, stable
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 */
--
1.8.3.1
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [dpdk-dev] [PATCH v3] net: fix the way how L4 checksum choice is tested
2019-06-28 3:13 ` [dpdk-dev] [PATCH v3] " Ivan Malov
@ 2019-06-28 4:26 ` Stephen Hemminger
2019-06-28 10:47 ` Ananyev, Konstantin
1 sibling, 0 replies; 11+ messages in thread
From: Stephen Hemminger @ 2019-06-28 4:26 UTC (permalink / raw)
To: Ivan Malov
Cc: Olivier Matz, dev, Ananyev, Konstantin, Andrew Rybchenko,
Tomasz Kulasek, stable
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>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [dpdk-dev] [PATCH v3] net: fix the way how L4 checksum choice is tested
2019-06-28 3:13 ` [dpdk-dev] [PATCH v3] " Ivan Malov
2019-06-28 4:26 ` Stephen Hemminger
@ 2019-06-28 10:47 ` Ananyev, Konstantin
2019-06-28 16:24 ` Ferruh Yigit
1 sibling, 1 reply; 11+ messages in thread
From: Ananyev, Konstantin @ 2019-06-28 10:47 UTC (permalink / raw)
To: Ivan Malov, Olivier Matz
Cc: dev, Andrew Rybchenko, Stephen Hemminger, Kulasek, TomaszX, stable
> -----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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [dpdk-dev] [PATCH v3] net: fix the way how L4 checksum choice is tested
2019-06-28 10:47 ` Ananyev, Konstantin
@ 2019-06-28 16:24 ` Ferruh Yigit
0 siblings, 0 replies; 11+ messages in thread
From: Ferruh Yigit @ 2019-06-28 16:24 UTC (permalink / raw)
To: Ananyev, Konstantin, Ivan Malov, Olivier Matz
Cc: dev, Andrew Rybchenko, Stephen Hemminger, Kulasek, TomaszX, stable
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.
^ permalink raw reply [flat|nested] 11+ messages in thread