[dpdk-dev] [PATCH] vfio: correct system call error checking

[dpdk-dev] [PATCH] vfio: correct system call error checking

[Neil Horman]

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

Re: [dpdk-dev] [PATCH] vfio: correct system call error checking

[Richardson, Bruce]

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

Re: [dpdk-dev] [PATCH] vfio: correct system call error checking

[Neil Horman]

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
> 

Re: [dpdk-dev] [PATCH] vfio: correct system call error checking

[Richardson, Bruce]

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