* Re: [dpdk-dev] [PATCH] test/timer: fix memzone reserve failure check
2021-04-22 9:18 [dpdk-dev] [PATCH] test/timer: fix memzone reserve failure check Min Hu (Connor)
@ 2021-05-01 20:00 ` Carrillo, Erik G
2021-05-04 1:08 ` Min Hu (Connor)
2021-05-04 1:07 ` [dpdk-dev] [PATCH v2] " Min Hu (Connor)
2021-05-11 0:53 ` [dpdk-dev] [PATCH v3] " Min Hu (Connor)
2 siblings, 1 reply; 10+ messages in thread
From: Carrillo, Erik G @ 2021-05-01 20:00 UTC (permalink / raw)
To: Min Hu (Connor), dev; +Cc: Yigit, Ferruh, rsanford
> -----Original Message-----
> From: Min Hu (Connor) <humin29@huawei.com>
> Sent: Thursday, April 22, 2021 4:19 AM
> To: dev@dpdk.org
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; rsanford@akamai.com; Carrillo,
> Erik G <erik.g.carrillo@intel.com>
> Subject: [PATCH] test/timer: fix memzone reserve failure check
>
> Segmentation fault may occur without checking if memzone reserves
> succeed or not.
>
> This patch fixed it.
>
> Fixes: 50247fe03fe0 ("test/timer: exercise new APIs in secondary process")
> Cc: stable@dpdk.org
>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
> app/test/test_timer_secondary.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/app/test/test_timer_secondary.c
> b/app/test/test_timer_secondary.c index 1e8f1d4..281f5bd 100644
> --- a/app/test/test_timer_secondary.c
> +++ b/app/test/test_timer_secondary.c
> @@ -125,6 +125,11 @@ test_timer_secondary(void)
>
> mz = rte_memzone_reserve(TEST_INFO_MZ_NAME,
> sizeof(*test_info),
> SOCKET_ID_ANY, 0);
> + if (mz == NULL) {
> + printf("Failed to reserve memzone\n");
> + return TEST_SKIPPED;
> + }
> +
> test_info = mz->addr;
> TEST_ASSERT_NOT_NULL(test_info, "Couldn't allocate
> memory for "
> "test data");
I think the TEST_ASSERT_NOT_NULL check here should be the area we update -- instead of checking the test_info pointer, we should check "mz", and that will address the issue you have noted. We can then move the "test_info = mz->addr" assignment below the assert.
> @@ -171,6 +176,11 @@ test_timer_secondary(void)
> int i;
>
> mz = rte_memzone_lookup(TEST_INFO_MZ_NAME);
> + if (mz == NULL) {
> + printf("Failed to lookup memzone\n");
> + return TEST_SKIPPED;
> + }
> +
> test_info = mz->addr;
> TEST_ASSERT_NOT_NULL(test_info, "Couldn't lookup
> memzone for "
> "test info");
Same thing here -- we can update the TEST_ASSERT_NOT_NULL call here instead, and move it above the "test_info = mz->addr" assignment.
Thanks,
Erik
> --
> 2.7.4
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [dpdk-dev] [PATCH] test/timer: fix memzone reserve failure check
2021-05-01 20:00 ` Carrillo, Erik G
@ 2021-05-04 1:08 ` Min Hu (Connor)
0 siblings, 0 replies; 10+ messages in thread
From: Min Hu (Connor) @ 2021-05-04 1:08 UTC (permalink / raw)
To: Carrillo, Erik G, dev; +Cc: Yigit, Ferruh, rsanford
在 2021/5/2 4:00, Carrillo, Erik G 写道:
>> -----Original Message-----
>> From: Min Hu (Connor) <humin29@huawei.com>
>> Sent: Thursday, April 22, 2021 4:19 AM
>> To: dev@dpdk.org
>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; rsanford@akamai.com; Carrillo,
>> Erik G <erik.g.carrillo@intel.com>
>> Subject: [PATCH] test/timer: fix memzone reserve failure check
>>
>> Segmentation fault may occur without checking if memzone reserves
>> succeed or not.
>>
>> This patch fixed it.
>>
>> Fixes: 50247fe03fe0 ("test/timer: exercise new APIs in secondary process")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>> app/test/test_timer_secondary.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/app/test/test_timer_secondary.c
>> b/app/test/test_timer_secondary.c index 1e8f1d4..281f5bd 100644
>> --- a/app/test/test_timer_secondary.c
>> +++ b/app/test/test_timer_secondary.c
>> @@ -125,6 +125,11 @@ test_timer_secondary(void)
>>
>> mz = rte_memzone_reserve(TEST_INFO_MZ_NAME,
>> sizeof(*test_info),
>> SOCKET_ID_ANY, 0);
>> + if (mz == NULL) {
>> + printf("Failed to reserve memzone\n");
>> + return TEST_SKIPPED;
>> + }
>> +
>> test_info = mz->addr;
>> TEST_ASSERT_NOT_NULL(test_info, "Couldn't allocate
>> memory for "
>> "test data");
>
> I think the TEST_ASSERT_NOT_NULL check here should be the area we update -- instead of checking the test_info pointer, we should check "mz", and that will address the issue you have noted. We can then move the "test_info = mz->addr" assignment below the assert.
>
Agreed, fixed in v2, thanks.
>> @@ -171,6 +176,11 @@ test_timer_secondary(void)
>> int i;
>>
>> mz = rte_memzone_lookup(TEST_INFO_MZ_NAME);
>> + if (mz == NULL) {
>> + printf("Failed to lookup memzone\n");
>> + return TEST_SKIPPED;
>> + }
>> +
>> test_info = mz->addr;
>> TEST_ASSERT_NOT_NULL(test_info, "Couldn't lookup
>> memzone for "
>> "test info");
>
> Same thing here -- we can update the TEST_ASSERT_NOT_NULL call here instead, and move it above the "test_info = mz->addr" assignment.
>
Fixed in v2, thanks.
> Thanks,
> Erik
>
>> --
>> 2.7.4
>
> .
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [dpdk-dev] [PATCH v2] test/timer: fix memzone reserve failure check
2021-04-22 9:18 [dpdk-dev] [PATCH] test/timer: fix memzone reserve failure check Min Hu (Connor)
2021-05-01 20:00 ` Carrillo, Erik G
@ 2021-05-04 1:07 ` Min Hu (Connor)
2021-05-04 1:18 ` Carrillo, Erik G
2021-05-10 14:13 ` Thomas Monjalon
2021-05-11 0:53 ` [dpdk-dev] [PATCH v3] " Min Hu (Connor)
2 siblings, 2 replies; 10+ messages in thread
From: Min Hu (Connor) @ 2021-05-04 1:07 UTC (permalink / raw)
To: dev; +Cc: ferruh.yigit, erik.g.carrillo, rsanford
Segmentation fault may occur without checking if memzone
reserves succeed or not.
This patch fixed it.
Fixes: 50247fe03fe0 ("test/timer: exercise new APIs in secondary process")
Cc: stable@dpdk.org
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
v2:
* use TEST_ASSERT_NOT_NULL check "mz" instead of checking
the test_info pointer.
---
app/test/test_timer_secondary.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/app/test/test_timer_secondary.c b/app/test/test_timer_secondary.c
index 1e8f1d4..16a9f18 100644
--- a/app/test/test_timer_secondary.c
+++ b/app/test/test_timer_secondary.c
@@ -125,9 +125,9 @@ test_timer_secondary(void)
mz = rte_memzone_reserve(TEST_INFO_MZ_NAME, sizeof(*test_info),
SOCKET_ID_ANY, 0);
- test_info = mz->addr;
- TEST_ASSERT_NOT_NULL(test_info, "Couldn't allocate memory for "
+ TEST_ASSERT_NOT_NULL(mz, "Couldn't allocate memory for "
"test data");
+ test_info = mz->addr;
test_info->tim_mempool = rte_mempool_create("test_timer_mp",
NUM_TIMERS, sizeof(struct rte_timer), 0, 0,
@@ -171,9 +171,9 @@ test_timer_secondary(void)
int i;
mz = rte_memzone_lookup(TEST_INFO_MZ_NAME);
- test_info = mz->addr;
- TEST_ASSERT_NOT_NULL(test_info, "Couldn't lookup memzone for "
+ TEST_ASSERT_NOT_NULL(mz, "Couldn't lookup memzone for "
"test info");
+ test_info = mz->addr;
for (i = 0; i < NUM_TIMERS; i++) {
rte_mempool_get(test_info->tim_mempool, (void **)&tim);
--
2.7.4
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [dpdk-dev] [PATCH v2] test/timer: fix memzone reserve failure check
2021-05-04 1:07 ` [dpdk-dev] [PATCH v2] " Min Hu (Connor)
@ 2021-05-04 1:18 ` Carrillo, Erik G
2021-05-10 14:13 ` Thomas Monjalon
1 sibling, 0 replies; 10+ messages in thread
From: Carrillo, Erik G @ 2021-05-04 1:18 UTC (permalink / raw)
To: Min Hu (Connor), dev; +Cc: Yigit, Ferruh, rsanford
> -----Original Message-----
> From: Min Hu (Connor) <humin29@huawei.com>
> Sent: Monday, May 3, 2021 8:08 PM
> To: dev@dpdk.org
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Carrillo, Erik G
> <erik.g.carrillo@intel.com>; rsanford@akamai.com
> Subject: [PATCH v2] test/timer: fix memzone reserve failure check
>
> Segmentation fault may occur without checking if memzone reserves
> succeed or not.
>
> This patch fixed it.
>
> Fixes: 50247fe03fe0 ("test/timer: exercise new APIs in secondary process")
> Cc: stable@dpdk.org
>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
> v2:
> * use TEST_ASSERT_NOT_NULL check "mz" instead of checking
> the test_info pointer.
> ---
Thanks,
Acked-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [dpdk-dev] [PATCH v2] test/timer: fix memzone reserve failure check
2021-05-04 1:07 ` [dpdk-dev] [PATCH v2] " Min Hu (Connor)
2021-05-04 1:18 ` Carrillo, Erik G
@ 2021-05-10 14:13 ` Thomas Monjalon
2021-05-11 0:53 ` Min Hu (Connor)
1 sibling, 1 reply; 10+ messages in thread
From: Thomas Monjalon @ 2021-05-10 14:13 UTC (permalink / raw)
To: Min Hu (Connor); +Cc: dev, ferruh.yigit, erik.g.carrillo, rsanford
04/05/2021 03:07, Min Hu (Connor):
> Segmentation fault may occur without checking if memzone
> reserves succeed or not.
The sentence is confusing. Please try to make it more logical.
Something like "It was potentially dereferencing a null pointer.
It is fixed by checking the pointer before dereferencing."
>
> This patch fixed it.
>
> Fixes: 50247fe03fe0 ("test/timer: exercise new APIs in secondary process")
> Cc: stable@dpdk.org
>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
[...]
> - test_info = mz->addr;
> - TEST_ASSERT_NOT_NULL(test_info, "Couldn't allocate memory for "
> + TEST_ASSERT_NOT_NULL(mz, "Couldn't allocate memory for "
> "test data");
Error messages should not be split. I makes search difficult.
Please fix it in this patch while touching this line.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [dpdk-dev] [PATCH v2] test/timer: fix memzone reserve failure check
2021-05-10 14:13 ` Thomas Monjalon
@ 2021-05-11 0:53 ` Min Hu (Connor)
0 siblings, 0 replies; 10+ messages in thread
From: Min Hu (Connor) @ 2021-05-11 0:53 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, ferruh.yigit, erik.g.carrillo, rsanford
在 2021/5/10 22:13, Thomas Monjalon 写道:
> 04/05/2021 03:07, Min Hu (Connor):
>> Segmentation fault may occur without checking if memzone
>> reserves succeed or not.
>
> The sentence is confusing. Please try to make it more logical.
> Something like "It was potentially dereferencing a null pointer.
> It is fixed by checking the pointer before dereferencing."
>
>>
>> This patch fixed it.
>>
>> Fixes: 50247fe03fe0 ("test/timer: exercise new APIs in secondary process")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> [...]
>> - test_info = mz->addr;
>> - TEST_ASSERT_NOT_NULL(test_info, "Couldn't allocate memory for "
>> + TEST_ASSERT_NOT_NULL(mz, "Couldn't allocate memory for "
>> "test data");
>
> Error messages should not be split. I makes search difficult.
> Please fix it in this patch while touching this line.
>
Fixed in v3, thanks.
>
>
> .
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [dpdk-dev] [PATCH v3] test/timer: fix memzone reserve failure check
2021-04-22 9:18 [dpdk-dev] [PATCH] test/timer: fix memzone reserve failure check Min Hu (Connor)
2021-05-01 20:00 ` Carrillo, Erik G
2021-05-04 1:07 ` [dpdk-dev] [PATCH v2] " Min Hu (Connor)
@ 2021-05-11 0:53 ` Min Hu (Connor)
2021-05-12 14:36 ` Thomas Monjalon
2 siblings, 1 reply; 10+ messages in thread
From: Min Hu (Connor) @ 2021-05-11 0:53 UTC (permalink / raw)
To: dev; +Cc: ferruh.yigit, thomas, erik.g.carrillo, rsanford
Segmentation fault may occur without checking if memzone
reserves succeed or not.
This patch fixed it.
Fixes: 50247fe03fe0 ("test/timer: exercise new APIs in secondary process")
Cc: stable@dpdk.org
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
v3:
* do not split error messages.
v2:
* use TEST_ASSERT_NOT_NULL check "mz" instead of checking
the test_info pointer.
---
app/test/test_timer_secondary.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/app/test/test_timer_secondary.c b/app/test/test_timer_secondary.c
index 1e8f1d4..decfaef 100644
--- a/app/test/test_timer_secondary.c
+++ b/app/test/test_timer_secondary.c
@@ -125,9 +125,8 @@ test_timer_secondary(void)
mz = rte_memzone_reserve(TEST_INFO_MZ_NAME, sizeof(*test_info),
SOCKET_ID_ANY, 0);
+ TEST_ASSERT_NOT_NULL(mz, "Couldn't allocate memory for test data");
test_info = mz->addr;
- TEST_ASSERT_NOT_NULL(test_info, "Couldn't allocate memory for "
- "test data");
test_info->tim_mempool = rte_mempool_create("test_timer_mp",
NUM_TIMERS, sizeof(struct rte_timer), 0, 0,
@@ -171,9 +170,8 @@ test_timer_secondary(void)
int i;
mz = rte_memzone_lookup(TEST_INFO_MZ_NAME);
+ TEST_ASSERT_NOT_NULL(mz, "Couldn't lookup memzone for test info");
test_info = mz->addr;
- TEST_ASSERT_NOT_NULL(test_info, "Couldn't lookup memzone for "
- "test info");
for (i = 0; i < NUM_TIMERS; i++) {
rte_mempool_get(test_info->tim_mempool, (void **)&tim);
--
2.7.4
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [dpdk-dev] [PATCH v3] test/timer: fix memzone reserve failure check
2021-05-11 0:53 ` [dpdk-dev] [PATCH v3] " Min Hu (Connor)
@ 2021-05-12 14:36 ` Thomas Monjalon
2021-05-13 0:19 ` Min Hu (Connor)
0 siblings, 1 reply; 10+ messages in thread
From: Thomas Monjalon @ 2021-05-12 14:36 UTC (permalink / raw)
To: Min Hu (Connor); +Cc: dev, ferruh.yigit, erik.g.carrillo, rsanford
11/05/2021 02:53, Min Hu (Connor):
> Segmentation fault may occur without checking if memzone
> reserves succeed or not.
>
> This patch fixed it.
>
> Fixes: 50247fe03fe0 ("test/timer: exercise new APIs in secondary process")
> Cc: stable@dpdk.org
>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
You missed reporting previous ack:
Acked-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
Applied, thanks
PS: please do not forget marking old patches as superseded
when sending a new version.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [dpdk-dev] [PATCH v3] test/timer: fix memzone reserve failure check
2021-05-12 14:36 ` Thomas Monjalon
@ 2021-05-13 0:19 ` Min Hu (Connor)
0 siblings, 0 replies; 10+ messages in thread
From: Min Hu (Connor) @ 2021-05-13 0:19 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, ferruh.yigit, erik.g.carrillo, rsanford
在 2021/5/12 22:36, Thomas Monjalon 写道:
> 11/05/2021 02:53, Min Hu (Connor):
>> Segmentation fault may occur without checking if memzone
>> reserves succeed or not.
>>
>> This patch fixed it.
>>
>> Fixes: 50247fe03fe0 ("test/timer: exercise new APIs in secondary process")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>
> You missed reporting previous ack:
> Acked-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
>
> Applied, thanks
>
> PS: please do not forget marking old patches as superseded
> when sending a new version.
Got it, thanks.
>
>
> .
>
^ permalink raw reply [flat|nested] 10+ messages in thread