| Message ID | 1469024689-1076-1-git-send-email-michalx.k.jastrzebski@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 5A2733990; Wed, 20 Jul 2016 16:25:04 +0200 (CEST) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id C091B3989 for <dev@dpdk.org>; Wed, 20 Jul 2016 16:25:02 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga103.jf.intel.com with ESMTP; 20 Jul 2016 07:25:01 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos; i="5.28,394,1464678000"; d="scan'208"; a="1025601069" Received: from unknown (HELO Sent) ([10.103.103.182]) by fmsmga002.fm.intel.com with SMTP; 20 Jul 2016 07:24:59 -0700 Received: by Sent (sSMTP sendmail emulation); Wed, 20 Jul 2016 16:24:58 +0200 From: Michal Jastrzebski <michalx.k.jastrzebski@intel.com> To: bruce.richardson@intel.com Cc: dev@dpdk.org, michalx.kobylinski@intel.com, sergio.gonzalez.monroy@intel.com, david.marchand@6wind.com Date: Wed, 20 Jul 2016 16:24:49 +0200 Message-Id: <1469024689-1076-1-git-send-email-michalx.k.jastrzebski@intel.com> X-Mailer: git-send-email 2.7.0 Subject: [dpdk-dev] [PATCH] eal: fix check number of bytes from read function X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Michal Jastrzebski
July 20, 2016, 2:24 p.m. UTC
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(-)
Comments
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; > }
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; > } > > /*
> -----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.
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.
> -----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.
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; } /*