2016-04-06 10:09 GMT-07:00 Darrell Ball <dlu...@gmail.com>:
> On Wed, Apr 6, 2016 at 9:37 AM, William Tu <u9012...@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 <u9012...@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!
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
