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

[Darrell Ball]

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
}






>  }
>
>  static void
> --
> 2.5.0
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>