[ovs-dev] [PATCH] dp-packet: Fix use of uninitialised value at emc_lookup.

William Tu u9012063 at gmail.com
Wed Apr 6 18:31:33 UTC 2016


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 at ovn.org>
wrote:

> 2016-04-06 10:09 GMT-07:00 Darrell Ball <dlu998 at gmail.com>:
> > On Wed, Apr 6, 2016 at 9:37 AM, William Tu <u9012063 at 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 at 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!
>



More information about the dev mailing list