DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 0/2] Coverity fixes for EAL
@ 2018-04-17 15:42 Anatoly Burakov
  2018-04-17 15:42 ` [dpdk-dev] [PATCH 1/2] eal: fix potential negative return Anatoly Burakov
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Anatoly Burakov @ 2018-04-17 15:42 UTC (permalink / raw)
  To: dev; +Cc: thomas

This patchset fixes a few Coverity fixes in EAL introduced
by recent DPDK memory hotplug patchset.

Coverity issues fixed:
- 272600 - negative return not handled
- 272607 - error condition not handled

One of existing coverity issues is not fixed by this patchset:
- 272585 - memory leak

due to being independently discovered and addressed in a
separate patch [1].

[1] http://dpdk.org/dev/patchwork/patch/38301/

Anatoly Burakov (2):
  eal: fix potential negative return
  eal: fix not checking unlock result

 lib/librte_eal/linuxapp/eal/eal_hugepage_info.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [dpdk-dev] [PATCH 1/2] eal: fix potential negative return
  2018-04-17 15:42 [dpdk-dev] [PATCH 0/2] Coverity fixes for EAL Anatoly Burakov
@ 2018-04-17 15:42 ` Anatoly Burakov
  2018-04-25  5:55   ` Tan, Jianfeng
  2018-04-17 15:42 ` [dpdk-dev] [PATCH 2/2] eal: fix not checking unlock result Anatoly Burakov
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Anatoly Burakov @ 2018-04-17 15:42 UTC (permalink / raw)
  To: dev; +Cc: thomas, anatoly.burakov

Value returned by rte_socket_id_by_idx() may be negative, which would
result in negative array index access.

Coverity issue: 272600

Fixes: b666f17858a3 ("mem: read hugepage counts from node-specific sysfs path")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_hugepage_info.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c b/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
index fb4b667..009f963 100644
--- a/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
+++ b/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
@@ -402,9 +402,16 @@ hugepage_info_init(void)
 		/* we also don't want to do this for legacy init */
 		if (!internal_config.legacy_mem)
 			for (i = 0; i < rte_socket_count(); i++) {
-				int socket = rte_socket_id_by_idx(i);
-				unsigned int num_pages =
-						get_num_hugepages_on_node(
+				int socket;
+				unsigned int num_pages;
+
+				socket = rte_socket_id_by_idx(i);
+				if (socket < 0) {
+					RTE_LOG(ERR, EAL, "Invalid socket index: %u\n",
+							i);
+					continue;
+				}
+				num_pages = get_num_hugepages_on_node(
 							dirent->d_name, socket);
 				hpi->num_pages[socket] = num_pages;
 				total_pages += num_pages;
-- 
2.7.4

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [dpdk-dev] [PATCH 2/2] eal: fix not checking unlock result
  2018-04-17 15:42 [dpdk-dev] [PATCH 0/2] Coverity fixes for EAL Anatoly Burakov
  2018-04-17 15:42 ` [dpdk-dev] [PATCH 1/2] eal: fix potential negative return Anatoly Burakov
@ 2018-04-17 15:42 ` Anatoly Burakov
  2018-04-25  7:09   ` Tan, Jianfeng
  2018-04-25 10:08 ` [dpdk-dev] [PATCH v2] Coverity fixes for EAL Anatoly Burakov
  2018-04-25 10:08 ` [dpdk-dev] [PATCH v2] eal: remove call to unlock Anatoly Burakov
  3 siblings, 1 reply; 9+ messages in thread
From: Anatoly Burakov @ 2018-04-17 15:42 UTC (permalink / raw)
  To: dev; +Cc: thomas, anatoly.burakov

Coverity issue: 272607

Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_hugepage_info.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c b/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
index 009f963..01fee51 100644
--- a/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
+++ b/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
@@ -304,8 +304,11 @@ clear_hugedir(const char * hugedir)
 		/* if lock succeeds, unlock and remove the file */
 		if (lck_result != -1) {
 			lck.l_type = F_UNLCK;
-			fcntl(fd, F_SETLK, &lck);
-			unlinkat(dir_fd, dirent->d_name, 0);
+			if (fcntl(fd, F_SETLK, &lck) < 0)
+				RTE_LOG(ERR, EAL, "Couldn't unlock %s: %s\n",
+					dirent->d_name, strerror(errno));
+			else
+				unlinkat(dir_fd, dirent->d_name, 0);
 		}
 		close (fd);
 		dirent = readdir(dir);
-- 
2.7.4

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] eal: fix potential negative return
  2018-04-17 15:42 ` [dpdk-dev] [PATCH 1/2] eal: fix potential negative return Anatoly Burakov
@ 2018-04-25  5:55   ` Tan, Jianfeng
  0 siblings, 0 replies; 9+ messages in thread
From: Tan, Jianfeng @ 2018-04-25  5:55 UTC (permalink / raw)
  To: Anatoly Burakov, dev; +Cc: thomas



On 4/17/2018 11:42 PM, Anatoly Burakov wrote:
> Value returned by rte_socket_id_by_idx() may be negative, which would
> result in negative array index access.
>
> Coverity issue: 272600
>
> Fixes: b666f17858a3 ("mem: read hugepage counts from node-specific sysfs path")
> Cc: anatoly.burakov@intel.com
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>

Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] eal: fix not checking unlock result
  2018-04-17 15:42 ` [dpdk-dev] [PATCH 2/2] eal: fix not checking unlock result Anatoly Burakov
@ 2018-04-25  7:09   ` Tan, Jianfeng
  2018-04-25  8:53     ` Burakov, Anatoly
  0 siblings, 1 reply; 9+ messages in thread
From: Tan, Jianfeng @ 2018-04-25  7:09 UTC (permalink / raw)
  To: Anatoly Burakov, dev; +Cc: thomas



On 4/17/2018 11:42 PM, Anatoly Burakov wrote:
> Coverity issue: 272607
>
> Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
> Cc: anatoly.burakov@intel.com
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>   lib/librte_eal/linuxapp/eal/eal_hugepage_info.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c b/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
> index 009f963..01fee51 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
> @@ -304,8 +304,11 @@ clear_hugedir(const char * hugedir)
>   		/* if lock succeeds, unlock and remove the file */
>   		if (lck_result != -1) {
>   			lck.l_type = F_UNLCK;
> -			fcntl(fd, F_SETLK, &lck);
> -			unlinkat(dir_fd, dirent->d_name, 0);
> +			if (fcntl(fd, F_SETLK, &lck) < 0)
> +				RTE_LOG(ERR, EAL, "Couldn't unlock %s: %s\n",
> +					dirent->d_name, strerror(errno));

It seems that we shall return error if this nearly-impossible error 
happens, no?

> +			else
> +				unlinkat(dir_fd, dirent->d_name, 0);
>   		}
>   		close (fd);
>   		dirent = readdir(dir);

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] eal: fix not checking unlock result
  2018-04-25  7:09   ` Tan, Jianfeng
@ 2018-04-25  8:53     ` Burakov, Anatoly
  0 siblings, 0 replies; 9+ messages in thread
From: Burakov, Anatoly @ 2018-04-25  8:53 UTC (permalink / raw)
  To: Tan, Jianfeng, dev; +Cc: thomas

On 25-Apr-18 8:09 AM, Tan, Jianfeng wrote:
> 
> 
> On 4/17/2018 11:42 PM, Anatoly Burakov wrote:
>> Coverity issue: 272607
>>
>> Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
>> Cc: anatoly.burakov@intel.com
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> ---
>>   lib/librte_eal/linuxapp/eal/eal_hugepage_info.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c 
>> b/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
>> index 009f963..01fee51 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
>> @@ -304,8 +304,11 @@ clear_hugedir(const char * hugedir)
>>           /* if lock succeeds, unlock and remove the file */
>>           if (lck_result != -1) {
>>               lck.l_type = F_UNLCK;
>> -            fcntl(fd, F_SETLK, &lck);
>> -            unlinkat(dir_fd, dirent->d_name, 0);
>> +            if (fcntl(fd, F_SETLK, &lck) < 0)
>> +                RTE_LOG(ERR, EAL, "Couldn't unlock %s: %s\n",
>> +                    dirent->d_name, strerror(errno));
> 
> It seems that we shall return error if this nearly-impossible error 
> happens, no?

I'm not sure if we should. In any case, lock will be dropped by close(), 
so the proper fix would've been just remove the fcntl() call altogether.

I'll respin.

> 
>> +            else
>> +                unlinkat(dir_fd, dirent->d_name, 0);
>>           }
>>           close (fd);
>>           dirent = readdir(dir);
> 
> 


-- 
Thanks,
Anatoly

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [dpdk-dev] [PATCH v2] Coverity fixes for EAL
  2018-04-17 15:42 [dpdk-dev] [PATCH 0/2] Coverity fixes for EAL Anatoly Burakov
  2018-04-17 15:42 ` [dpdk-dev] [PATCH 1/2] eal: fix potential negative return Anatoly Burakov
  2018-04-17 15:42 ` [dpdk-dev] [PATCH 2/2] eal: fix not checking unlock result Anatoly Burakov
@ 2018-04-25 10:08 ` Anatoly Burakov
  2018-04-25 10:08 ` [dpdk-dev] [PATCH v2] eal: remove call to unlock Anatoly Burakov
  3 siblings, 0 replies; 9+ messages in thread
From: Anatoly Burakov @ 2018-04-25 10:08 UTC (permalink / raw)
  To: dev; +Cc: thomas

This patchset fixes a few Coverity fixes in EAL introduced
by recent DPDK memory hotplug patchset.

Coverity issues fixed:
- 272607 - error condition not handled

Coverity issues not fixed:
- 272600 - negative return not handled
  - Proper usage of API guarantees no negative return

One of existing coverity issues is not fixed by this patchset:
- 272585 - memory leak

due to being independently discovered and addressed in a
separate patch [1].

[1] http://dpdk.org/dev/patchwork/patch/38301/

v2:
- Dropped fix for 272600 and marked it as false positive

Anatoly Burakov (1):
  eal: remove call to unlock

 lib/librte_eal/linuxapp/eal/eal_hugepage_info.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [dpdk-dev] [PATCH v2] eal: remove call to unlock
  2018-04-17 15:42 [dpdk-dev] [PATCH 0/2] Coverity fixes for EAL Anatoly Burakov
                   ` (2 preceding siblings ...)
  2018-04-25 10:08 ` [dpdk-dev] [PATCH v2] Coverity fixes for EAL Anatoly Burakov
@ 2018-04-25 10:08 ` Anatoly Burakov
  2018-04-25 10:32   ` Thomas Monjalon
  3 siblings, 1 reply; 9+ messages in thread
From: Anatoly Burakov @ 2018-04-25 10:08 UTC (permalink / raw)
  To: dev; +Cc: thomas, anatoly.burakov

Coverity was complaining about not checking result of call to
fcntl() for unlocking the file. Disregarding the fact that error
value returned from fcntl() unlock call is highly unlikely in the
first place, we are subsequently calling close() on that same fd,
which will drop the lock, which makes call to fcntl() unnecessary.

Fix this by removing a call to fcntl() altogether.

Coverity issue: 272607

Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---

Notes:
    v2:
    - Removed call to fcntl() instead of handling return value

 lib/librte_eal/linuxapp/eal/eal_hugepage_info.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c b/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
index db5aabd..485a89e 100644
--- a/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
+++ b/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
@@ -304,11 +304,8 @@ clear_hugedir(const char * hugedir)
 		lck_result = fcntl(fd, F_SETLK, &lck);
 
 		/* if lock succeeds, unlock and remove the file */
-		if (lck_result != -1) {
-			lck.l_type = F_UNLCK;
-			fcntl(fd, F_SETLK, &lck);
+		if (lck_result != -1)
 			unlinkat(dir_fd, dirent->d_name, 0);
-		}
 		close (fd);
 		dirent = readdir(dir);
 	}
-- 
2.7.4

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH v2] eal: remove call to unlock
  2018-04-25 10:08 ` [dpdk-dev] [PATCH v2] eal: remove call to unlock Anatoly Burakov
@ 2018-04-25 10:32   ` Thomas Monjalon
  0 siblings, 0 replies; 9+ messages in thread
From: Thomas Monjalon @ 2018-04-25 10:32 UTC (permalink / raw)
  To: Anatoly Burakov; +Cc: dev

25/04/2018 12:08, Anatoly Burakov:
> Coverity was complaining about not checking result of call to
> fcntl() for unlocking the file. Disregarding the fact that error
> value returned from fcntl() unlock call is highly unlikely in the
> first place, we are subsequently calling close() on that same fd,
> which will drop the lock, which makes call to fcntl() unnecessary.
> 
> Fix this by removing a call to fcntl() altogether.
> 
> Coverity issue: 272607
> 
> Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
> Cc: anatoly.burakov@intel.com
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>

Applied, thanks

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-04-25 10:32 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-17 15:42 [dpdk-dev] [PATCH 0/2] Coverity fixes for EAL Anatoly Burakov
2018-04-17 15:42 ` [dpdk-dev] [PATCH 1/2] eal: fix potential negative return Anatoly Burakov
2018-04-25  5:55   ` Tan, Jianfeng
2018-04-17 15:42 ` [dpdk-dev] [PATCH 2/2] eal: fix not checking unlock result Anatoly Burakov
2018-04-25  7:09   ` Tan, Jianfeng
2018-04-25  8:53     ` Burakov, Anatoly
2018-04-25 10:08 ` [dpdk-dev] [PATCH v2] Coverity fixes for EAL Anatoly Burakov
2018-04-25 10:08 ` [dpdk-dev] [PATCH v2] eal: remove call to unlock Anatoly Burakov
2018-04-25 10:32   ` Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).