| Message ID | 1459960625-9397-1-git-send-email-u9012063@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Apr 6, 2016 at 9:37 AM, William Tu <u9012063@gmail.com> wrote: > Valgrind reports "Conditional jump or move depends on uninitialised value" > and "Use of uninitialised value" at case 2016 ovn -- 3 HVs, 1 LS, 3 > lports/HV. It is caused by reading uninitialized 'key->hash' at > emc_lookup() > and 'rss_hash_valid' from dp_packet_rss_valid(). At emc_processing(), > the value of key->hash is initilized by dpif_netdev_packet_get_rss_hash(), > which returns an uninitialized hash value. Call stacks below: > > - Connditional jump or move depends on uninitialised value(s) > dpif_netdev_packet_get_rss_hash (dpif-netdev.c:3334) > emc_processing (dpif-netdev.c:3455) > dp_netdev_input__ (dpif-netdev.c:3639) > and, > - Use of uninitialised value of size 8 > emc_lookup (dpif-netdev.c:1785) > emc_processing (dpif-netdev.c:3457) > dp_netdev_input__ (dpif-netdev.c:3639) > > Signed-off-by: William Tu <u9012063@gmail.com> > --- > lib/dp-packet.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/lib/dp-packet.c b/lib/dp-packet.c > index aec7fe7..87ed329 100644 > --- a/lib/dp-packet.c > +++ b/lib/dp-packet.c > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc. > + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2016 Nicira, Inc. > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -29,6 +29,13 @@ dp_packet_init__(struct dp_packet *b, size_t allocated, > enum dp_packet_source so > b->source = source; > dp_packet_reset_offsets(b); > pkt_metadata_init(&b->md, 0); > +#ifdef DPDK_NETDEV > + b->mbuf.ol_flags &= ~PKT_RX_RSS_HASH; > + b->mbuf.hash.rss = 0; > +#else > + b->rss_hash_valid = false; > + b->rss_hash = 0; > +#endif > Just a general comment, not a review: Do you need to set the hash value to zero as well as set the "hash_valid" flag to false; should not setting the "hash_valid" flag to false be enough to handle a <potential> initialization issue ? I think there is already an API for setting "hash_valid" to false here static inline void dp_packet_rss_invalidate(struct dp_packet *p) { #ifdef DPDK_NETDEV p->mbuf.ol_flags &= ~PKT_RX_RSS_HASH; #else p->rss_hash_valid = false; #endif } > } > > static void > -- > 2.5.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev >
2016-04-06 10:09 GMT-07:00 Darrell Ball <dlu998@gmail.com>: > On Wed, Apr 6, 2016 at 9:37 AM, William Tu <u9012063@gmail.com> wrote: > >> Valgrind reports "Conditional jump or move depends on uninitialised value" >> and "Use of uninitialised value" at case 2016 ovn -- 3 HVs, 1 LS, 3 >> lports/HV. It is caused by reading uninitialized 'key->hash' at >> emc_lookup() >> and 'rss_hash_valid' from dp_packet_rss_valid(). At emc_processing(), >> the value of key->hash is initilized by dpif_netdev_packet_get_rss_hash(), >> which returns an uninitialized hash value. Call stacks below: >> >> - Connditional jump or move depends on uninitialised value(s) >> dpif_netdev_packet_get_rss_hash (dpif-netdev.c:3334) >> emc_processing (dpif-netdev.c:3455) >> dp_netdev_input__ (dpif-netdev.c:3639) >> and, >> - Use of uninitialised value of size 8 >> emc_lookup (dpif-netdev.c:1785) >> emc_processing (dpif-netdev.c:3457) >> dp_netdev_input__ (dpif-netdev.c:3639) >> >> Signed-off-by: William Tu <u9012063@gmail.com> >> --- >> lib/dp-packet.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/lib/dp-packet.c b/lib/dp-packet.c >> index aec7fe7..87ed329 100644 >> --- a/lib/dp-packet.c >> +++ b/lib/dp-packet.c >> @@ -1,5 +1,5 @@ >> /* >> - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc. >> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2016 Nicira, Inc. >> * >> * Licensed under the Apache License, Version 2.0 (the "License"); >> * you may not use this file except in compliance with the License. >> @@ -29,6 +29,13 @@ dp_packet_init__(struct dp_packet *b, size_t allocated, >> enum dp_packet_source so >> b->source = source; >> dp_packet_reset_offsets(b); >> pkt_metadata_init(&b->md, 0); >> +#ifdef DPDK_NETDEV >> + b->mbuf.ol_flags &= ~PKT_RX_RSS_HASH; >> + b->mbuf.hash.rss = 0; >> +#else >> + b->rss_hash_valid = false; >> + b->rss_hash = 0; >> +#endif >> > > > Just a general comment, not a review: > > Do you need to set the hash value to zero as well as set > the "hash_valid" flag to false; should not setting the "hash_valid" > flag to false be enough to handle a <potential> initialization issue ? > > I think there is already an API for setting "hash_valid" > to false here > > static inline void > dp_packet_rss_invalidate(struct dp_packet *p) > { > #ifdef DPDK_NETDEV > p->mbuf.ol_flags &= ~PKT_RX_RSS_HASH; > #else > p->rss_hash_valid = false; > #endif > } > > I agree with Darrell, I think it's better to use dp_packet_rss_invalidate(). Also, if we include dp_packet_rss_invalidate() in dp_packet_init__(), we will have redundant calls to dp_packet_rss_invalidate() in netdev-{bsd,dummy,linux}.c. Would you mind removing those? There's another one in netdev-dpdk.c, but that will be requires anyway. Would you mind sending a v2 with the suggested changes? Thanks!
Hi Darrell and Daniele, Thanks for the comments! I will use dp_packet_rss_invalidate() instead and send v2 patch. Regards, William On Wed, Apr 6, 2016 at 11:26 AM, Daniele Di Proietto <diproiettod@ovn.org> wrote: > 2016-04-06 10:09 GMT-07:00 Darrell Ball <dlu998@gmail.com>: > > On Wed, Apr 6, 2016 at 9:37 AM, William Tu <u9012063@gmail.com> wrote: > > > >> Valgrind reports "Conditional jump or move depends on uninitialised > value" > >> and "Use of uninitialised value" at case 2016 ovn -- 3 HVs, 1 LS, 3 > >> lports/HV. It is caused by reading uninitialized 'key->hash' at > >> emc_lookup() > >> and 'rss_hash_valid' from dp_packet_rss_valid(). At emc_processing(), > >> the value of key->hash is initilized by > dpif_netdev_packet_get_rss_hash(), > >> which returns an uninitialized hash value. Call stacks below: > >> > >> - Connditional jump or move depends on uninitialised value(s) > >> dpif_netdev_packet_get_rss_hash (dpif-netdev.c:3334) > >> emc_processing (dpif-netdev.c:3455) > >> dp_netdev_input__ (dpif-netdev.c:3639) > >> and, > >> - Use of uninitialised value of size 8 > >> emc_lookup (dpif-netdev.c:1785) > >> emc_processing (dpif-netdev.c:3457) > >> dp_netdev_input__ (dpif-netdev.c:3639) > >> > >> Signed-off-by: William Tu <u9012063@gmail.com> > >> --- > >> lib/dp-packet.c | 9 ++++++++- > >> 1 file changed, 8 insertions(+), 1 deletion(-) > >> > >> diff --git a/lib/dp-packet.c b/lib/dp-packet.c > >> index aec7fe7..87ed329 100644 > >> --- a/lib/dp-packet.c > >> +++ b/lib/dp-packet.c > >> @@ -1,5 +1,5 @@ > >> /* > >> - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc. > >> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2016 Nicira, Inc. > >> * > >> * Licensed under the Apache License, Version 2.0 (the "License"); > >> * you may not use this file except in compliance with the License. > >> @@ -29,6 +29,13 @@ dp_packet_init__(struct dp_packet *b, size_t > allocated, > >> enum dp_packet_source so > >> b->source = source; > >> dp_packet_reset_offsets(b); > >> pkt_metadata_init(&b->md, 0); > >> +#ifdef DPDK_NETDEV > >> + b->mbuf.ol_flags &= ~PKT_RX_RSS_HASH; > >> + b->mbuf.hash.rss = 0; > >> +#else > >> + b->rss_hash_valid = false; > >> + b->rss_hash = 0; > >> +#endif > >> > > > > > > Just a general comment, not a review: > > > > Do you need to set the hash value to zero as well as set > > the "hash_valid" flag to false; should not setting the "hash_valid" > > flag to false be enough to handle a <potential> initialization issue ? > > > > I think there is already an API for setting "hash_valid" > > to false here > > > > static inline void > > dp_packet_rss_invalidate(struct dp_packet *p) > > { > > #ifdef DPDK_NETDEV > > p->mbuf.ol_flags &= ~PKT_RX_RSS_HASH; > > #else > > p->rss_hash_valid = false; > > #endif > > } > > > > > > I agree with Darrell, I think it's better to use > dp_packet_rss_invalidate(). > > Also, if we include dp_packet_rss_invalidate() in dp_packet_init__(), > we will have redundant calls to dp_packet_rss_invalidate() in > netdev-{bsd,dummy,linux}.c. Would you mind removing those? There's > another one in netdev-dpdk.c, but that will be requires anyway. > > Would you mind sending a v2 with the suggested changes? > > Thanks! >
diff --git a/lib/dp-packet.c b/lib/dp-packet.c index aec7fe7..87ed329 100644 --- a/lib/dp-packet.c +++ b/lib/dp-packet.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc. + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2016 Nicira, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -29,6 +29,13 @@ dp_packet_init__(struct dp_packet *b, size_t allocated, enum dp_packet_source so b->source = source; dp_packet_reset_offsets(b); pkt_metadata_init(&b->md, 0); +#ifdef DPDK_NETDEV + b->mbuf.ol_flags &= ~PKT_RX_RSS_HASH; + b->mbuf.hash.rss = 0; +#else + b->rss_hash_valid = false; + b->rss_hash = 0; +#endif } static void
Valgrind reports "Conditional jump or move depends on uninitialised value" and "Use of uninitialised value" at case 2016 ovn -- 3 HVs, 1 LS, 3 lports/HV. It is caused by reading uninitialized 'key->hash' at emc_lookup() and 'rss_hash_valid' from dp_packet_rss_valid(). At emc_processing(), the value of key->hash is initilized by dpif_netdev_packet_get_rss_hash(), which returns an uninitialized hash value. Call stacks below: - Connditional jump or move depends on uninitialised value(s) dpif_netdev_packet_get_rss_hash (dpif-netdev.c:3334) emc_processing (dpif-netdev.c:3455) dp_netdev_input__ (dpif-netdev.c:3639) and, - Use of uninitialised value of size 8 emc_lookup (dpif-netdev.c:1785) emc_processing (dpif-netdev.c:3457) dp_netdev_input__ (dpif-netdev.c:3639) Signed-off-by: William Tu <u9012063@gmail.com> --- lib/dp-packet.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)