Re: [dpdk-dev] [PATCH v5 1/4] hash: fix race condition in iterate

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

> -----Original Message-----
> From: Yipeng Wang <yipeng1.wang@intel.com>
> Sent: Monday, October 1, 2018 1:35 PM
> To: bruce.richardson@intel.com
> Cc: konstantin.ananyev@intel.com; dev@dpdk.org;
> yipeng1.wang@intel.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; sameh.gobriel@intel.com
> Subject: [PATCH v5 1/4] hash: fix race condition in iterate
> 
> In rte_hash_iterate, the reader lock did not protect the while loop which
> checks empty entry. This created a race condition that the entry may
> become empty when enters the lock, then a wrong key data value would be
> read out.
> 
> This commit reads out the position in the while condition, which makes sure
> that the position will not be changed to empty before entering the lock.
> 
> Fixes: f2e3001b53ec ("hash: support read/write concurrency")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yipeng Wang <yipeng1.wang@intel.com>
> Reported-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> ---
>  lib/librte_hash/rte_cuckoo_hash.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_hash/rte_cuckoo_hash.c
> b/lib/librte_hash/rte_cuckoo_hash.c
> index f7b86c8..da8ddf4 100644
> --- a/lib/librte_hash/rte_cuckoo_hash.c
> +++ b/lib/librte_hash/rte_cuckoo_hash.c
> @@ -1318,7 +1318,7 @@ rte_hash_iterate(const struct rte_hash *h, const
> void **key, void **data, uint32
>  	idx = *next % RTE_HASH_BUCKET_ENTRIES;
> 
>  	/* If current position is empty, go to the next one */
> -	while (h->buckets[bucket_idx].key_idx[idx] == EMPTY_SLOT) {
> +	while ((position = h->buckets[bucket_idx].key_idx[idx]) ==
> EMPTY_SLOT)
> +{
>  		(*next)++;
>  		/* End of table */
>  		if (*next == total_entries)
> @@ -1326,9 +1326,8 @@ rte_hash_iterate(const struct rte_hash *h, const
> void **key, void **data, uint32
>  		bucket_idx = *next / RTE_HASH_BUCKET_ENTRIES;
>  		idx = *next % RTE_HASH_BUCKET_ENTRIES;
>  	}
> +
>  	__hash_rw_reader_lock(h);
> -	/* Get position of entry in key table */
> -	position = h->buckets[bucket_idx].key_idx[idx];
>  	next_key = (struct rte_hash_key *) ((char *)h->key_store +
>  				position * h->key_entry_size);
>  	/* Return key and data */
> --
> 2.7.4
This looks good. I can rework my patch too. I will leave the decision to you.

Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>