diff mbox series

[v3,1/3] hash: fix position bug in 'free key with position'

Message ID 20190509133924.7153-2-dharmik.thakkar@arm.com (mailing list archive)
State Superseded, archived
Headers
Series hash: fix bugs in 'free key with position' |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Dharmik Thakkar May 9, 2019, 1:39 p.m. UTC 
  Currently, in rte_hash_free_key_with_position(), the position returned
to the ring of free_slots leads to an unexpected conflict with a key
already in use.

This patch fixes incorrect position returned to the ring of free_slots.

Bugzilla ID: 261
Fixes: 9d033dac7d7c ("hash: support no free on delete")
Cc: honnappa.nagarahalli@arm.com
Cc: stable@dpdk.org

Reported-by: Linfan <zhongdahulinfan@163.com>
Suggested-by: Linfan <zhongdahulinfan@163.com>
Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
---
 lib/librte_hash/rte_cuckoo_hash.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Wang, Yipeng1 May 9, 2019, 3:48 p.m. UTC | #1 
> -----Original Message-----
> From: Dharmik Thakkar [mailto:dharmik.thakkar@arm.com]
> Sent: Thursday, May 9, 2019 6:39 AM
> To: Wang, Yipeng1 <yipeng1.wang@intel.com>; Gobriel, Sameh
> <sameh.gobriel@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>;
> De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Cc: dev@dpdk.org; honnappa.nagarahalli@arm.com;
> zhongdahulinfan@163.com; Dharmik Thakkar <dharmik.thakkar@arm.com>;
> stable@dpdk.org
> Subject: [PATCH v3 1/3] hash: fix position bug in 'free key with position'
> 
> Currently, in rte_hash_free_key_with_position(), the position returned to the
> ring of free_slots leads to an unexpected conflict with a key already in use.
> 
> This patch fixes incorrect position returned to the ring of free_slots.
> 
> Bugzilla ID: 261
> Fixes: 9d033dac7d7c ("hash: support no free on delete")
> Cc: honnappa.nagarahalli@arm.com
> Cc: stable@dpdk.org
> 
> Reported-by: Linfan <zhongdahulinfan@163.com>
> Suggested-by: Linfan <zhongdahulinfan@163.com>
> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
> ---
>  lib/librte_hash/rte_cuckoo_hash.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_hash/rte_cuckoo_hash.c
> b/lib/librte_hash/rte_cuckoo_hash.c
> index 261267b7fd3d..8646ca52e60b 100644
> --- a/lib/librte_hash/rte_cuckoo_hash.c
> +++ b/lib/librte_hash/rte_cuckoo_hash.c
> @@ -1587,14 +1587,17 @@ int __rte_experimental
> rte_hash_free_key_with_position(const struct rte_hash *h,
>  				const int32_t position)
>  {
> -	RETURN_IF_TRUE(((h == NULL) || (position == EMPTY_SLOT)), -EINVAL);
> +	/*  Key index where key is stored, adding the first dummy index*/
> +	uint32_t key_idx = position + 1;
> +
> +	RETURN_IF_TRUE(((h == NULL) || (key_idx == EMPTY_SLOT)), -EINVAL);
> 
>  	unsigned int lcore_id, n_slots;
>  	struct lcore_cache *cached_free_slots;
>  	const int32_t total_entries = h->num_buckets *
> RTE_HASH_BUCKET_ENTRIES;
> 
>  	/* Out of bounds */
> -	if (position >= total_entries)
> +	if (key_idx >= total_entries)
[Wang, Yipeng] 
Compiling fail after this commit.
/rte_cuckoo_hash.c:1600:14: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
  if (key_idx >= total_entries)
please fix.

Please run the test-build and test-meson-build scripts. I currently have some issue with my meson setup on my new system so I will
rely on other reviewers or you guys to pass the meson test.

Logically these two bug fixes are valid and should be upstreamed otherwise this API breaks. But this is the experimental tag for :)

Thanks
Yipeng
Dharmik Thakkar May 9, 2019, 5:25 p.m. UTC | #2 
> On May 9, 2019, at 10:48 AM, Wang, Yipeng1 <yipeng1.wang@intel.com> wrote:
> 
>> -----Original Message-----
>> From: Dharmik Thakkar [mailto:dharmik.thakkar@arm.com]
>> Sent: Thursday, May 9, 2019 6:39 AM
>> To: Wang, Yipeng1 <yipeng1.wang@intel.com>; Gobriel, Sameh
>> <sameh.gobriel@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>;
>> De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
>> Cc: dev@dpdk.org; honnappa.nagarahalli@arm.com;
>> zhongdahulinfan@163.com; Dharmik Thakkar <dharmik.thakkar@arm.com>;
>> stable@dpdk.org
>> Subject: [PATCH v3 1/3] hash: fix position bug in 'free key with position'
>> 
>> Currently, in rte_hash_free_key_with_position(), the position returned to the
>> ring of free_slots leads to an unexpected conflict with a key already in use.
>> 
>> This patch fixes incorrect position returned to the ring of free_slots.
>> 
>> Bugzilla ID: 261
>> Fixes: 9d033dac7d7c ("hash: support no free on delete")
>> Cc: honnappa.nagarahalli@arm.com
>> Cc: stable@dpdk.org
>> 
>> Reported-by: Linfan <zhongdahulinfan@163.com>
>> Suggested-by: Linfan <zhongdahulinfan@163.com>
>> Signed-off-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
>> ---
>> lib/librte_hash/rte_cuckoo_hash.c | 11 +++++++----
>> 1 file changed, 7 insertions(+), 4 deletions(-)
>> 
>> diff --git a/lib/librte_hash/rte_cuckoo_hash.c
>> b/lib/librte_hash/rte_cuckoo_hash.c
>> index 261267b7fd3d..8646ca52e60b 100644
>> --- a/lib/librte_hash/rte_cuckoo_hash.c
>> +++ b/lib/librte_hash/rte_cuckoo_hash.c
>> @@ -1587,14 +1587,17 @@ int __rte_experimental
>> rte_hash_free_key_with_position(const struct rte_hash *h,
>> 				const int32_t position)
>> {
>> -	RETURN_IF_TRUE(((h == NULL) || (position == EMPTY_SLOT)), -EINVAL);
>> +	/*  Key index where key is stored, adding the first dummy index*/
>> +	uint32_t key_idx = position + 1;
>> +
>> +	RETURN_IF_TRUE(((h == NULL) || (key_idx == EMPTY_SLOT)), -EINVAL);
>> 
>> 	unsigned int lcore_id, n_slots;
>> 	struct lcore_cache *cached_free_slots;
>> 	const int32_t total_entries = h->num_buckets *
>> RTE_HASH_BUCKET_ENTRIES;
>> 
>> 	/* Out of bounds */
>> -	if (position >= total_entries)
>> +	if (key_idx >= total_entries)
> [Wang, Yipeng] 
> Compiling fail after this commit.
> /rte_cuckoo_hash.c:1600:14: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
>  if (key_idx >= total_entries)
> please fix.
Thank you for reporting! I have fixed the issue and updated the patch.
> 
> Please run the test-build and test-meson-build scripts. I currently have some issue with my meson setup on my new system so I will
> rely on other reviewers or you guys to pass the meson test.
I ran the scripts on my setup. Thanks!
> 
> Logically these two bug fixes are valid and should be upstreamed otherwise this API breaks. But this is the experimental tag for :)
> 
> Thanks
> Yipeng
diff mbox series

Patch

diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
index 261267b7fd3d..8646ca52e60b 100644
--- a/lib/librte_hash/rte_cuckoo_hash.c
+++ b/lib/librte_hash/rte_cuckoo_hash.c
@@ -1587,14 +1587,17 @@  int __rte_experimental
 rte_hash_free_key_with_position(const struct rte_hash *h,
 				const int32_t position)
 {
-	RETURN_IF_TRUE(((h == NULL) || (position == EMPTY_SLOT)), -EINVAL);
+	/*  Key index where key is stored, adding the first dummy index*/
+	uint32_t key_idx = position + 1;
+
+	RETURN_IF_TRUE(((h == NULL) || (key_idx == EMPTY_SLOT)), -EINVAL);
 
 	unsigned int lcore_id, n_slots;
 	struct lcore_cache *cached_free_slots;
 	const int32_t total_entries = h->num_buckets * RTE_HASH_BUCKET_ENTRIES;
 
 	/* Out of bounds */
-	if (position >= total_entries)
+	if (key_idx >= total_entries)
 		return -EINVAL;
 	if (h->ext_table_support && h->readwrite_concur_lf_support) {
 		uint32_t index = h->ext_bkt_to_free[position];
@@ -1619,11 +1622,11 @@  rte_hash_free_key_with_position(const struct rte_hash *h,
 		}
 		/* Put index of new free slot in cache. */
 		cached_free_slots->objs[cached_free_slots->len] =
-					(void *)((uintptr_t)position);
+					(void *)((uintptr_t)key_idx);
 		cached_free_slots->len++;
 	} else {
 		rte_ring_sp_enqueue(h->free_slots,
-				(void *)((uintptr_t)position));
+				(void *)((uintptr_t)key_idx));
 	}
 
 	return 0;