* [dpdk-dev] [PATCH] vfio: correct system call error checking
@ 2014-06-17 19:03 Neil Horman
2014-06-17 20:21 ` Richardson, Bruce
0 siblings, 1 reply; 4+ messages in thread
From: Neil Horman @ 2014-06-17 19:03 UTC (permalink / raw)
To: dev
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@tuxdriver.com>
CC: Thomas Monjalon <thomas.monjalon@6wind.com>
---
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 [flat|nested] 4+ messages in thread* Re: [dpdk-dev] [PATCH] vfio: correct system call error checking
2014-06-17 19:03 [dpdk-dev] [PATCH] vfio: correct system call error checking Neil Horman
@ 2014-06-17 20:21 ` Richardson, Bruce
2014-06-17 20:40 ` Neil Horman
0 siblings, 1 reply; 4+ messages in thread
From: Richardson, Bruce @ 2014-06-17 20:21 UTC (permalink / raw)
To: Neil Horman, dev
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Neil Horman
> Sent: Tuesday, June 17, 2014 12:04 PM
> To: dev@dpdk.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@tuxdriver.com>
> CC: Thomas Monjalon <thomas.monjalon@6wind.com>
> ---
> 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* Re: [dpdk-dev] [PATCH] vfio: correct system call error checking
2014-06-17 20:21 ` Richardson, Bruce
@ 2014-06-17 20:40 ` Neil Horman
2014-06-17 20:43 ` Richardson, Bruce
0 siblings, 1 reply; 4+ messages in thread
From: Neil Horman @ 2014-06-17 20:40 UTC (permalink / raw)
To: Richardson, Bruce; +Cc: dev
On Tue, Jun 17, 2014 at 08:21:29PM +0000, Richardson, Bruce wrote:
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Neil Horman
> > Sent: Tuesday, June 17, 2014 12:04 PM
> > To: dev@dpdk.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@tuxdriver.com>
> > CC: Thomas Monjalon <thomas.monjalon@6wind.com>
> > ---
> > 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* Re: [dpdk-dev] [PATCH] vfio: correct system call error checking
2014-06-17 20:40 ` Neil Horman
@ 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
> -----Original Message-----
> From: Neil Horman [mailto:nhorman@tuxdriver.com]
> Sent: Tuesday, June 17, 2014 1:40 PM
> To: Richardson, Bruce
> Cc: dev@dpdk.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@dpdk.org] On Behalf Of Neil Horman
> > > Sent: Tuesday, June 17, 2014 12:04 PM
> > > To: dev@dpdk.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@tuxdriver.com>
> > > CC: Thomas Monjalon <thomas.monjalon@6wind.com>
> > > ---
> > > 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:42 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 [dpdk-dev] [PATCH] vfio: correct system call error checking Neil Horman
2014-06-17 20:21 ` Richardson, Bruce
2014-06-17 20:40 ` Neil Horman
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).