diff mbox

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

Message ID 1469198030-8664-1-git-send-email-michalx.k.jastrzebski@intel.com (mailing list archive)
State Accepted, archived
Headers

Commit Message

Michal Jastrzebski July 22, 2016, 2:33 p.m. UTC 
  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(-)

Comments

Thomas Monjalon July 22, 2016, 3:24 p.m. UTC | #1 
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.
Sergio Gonzalez Monroy July 22, 2016, 4:02 p.m. UTC | #2 
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
Thomas Monjalon July 22, 2016, 4:22 p.m. UTC | #3 
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
Michal Jastrzebski July 22, 2016, 4:23 p.m. UTC | #4 
> -----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.
diff mbox

Patch

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;
 }