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

[Daniele Di Proietto]

Thanks for the fix!

I've applied this to master and branch-2.5

2016-04-06 16:28 GMT-07:00 William Tu <u9012063 at gmail.com>:
> 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 1) assigning an uninitialized value to 'key->hash'
> at emc_processing(). Due to uninit rss_hash_valid, dp_packet_rss_valid() might
> return true and undefined hash value is returned, and 2) at emc_lookup, the
> 'current_entry->key.hash' could be uninitialized due to dp_packet_clone().
> The patch fixes the two and as a result, a couple of calls to
> dp_packet_rss_valid() become redundant and thus are removed.
>
> Call stacks:
> - 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>
> ---
> v1->v2
>     - use dp_packet_rss_invalidate() instead of direct assignment
>     - fix dp_packet_clone_with_headroom()
>     - remove redundant dp_packet_rss_invalidate()
> ---
>  lib/dp-packet.c    | 16 +++++++++++++++-
>  lib/netdev-bsd.c   |  1 -
>  lib/netdev-dummy.c |  1 -
>  lib/netdev-linux.c |  1 -
>  4 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> index aec7fe7..0c85d50 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,7 @@ 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);
> +    dp_packet_rss_invalidate(b);
>  }
>
>  static void
> @@ -167,6 +168,19 @@ dp_packet_clone_with_headroom(const struct dp_packet *buffer, size_t headroom)
>      new_buffer->l3_ofs = buffer->l3_ofs;
>      new_buffer->l4_ofs = buffer->l4_ofs;
>      new_buffer->md = buffer->md;
> +#ifdef DPDK_NETDEV
> +    new_buffer->mbuf.ol_flags = buffer->mbuf.ol_flags;
> +#else
> +    new_buffer->rss_hash_valid = buffer->rss_hash_valid;
> +#endif
> +
> +    if (dp_packet_rss_valid(new_buffer)) {
> +#ifdef DPDK_NETDEV
> +        new_buffer->mbuf.hash.rss = buffer->mbuf.hash.rss;
> +#else
> +        new_buffer->rss_hash = buffer->rss_hash;
> +#endif
> +    }
>
>      return new_buffer;
>  }
> diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
> index 75bd5a3..49c05f4 100644
> --- a/lib/netdev-bsd.c
> +++ b/lib/netdev-bsd.c
> @@ -641,7 +641,6 @@ netdev_bsd_rxq_recv(struct netdev_rxq *rxq_, struct dp_packet **packets,
>          dp_packet_delete(packet);
>      } else {
>          dp_packet_pad(packet);
> -        dp_packet_rss_invalidate(packet);
>          packets[0] = packet;
>          *c = 1;
>      }
> diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
> index edc86fa..a1013ff 100644
> --- a/lib/netdev-dummy.c
> +++ b/lib/netdev-dummy.c
> @@ -905,7 +905,6 @@ netdev_dummy_rxq_recv(struct netdev_rxq *rxq_, struct dp_packet **arr,
>      ovs_mutex_unlock(&netdev->mutex);
>
>      dp_packet_pad(packet);
> -    dp_packet_rss_invalidate(packet);
>
>      arr[0] = packet;
>      *c = 1;
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 3f5b608..a7d7ac7 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -1116,7 +1116,6 @@ netdev_linux_rxq_recv(struct netdev_rxq *rxq_, struct dp_packet **packets,
>          dp_packet_delete(buffer);
>      } else {
>          dp_packet_pad(buffer);
> -        dp_packet_rss_invalidate(buffer);
>          packets[0] = buffer;
>          *c = 1;
>      }
> --
> 2.5.0
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev