* [PATCH] vfio: correct system call error checking @ 2014-06-17 19:03 Neil Horman [not found] ` <1403031832-28540-1-git-send-email-nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org> 0 siblings, 1 reply; 4+ messages in thread From: Neil Horman @ 2014-06-17 19:03 UTC (permalink / raw) To: dev-VfR2kkLFssw Noticed today that ioctl error code return checking was incorrect in some of the vfio code. ioctl can return a negative value if the system detects an error before the target device/driver can produce a return code. The dpdk vfio code only checks specfically for the values that it expects, which leaves it open to accepting unexpected error codes as success. For instance, if the vfio layer noted that the iommu driver hadn't finished registering yet, it would return an -EINVAL error code, but the dpdk would accept that as success, becuase it wasn't 0. Fix this to specifically check for < 0 error codes Signed-off-by: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org> CC: Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org> --- lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c index 4de6061..65aa8ad 100644 --- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c @@ -319,16 +319,16 @@ pci_vfio_get_container_fd(void) /* check VFIO API version */ ret = ioctl(vfio_container_fd, VFIO_GET_API_VERSION); - if (ret != VFIO_API_VERSION) { - RTE_LOG(ERR, EAL, " unknown VFIO API version!\n"); + if ((ret < 0) || (ret != VFIO_API_VERSION)) { + RTE_LOG(ERR, EAL, " unknown VFIO API version! errno = %d\n", errno); close(vfio_container_fd); return -1; } /* check if we support IOMMU type 1 */ ret = ioctl(vfio_container_fd, VFIO_CHECK_EXTENSION, VFIO_TYPE1_IOMMU); - if (!ret) { - RTE_LOG(ERR, EAL, " unknown IOMMU driver!\n"); + if (ret <= 0) { + RTE_LOG(ERR, EAL, " unknown IOMMU driver! errno = %d\n", errno); close(vfio_container_fd); return -1; } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
[parent not found: <1403031832-28540-1-git-send-email-nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>]
* Re: [PATCH] vfio: correct system call error checking [not found] ` <1403031832-28540-1-git-send-email-nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org> @ 2014-06-17 20:21 ` Richardson, Bruce [not found] ` <59AF69C657FD0841A61C55336867B5B01AA37370-kPTMFJFq+rELt2AQoY/u9bfspsVTdybXVpNB7YpNyf8@public.gmane.org> 0 siblings, 1 reply; 4+ messages in thread From: Richardson, Bruce @ 2014-06-17 20:21 UTC (permalink / raw) To: Neil Horman, dev-VfR2kkLFssw@public.gmane.org > -----Original Message----- > From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Neil Horman > Sent: Tuesday, June 17, 2014 12:04 PM > To: dev-VfR2kkLFssw@public.gmane.org > Subject: [dpdk-dev] [PATCH] vfio: correct system call error checking > > Noticed today that ioctl error code return checking was incorrect in some of the > vfio code. ioctl can return a negative value if the system detects an error > before the target device/driver can produce a return code. The dpdk vfio code > only checks specfically for the values that it expects, which leaves it open to > accepting unexpected error codes as success. For instance, if the vfio layer > noted that the iommu driver hadn't finished registering yet, it would return an > -EINVAL error code, but the dpdk would accept that as success, becuase it > wasn't > 0. > > Fix this to specifically check for < 0 error codes > > Signed-off-by: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org> > CC: Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org> > --- > lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c > b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c > index 4de6061..65aa8ad 100644 > --- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c > +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c > @@ -319,16 +319,16 @@ pci_vfio_get_container_fd(void) > > /* check VFIO API version */ > ret = ioctl(vfio_container_fd, VFIO_GET_API_VERSION); > - if (ret != VFIO_API_VERSION) { > - RTE_LOG(ERR, EAL, " unknown VFIO API version!\n"); > + if ((ret < 0) || (ret != VFIO_API_VERSION)) { > + RTE_LOG(ERR, EAL, " unknown VFIO API version! errno > = %d\n", errno); > close(vfio_container_fd); > return -1; > } Not sure how this change improves things, since the existing check will already trigger an error on all values <0. Can you please clarify why you think this needs to be changed? > > /* check if we support IOMMU type 1 */ > ret = ioctl(vfio_container_fd, VFIO_CHECK_EXTENSION, > VFIO_TYPE1_IOMMU); > - if (!ret) { > - RTE_LOG(ERR, EAL, " unknown IOMMU driver!\n"); > + if (ret <= 0) { > + RTE_LOG(ERR, EAL, " unknown IOMMU driver! errno = > %d\n", errno); > close(vfio_container_fd); > return -1; > } Ack on this change part. The previously code was incorrect according to what I read in the docs for VFIO. /Bruce ^ permalink raw reply [flat|nested] 4+ messages in thread
[parent not found: <59AF69C657FD0841A61C55336867B5B01AA37370-kPTMFJFq+rELt2AQoY/u9bfspsVTdybXVpNB7YpNyf8@public.gmane.org>]
* Re: [PATCH] vfio: correct system call error checking [not found] ` <59AF69C657FD0841A61C55336867B5B01AA37370-kPTMFJFq+rELt2AQoY/u9bfspsVTdybXVpNB7YpNyf8@public.gmane.org> @ 2014-06-17 20:40 ` Neil Horman [not found] ` <20140617204011.GH8539-B26myB8xz7F8NnZeBjwnZQMhkBWG/bsMQH7oEaQurus@public.gmane.org> 0 siblings, 1 reply; 4+ messages in thread From: Neil Horman @ 2014-06-17 20:40 UTC (permalink / raw) To: Richardson, Bruce; +Cc: dev-VfR2kkLFssw@public.gmane.org On Tue, Jun 17, 2014 at 08:21:29PM +0000, Richardson, Bruce wrote: > > -----Original Message----- > > From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Neil Horman > > Sent: Tuesday, June 17, 2014 12:04 PM > > To: dev-VfR2kkLFssw@public.gmane.org > > Subject: [dpdk-dev] [PATCH] vfio: correct system call error checking > > > > Noticed today that ioctl error code return checking was incorrect in some of the > > vfio code. ioctl can return a negative value if the system detects an error > > before the target device/driver can produce a return code. The dpdk vfio code > > only checks specfically for the values that it expects, which leaves it open to > > accepting unexpected error codes as success. For instance, if the vfio layer > > noted that the iommu driver hadn't finished registering yet, it would return an > > -EINVAL error code, but the dpdk would accept that as success, becuase it > > wasn't > > 0. > > > > Fix this to specifically check for < 0 error codes > > > > Signed-off-by: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org> > > CC: Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org> > > --- > > lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c > > b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c > > index 4de6061..65aa8ad 100644 > > --- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c > > +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c > > @@ -319,16 +319,16 @@ pci_vfio_get_container_fd(void) > > > > /* check VFIO API version */ > > ret = ioctl(vfio_container_fd, VFIO_GET_API_VERSION); > > - if (ret != VFIO_API_VERSION) { > > - RTE_LOG(ERR, EAL, " unknown VFIO API version!\n"); > > + if ((ret < 0) || (ret != VFIO_API_VERSION)) { > > + RTE_LOG(ERR, EAL, " unknown VFIO API version! errno > > = %d\n", errno); > > close(vfio_container_fd); > > return -1; > > } > > Not sure how this change improves things, since the existing check will already trigger an error on all values <0. Can you please clarify why you think this needs to be changed? Ah, my bad, the ret < 0 is superfulous, as the != already catches it, but the log message change is valuable in that it differentiates bad API version detection from other system errors. I can respin that if you like. Neil > > > > > /* check if we support IOMMU type 1 */ > > ret = ioctl(vfio_container_fd, VFIO_CHECK_EXTENSION, > > VFIO_TYPE1_IOMMU); > > - if (!ret) { > > - RTE_LOG(ERR, EAL, " unknown IOMMU driver!\n"); > > + if (ret <= 0) { > > + RTE_LOG(ERR, EAL, " unknown IOMMU driver! errno = > > %d\n", errno); > > close(vfio_container_fd); > > return -1; > > } > > Ack on this change part. The previously code was incorrect according to what I read in the docs for VFIO. > > /Bruce > ^ permalink raw reply [flat|nested] 4+ messages in thread
[parent not found: <20140617204011.GH8539-B26myB8xz7F8NnZeBjwnZQMhkBWG/bsMQH7oEaQurus@public.gmane.org>]
* Re: [PATCH] vfio: correct system call error checking [not found] ` <20140617204011.GH8539-B26myB8xz7F8NnZeBjwnZQMhkBWG/bsMQH7oEaQurus@public.gmane.org> @ 2014-06-17 20:43 ` Richardson, Bruce 0 siblings, 0 replies; 4+ messages in thread From: Richardson, Bruce @ 2014-06-17 20:43 UTC (permalink / raw) To: Neil Horman; +Cc: dev-VfR2kkLFssw@public.gmane.org > -----Original Message----- > From: Neil Horman [mailto:nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org] > Sent: Tuesday, June 17, 2014 1:40 PM > To: Richardson, Bruce > Cc: dev-VfR2kkLFssw@public.gmane.org > Subject: Re: [dpdk-dev] [PATCH] vfio: correct system call error checking > > On Tue, Jun 17, 2014 at 08:21:29PM +0000, Richardson, Bruce wrote: > > > -----Original Message----- > > > From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Neil Horman > > > Sent: Tuesday, June 17, 2014 12:04 PM > > > To: dev-VfR2kkLFssw@public.gmane.org > > > Subject: [dpdk-dev] [PATCH] vfio: correct system call error checking > > > > > > Noticed today that ioctl error code return checking was incorrect in some of > the > > > vfio code. ioctl can return a negative value if the system detects an error > > > before the target device/driver can produce a return code. The dpdk vfio > code > > > only checks specfically for the values that it expects, which leaves it open to > > > accepting unexpected error codes as success. For instance, if the vfio layer > > > noted that the iommu driver hadn't finished registering yet, it would return > an > > > -EINVAL error code, but the dpdk would accept that as success, becuase it > > > wasn't > > > 0. > > > > > > Fix this to specifically check for < 0 error codes > > > > > > Signed-off-by: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org> > > > CC: Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org> > > > --- > > > lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 8 ++++---- > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c > > > b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c > > > index 4de6061..65aa8ad 100644 > > > --- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c > > > +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c > > > @@ -319,16 +319,16 @@ pci_vfio_get_container_fd(void) > > > > > > /* check VFIO API version */ > > > ret = ioctl(vfio_container_fd, VFIO_GET_API_VERSION); > > > - if (ret != VFIO_API_VERSION) { > > > - RTE_LOG(ERR, EAL, " unknown VFIO API version!\n"); > > > + if ((ret < 0) || (ret != VFIO_API_VERSION)) { > > > + RTE_LOG(ERR, EAL, " unknown VFIO API version! errno > > > = %d\n", errno); > > > close(vfio_container_fd); > > > return -1; > > > } > > > > Not sure how this change improves things, since the existing check will already > trigger an error on all values <0. Can you please clarify why you think this needs > to be changed? > Ah, my bad, the ret < 0 is superfulous, as the != already catches it, but the > log message change is valuable in that it differentiates bad API version > detection from other system errors. I can respin that if you like. > Neil Perhaps a respin separating out ioctl errors vs version errors might be good, giving different error messages for each case. /Bruce ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-06-17 20:43 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-06-17 19:03 [PATCH] vfio: correct system call error checking Neil Horman [not found] ` <1403031832-28540-1-git-send-email-nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org> 2014-06-17 20:21 ` Richardson, Bruce [not found] ` <59AF69C657FD0841A61C55336867B5B01AA37370-kPTMFJFq+rELt2AQoY/u9bfspsVTdybXVpNB7YpNyf8@public.gmane.org> 2014-06-17 20:40 ` Neil Horman [not found] ` <20140617204011.GH8539-B26myB8xz7F8NnZeBjwnZQMhkBWG/bsMQH7oEaQurus@public.gmane.org> 2014-06-17 20:43 ` Richardson, Bruce
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).