[dpdk-dev] [PATCH] eal: fix check number of bytes from read function

[dpdk-dev] [PATCH] eal: fix check number of bytes from read function

[Michal Jastrzebski]

In rte_mem_virt2phy: Value returned from a function and indicating the
number of bytes was ignored. This could cause a wrong pfn (page frame
number) mask read from pagemap file.
When read returns less than the number of sizeof(uint64_t) bytes,
function rte_mem_virt2phy returns error.

Coverity issue: 13212
Fixes: 40b966a211ab ("ivshmem: library changes for mmaping using
ivshmem").

Signed-off-by: Michal Jastrzebski <michalx.k.jastrzebski@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memory.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
index 42a29fa..05769fb 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -158,7 +158,7 @@ rte_mem_lock_page(const void *virt)
 phys_addr_t
 rte_mem_virt2phy(const void *virtaddr)
 {
-	int fd;
+	int fd, retval;
 	uint64_t page, physaddr;
 	unsigned long virt_pfn;
 	int page_size;
@@ -209,11 +209,19 @@ rte_mem_virt2phy(const void *virtaddr)
 		close(fd);
 		return RTE_BAD_PHYS_ADDR;
 	}
-	if (read(fd, &page, sizeof(uint64_t)) < 0) {
+
+	retval = read(fd, &page, sizeof(uint64_t));
+	if (retval < 0) {
 		RTE_LOG(ERR, EAL, "%s(): cannot read /proc/self/pagemap: %s\n",
 				__func__, strerror(errno));
 		close(fd);
 		return RTE_BAD_PHYS_ADDR;
+	}	else if (retval >= 0 && retval < (int)sizeof(uint64_t))	{
+		RTE_LOG(ERR, EAL, "%s(): read %d bytes from /proc/self/pagemap "
+				"but expected %d: %s\n",
+				__func__, retval, (int)sizeof(uint64_t), strerror(errno));
+		close(fd);
+		return RTE_BAD_PHYS_ADDR;
 	}
 
 	/*
-- 
1.7.9.5

Re: [dpdk-dev] [PATCH] eal: fix check number of bytes from read function

[Thomas Monjalon]

Hi,

2016-07-20 16:24, Michal Jastrzebski:
> -	if (read(fd, &page, sizeof(uint64_t)) < 0) {
> +
> +	retval = read(fd, &page, sizeof(uint64_t));
> +	if (retval < 0) {
>  		RTE_LOG(ERR, EAL, "%s(): cannot read /proc/self/pagemap: %s\n",
>  				__func__, strerror(errno));
>  		close(fd);
>  		return RTE_BAD_PHYS_ADDR;
> +	}	else if (retval >= 0 && retval < (int)sizeof(uint64_t))	{

I have 4 comments about the above line:
- the check retval >= 0 is not needed because implied by else
- why not checking retval != sizeof(uint64_t) as it is the exact expected value?
- (int)sizeof(uint64_t) can be replaced by 8 but it's shorter ;)
- only 1 space is required between } and else

> +		RTE_LOG(ERR, EAL, "%s(): read %d bytes from /proc/self/pagemap "
> +				"but expected %d: %s\n",
> +				__func__, retval, (int)sizeof(uint64_t), strerror(errno));

Are you sure errno is meaningful here?

> +		close(fd);
> +		return RTE_BAD_PHYS_ADDR;
>  	}

Re: [dpdk-dev] [PATCH] eal: fix check number of bytes from read function

[Jastrzebski, MichalX K]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, July 21, 2016 4:36 PM
> To: Jastrzebski, MichalX K <michalx.k.jastrzebski@intel.com>
> Cc: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>;
> Kobylinski, MichalX <michalx.kobylinski@intel.com>; Gonzalez Monroy,
> Sergio <sergio.gonzalez.monroy@intel.com>; david.marchand@6wind.com
> Subject: Re: [dpdk-dev] [PATCH] eal: fix check number of bytes from read
> function
> 
> Hi,
> 
> 2016-07-20 16:24, Michal Jastrzebski:
> > -	if (read(fd, &page, sizeof(uint64_t)) < 0) {
> > +
> > +	retval = read(fd, &page, sizeof(uint64_t));
> > +	if (retval < 0) {
> >  		RTE_LOG(ERR, EAL, "%s(): cannot read /proc/self/pagemap:
> %s\n",
> >  				__func__, strerror(errno));
> >  		close(fd);
> >  		return RTE_BAD_PHYS_ADDR;
> > +	}	else if (retval >= 0 && retval < (int)sizeof(uint64_t))	{
> 

Hi Thomas,

> I have 4 comments about the above line:
That's too much for one line. I should improve next time:)

> - the check retval >= 0 is not needed because implied by else
> - why not checking retval != sizeof(uint64_t) as it is the exact expected
> value?

Yes, it is better solution,

> - (int)sizeof(uint64_t) can be replaced by 8 but it's shorter ;)

I didn't want to change all invokes of read() function here. 
I can use some macro:
#define PFN_MASK_SIZE	8
How do You think?

> - only 1 space is required between } and else
> 
> > +		RTE_LOG(ERR, EAL, "%s(): read %d bytes from
> /proc/self/pagemap "
> > +				"but expected %d: %s\n",
> > +				__func__, retval, (int)sizeof(uint64_t),
> strerror(errno));
> 
> Are you sure errno is meaningful here?

I think it is not. Will send v2.
> 
> > +		close(fd);
> > +		return RTE_BAD_PHYS_ADDR;
> >  	}

Thanks for a review
Michal.

Re: [dpdk-dev] [PATCH] eal: fix check number of bytes from read function

[Thomas Monjalon]

2016-07-21 20:50, Jastrzebski, MichalX K:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > - (int)sizeof(uint64_t) can be replaced by 8 but it's shorter ;)
> 
> I didn't want to change all invokes of read() function here. 
> I can use some macro:
> #define PFN_MASK_SIZE	8
> How do You think?

Yes may be an idea.

Re: [dpdk-dev] [PATCH] eal: fix check number of bytes from read function

[Sergio Gonzalez Monroy]

On 20/07/2016 15:24, Michal Jastrzebski wrote:
> In rte_mem_virt2phy: Value returned from a function and indicating the
> number of bytes was ignored. This could cause a wrong pfn (page frame
> number) mask read from pagemap file.
> When read returns less than the number of sizeof(uint64_t) bytes,
> function rte_mem_virt2phy returns error.
>
> Coverity issue: 13212
> Fixes: 40b966a211ab ("ivshmem: library changes for mmaping using
> ivshmem").
>
> Signed-off-by: Michal Jastrzebski <michalx.k.jastrzebski@intel.com>
> ---
>   lib/librte_eal/linuxapp/eal/eal_memory.c |   12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
> index 42a29fa..05769fb 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_memory.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
> @@ -158,7 +158,7 @@ rte_mem_lock_page(const void *virt)
>   phys_addr_t
>   rte_mem_virt2phy(const void *virtaddr)
>   {
> -	int fd;
> +	int fd, retval;
>   	uint64_t page, physaddr;
>   	unsigned long virt_pfn;
>   	int page_size;
> @@ -209,11 +209,19 @@ rte_mem_virt2phy(const void *virtaddr)
>   		close(fd);
>   		return RTE_BAD_PHYS_ADDR;
>   	}
> -	if (read(fd, &page, sizeof(uint64_t)) < 0) {
> +
> +	retval = read(fd, &page, sizeof(uint64_t));
> +	if (retval < 0) {
>   		RTE_LOG(ERR, EAL, "%s(): cannot read /proc/self/pagemap: %s\n",
>   				__func__, strerror(errno));
>   		close(fd);
>   		return RTE_BAD_PHYS_ADDR;
> +	}	else if (retval >= 0 && retval < (int)sizeof(uint64_t))	{

Just a couple of nits, retval >= 0 it's already implicit, no need to do 
that check.

> +		RTE_LOG(ERR, EAL, "%s(): read %d bytes from /proc/self/pagemap "
> +				"but expected %d: %s\n",
> +				__func__, retval, (int)sizeof(uint64_t), strerror(errno));
> +		close(fd);

Another nit, we could just close(fd) right after read, regardless of 
read being success or error as
we close(fd) also on success just before exiting the function.

Other than that:

Acked-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>

> +		return RTE_BAD_PHYS_ADDR;
>   	}
>   
>   	/*

Re: [dpdk-dev] [PATCH] eal: fix check number of bytes from read function

[Jastrzebski, MichalX K]

> -----Original Message-----
> From: Gonzalez Monroy, Sergio
> Sent: Thursday, July 21, 2016 4:37 PM
> To: Jastrzebski, MichalX K <michalx.k.jastrzebski@intel.com>; Richardson,
> Bruce <bruce.richardson@intel.com>
> Cc: dev@dpdk.org; Kobylinski, MichalX <michalx.kobylinski@intel.com>;
> david.marchand@6wind.com
> Subject: Re: [PATCH] eal: fix check number of bytes from read function
> 
> On 20/07/2016 15:24, Michal Jastrzebski wrote:
> > In rte_mem_virt2phy: Value returned from a function and indicating the
> > number of bytes was ignored. This could cause a wrong pfn (page frame
> > number) mask read from pagemap file.
> > When read returns less than the number of sizeof(uint64_t) bytes,
> > function rte_mem_virt2phy returns error.
> >
> > Coverity issue: 13212
> > Fixes: 40b966a211ab ("ivshmem: library changes for mmaping using
> > ivshmem").
> >
> > Signed-off-by: Michal Jastrzebski <michalx.k.jastrzebski@intel.com>
> > ---
> >   lib/librte_eal/linuxapp/eal/eal_memory.c |   12 ++++++++++--
> >   1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c
> b/lib/librte_eal/linuxapp/eal/eal_memory.c
> > index 42a29fa..05769fb 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal_memory.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
> > @@ -158,7 +158,7 @@ rte_mem_lock_page(const void *virt)
> >   phys_addr_t
> >   rte_mem_virt2phy(const void *virtaddr)
> >   {
> > -	int fd;
> > +	int fd, retval;
> >   	uint64_t page, physaddr;
> >   	unsigned long virt_pfn;
> >   	int page_size;
> > @@ -209,11 +209,19 @@ rte_mem_virt2phy(const void *virtaddr)
> >   		close(fd);
> >   		return RTE_BAD_PHYS_ADDR;
> >   	}
> > -	if (read(fd, &page, sizeof(uint64_t)) < 0) {
> > +
> > +	retval = read(fd, &page, sizeof(uint64_t));
> > +	if (retval < 0) {
> >   		RTE_LOG(ERR, EAL, "%s(): cannot read /proc/self/pagemap:
> %s\n",
> >   				__func__, strerror(errno));
> >   		close(fd);
> >   		return RTE_BAD_PHYS_ADDR;
> > +	}	else if (retval >= 0 && retval < (int)sizeof(uint64_t))	{
> 
> Just a couple of nits, retval >= 0 it's already implicit, no need to do
> that check.
> 
> > +		RTE_LOG(ERR, EAL, "%s(): read %d bytes from
> /proc/self/pagemap "
> > +				"but expected %d: %s\n",
> > +				__func__, retval, (int)sizeof(uint64_t),
> strerror(errno));
> > +		close(fd);
> 
> Another nit, we could just close(fd) right after read, regardless of
> read being success or error as
> we close(fd) also on success just before exiting the function.
> 
> Other than that:
> 
> Acked-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>
> 
> > +		return RTE_BAD_PHYS_ADDR;
> >   	}
> >
> >   	/*

Thanks Sergio,
I have sent v2 with the changes that You suggest

Michal.

[dpdk-dev] [PATCH v2] eal: fix check number of bytes from read function

[Michal Jastrzebski]

v2:
-moved close(fd) just after read.
-when read() from fd we expect 8 bytes, so PFN_MASK_SIZE macro 
was introduced instead sizeof(uint64_t).
-removed errno print when read returns less than 8 bytes

In rte_mem_virt2phy: Value returned from a function and indicating the
number of bytes was ignored. This could cause a wrong pfn (page frame
number) mask read from pagemap file.
When read returns less than the number of sizeof(uint64_t) bytes,
function rte_mem_virt2phy returns error.

Coverity issue: 13212
Fixes: 40b966a211ab ("ivshmem: library changes for mmaping using
ivshmem").

Signed-off-by: Michal Jastrzebski <michalx.k.jastrzebski@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memory.c |   19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
index 42a29fa..e20a38c 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -99,6 +99,8 @@
 #include "eal_filesystem.h"
 #include "eal_hugepages.h"
 
+#define PFN_MASK_SIZE	8
+
 #ifdef RTE_LIBRTE_XEN_DOM0
 int rte_xen_dom0_supported(void)
 {
@@ -158,7 +160,7 @@ rte_mem_lock_page(const void *virt)
 phys_addr_t
 rte_mem_virt2phy(const void *virtaddr)
 {
-	int fd;
+	int fd, retval;
 	uint64_t page, physaddr;
 	unsigned long virt_pfn;
 	int page_size;
@@ -209,10 +211,19 @@ rte_mem_virt2phy(const void *virtaddr)
 		close(fd);
 		return RTE_BAD_PHYS_ADDR;
 	}
-	if (read(fd, &page, sizeof(uint64_t)) < 0) {
+
+	retval = read(fd, &page, sizeof(uint64_t));
+	close(fd);
+	if (retval < 0) {
 		RTE_LOG(ERR, EAL, "%s(): cannot read /proc/self/pagemap: %s\n",
 				__func__, strerror(errno));
-		close(fd);
+
+		return RTE_BAD_PHYS_ADDR;
+	} else if (retval != PFN_MASK_SIZE) {
+		RTE_LOG(ERR, EAL, "%s(): read %d bytes from /proc/self/pagemap "
+				"but expected %d:\n",
+				__func__, retval, PFN_MASK_SIZE);
+
 		return RTE_BAD_PHYS_ADDR;
 	}
 
@@ -222,7 +233,7 @@ rte_mem_virt2phy(const void *virtaddr)
 	 */
 	physaddr = ((page & 0x7fffffffffffffULL) * page_size)
 		+ ((unsigned long)virtaddr % page_size);
-	close(fd);
+
 	return physaddr;
 }
 
-- 
1.7.9.5

Re: [dpdk-dev] [PATCH v2] eal: fix check number of bytes from read function

[Thomas Monjalon]

2016-07-22 16:33, Michal Jastrzebski:
> v2:
> -moved close(fd) just after read.
> -when read() from fd we expect 8 bytes, so PFN_MASK_SIZE macro 
> was introduced instead sizeof(uint64_t).
> -removed errno print when read returns less than 8 bytes

Looks better.
Note: this changelog should be below --- to avoid appearing in
the commit.

> In rte_mem_virt2phy: Value returned from a function and indicating the
> number of bytes was ignored. This could cause a wrong pfn (page frame
> number) mask read from pagemap file.
> When read returns less than the number of sizeof(uint64_t) bytes,
> function rte_mem_virt2phy returns error.

Better title to explain the issue:
mem: fix check of physical address retrieval

> +#define PFN_MASK_SIZE	8
> +
>  #ifdef RTE_LIBRTE_XEN_DOM0
>  int rte_xen_dom0_supported(void)
>  {
> @@ -158,7 +160,7 @@ rte_mem_lock_page(const void *virt)
>  phys_addr_t
>  rte_mem_virt2phy(const void *virtaddr)
>  {
> -	int fd;
> +	int fd, retval;
>  	uint64_t page, physaddr;
>  	unsigned long virt_pfn;
>  	int page_size;
> @@ -209,10 +211,19 @@ rte_mem_virt2phy(const void *virtaddr)
>  		close(fd);
>  		return RTE_BAD_PHYS_ADDR;
>  	}
> -	if (read(fd, &page, sizeof(uint64_t)) < 0) {
> +
> +	retval = read(fd, &page, sizeof(uint64_t));

We could use PFN_MASK_SIZE here

> +	close(fd);
> +	if (retval < 0) {
>  		RTE_LOG(ERR, EAL, "%s(): cannot read /proc/self/pagemap: %s\n",
>  				__func__, strerror(errno));
> -		close(fd);
> +

useless whitespace

> +		return RTE_BAD_PHYS_ADDR;
> +	} else if (retval != PFN_MASK_SIZE) {
> +		RTE_LOG(ERR, EAL, "%s(): read %d bytes from /proc/self/pagemap "
> +				"but expected %d:\n",
> +				__func__, retval, PFN_MASK_SIZE);
> +

useless whitespace

>  		return RTE_BAD_PHYS_ADDR;
>  	}
>  
> @@ -222,7 +233,7 @@ rte_mem_virt2phy(const void *virtaddr)
>  	 */
>  	physaddr = ((page & 0x7fffffffffffffULL) * page_size)
>  		+ ((unsigned long)virtaddr % page_size);
> -	close(fd);
> +
>  	return physaddr;
>  }

If you and Sergio agree, I can make the minor changes before applying.

Re: [dpdk-dev] [PATCH v2] eal: fix check number of bytes from read function

[Sergio Gonzalez Monroy]

On 22/07/2016 16:24, Thomas Monjalon wrote:
> 2016-07-22 16:33, Michal Jastrzebski:
>> v2:
>> -moved close(fd) just after read.
>> -when read() from fd we expect 8 bytes, so PFN_MASK_SIZE macro
>> was introduced instead sizeof(uint64_t).
>> -removed errno print when read returns less than 8 bytes
> Looks better.
> Note: this changelog should be below --- to avoid appearing in
> the commit.
>
>> In rte_mem_virt2phy: Value returned from a function and indicating the
>> number of bytes was ignored. This could cause a wrong pfn (page frame
>> number) mask read from pagemap file.
>> When read returns less than the number of sizeof(uint64_t) bytes,
>> function rte_mem_virt2phy returns error.
> Better title to explain the issue:
> mem: fix check of physical address retrieval
>
>> +#define PFN_MASK_SIZE	8
>> +
>>   #ifdef RTE_LIBRTE_XEN_DOM0
>>   int rte_xen_dom0_supported(void)
>>   {
>> @@ -158,7 +160,7 @@ rte_mem_lock_page(const void *virt)
>>   phys_addr_t
>>   rte_mem_virt2phy(const void *virtaddr)
>>   {
>> -	int fd;
>> +	int fd, retval;
>>   	uint64_t page, physaddr;
>>   	unsigned long virt_pfn;
>>   	int page_size;
>> @@ -209,10 +211,19 @@ rte_mem_virt2phy(const void *virtaddr)
>>   		close(fd);
>>   		return RTE_BAD_PHYS_ADDR;
>>   	}
>> -	if (read(fd, &page, sizeof(uint64_t)) < 0) {
>> +
>> +	retval = read(fd, &page, sizeof(uint64_t));
> We could use PFN_MASK_SIZE here
>
>> +	close(fd);
>> +	if (retval < 0) {
>>   		RTE_LOG(ERR, EAL, "%s(): cannot read /proc/self/pagemap: %s\n",
>>   				__func__, strerror(errno));
>> -		close(fd);
>> +
> useless whitespace
>
>> +		return RTE_BAD_PHYS_ADDR;
>> +	} else if (retval != PFN_MASK_SIZE) {
>> +		RTE_LOG(ERR, EAL, "%s(): read %d bytes from /proc/self/pagemap "
>> +				"but expected %d:\n",
>> +				__func__, retval, PFN_MASK_SIZE);
>> +
> useless whitespace
>
>>   		return RTE_BAD_PHYS_ADDR;
>>   	}
>>   
>> @@ -222,7 +233,7 @@ rte_mem_virt2phy(const void *virtaddr)
>>   	 */
>>   	physaddr = ((page & 0x7fffffffffffffULL) * page_size)
>>   		+ ((unsigned long)virtaddr % page_size);
>> -	close(fd);
>> +
>>   	return physaddr;
>>   }
> If you and Sergio agree, I can make the minor changes before applying.

Go for it!

Sergio

Re: [dpdk-dev] [PATCH v2] eal: fix check number of bytes from read function

[Thomas Monjalon]

2016-07-22 17:02, Sergio Gonzalez Monroy:
> On 22/07/2016 16:24, Thomas Monjalon wrote:
> > 2016-07-22 16:33, Michal Jastrzebski:
> >> v2:
> >> -moved close(fd) just after read.
> >> -when read() from fd we expect 8 bytes, so PFN_MASK_SIZE macro
> >> was introduced instead sizeof(uint64_t).
> >> -removed errno print when read returns less than 8 bytes
> > Looks better.
[...]
> > Better title to explain the issue:
> > mem: fix check of physical address retrieval
[...]
> >> +	retval = read(fd, &page, sizeof(uint64_t));
> > We could use PFN_MASK_SIZE here
[...]
> > useless whitespace
[...]
> > If you and Sergio agree, I can make the minor changes before applying.
> 
> Go for it!

Applied with above changes, thanks

Re: [dpdk-dev] [PATCH v2] eal: fix check number of bytes from read function

[Jastrzebski, MichalX K]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, July 22, 2016 5:25 PM
> To: Jastrzebski, MichalX K <michalx.k.jastrzebski@intel.com>
> Cc: Richardson, Bruce <bruce.richardson@intel.com>; dev@dpdk.org;
> Kobylinski, MichalX <michalx.kobylinski@intel.com>; Gonzalez Monroy,
> Sergio <sergio.gonzalez.monroy@intel.com>; david.marchand@6wind.com
> Subject: Re: [PATCH v2] eal: fix check number of bytes from read function
> 
> 2016-07-22 16:33, Michal Jastrzebski:
> > v2:
> > -moved close(fd) just after read.
> > -when read() from fd we expect 8 bytes, so PFN_MASK_SIZE macro
> > was introduced instead sizeof(uint64_t).
> > -removed errno print when read returns less than 8 bytes
> 
> Looks better.
> Note: this changelog should be below --- to avoid appearing in
> the commit.
> 
> > In rte_mem_virt2phy: Value returned from a function and indicating the
> > number of bytes was ignored. This could cause a wrong pfn (page frame
> > number) mask read from pagemap file.
> > When read returns less than the number of sizeof(uint64_t) bytes,
> > function rte_mem_virt2phy returns error.
> 
> Better title to explain the issue:
> mem: fix check of physical address retrieval
> 
> > +#define PFN_MASK_SIZE	8
> > +
> >  #ifdef RTE_LIBRTE_XEN_DOM0
> >  int rte_xen_dom0_supported(void)
> >  {
> > @@ -158,7 +160,7 @@ rte_mem_lock_page(const void *virt)
> >  phys_addr_t
> >  rte_mem_virt2phy(const void *virtaddr)
> >  {
> > -	int fd;
> > +	int fd, retval;
> >  	uint64_t page, physaddr;
> >  	unsigned long virt_pfn;
> >  	int page_size;
> > @@ -209,10 +211,19 @@ rte_mem_virt2phy(const void *virtaddr)
> >  		close(fd);
> >  		return RTE_BAD_PHYS_ADDR;
> >  	}
> > -	if (read(fd, &page, sizeof(uint64_t)) < 0) {
> > +
> > +	retval = read(fd, &page, sizeof(uint64_t));
> 
> We could use PFN_MASK_SIZE here
> 
> > +	close(fd);
> > +	if (retval < 0) {
> >  		RTE_LOG(ERR, EAL, "%s(): cannot read /proc/self/pagemap:
> %s\n",
> >  				__func__, strerror(errno));
> > -		close(fd);
> > +
> 
> useless whitespace
> 
> > +		return RTE_BAD_PHYS_ADDR;
> > +	} else if (retval != PFN_MASK_SIZE) {
> > +		RTE_LOG(ERR, EAL, "%s(): read %d bytes from
> /proc/self/pagemap "
> > +				"but expected %d:\n",
> > +				__func__, retval, PFN_MASK_SIZE);
> > +
> 
> useless whitespace
> 
> >  		return RTE_BAD_PHYS_ADDR;
> >  	}
> >
> > @@ -222,7 +233,7 @@ rte_mem_virt2phy(const void *virtaddr)
> >  	 */
> >  	physaddr = ((page & 0x7fffffffffffffULL) * page_size)
> >  		+ ((unsigned long)virtaddr % page_size);
> > -	close(fd);
> > +
> >  	return physaddr;
> >  }
> 
> If you and Sergio agree, I can make the minor changes before applying.

Thanks Thomas. Please apply.