[Qemu-devel] [PATCH] block/raw-posix: Linux compat-ioctl warning workaround

[Qemu-devel] [PATCH] block/raw-posix: Linux compat-ioctl warning workaround

[Johannes Stezenbach]

On Linux x86_64 host with 32bit userspace, running
qemu or even just "qemu-img create -f qcow2 some.img 1G"
causes a kernel warning:

ioctl32(qemu-img:5296): Unknown cmd fd(3) cmd(00005326){t:'S';sz:0} arg(7fffffff) on some.img
ioctl32(qemu-img:5296): Unknown cmd fd(3) cmd(801c0204){t:02;sz:28} arg(fff77350) on some.img

ioctl 00005326 is CDROM_DRIVE_STATUS,
ioctl 801c0204 is FDGETPRM.

The warning appears because the Linux compat-ioctl handler for these
ioctls only applies to block devices, while qemu also uses the ioctls on
plain files.  Work around by calling fstat() the ensure the ioctls are
only used on block devices.

Signed-off-by: Johannes Stezenbach <js@sig21.net>
---
discussed in http://lkml.kernel.org/r/20110617090424.GA19345@sig21.net

I'll also send a Linux kernel patch but this is needed to
fix the warning on old kernels.


diff --git a/block/raw-posix.c b/block/raw-posix.c
index 4cd7d7a..10d56f0 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -46,6 +46,8 @@
 #include <sys/dkio.h>
 #endif
 #ifdef __linux__
+#include <sys/types.h>
+#include <sys/stat.h>
 #include <sys/ioctl.h>
 #include <sys/param.h>
 #include <linux/cdrom.h>
@@ -1188,6 +1190,7 @@ static int floppy_probe_device(const char *filename)
     int fd, ret;
     int prio = 0;
     struct floppy_struct fdparam;
+    struct stat st;
 
     if (strstart(filename, "/dev/fd", NULL))
         prio = 50;
@@ -1196,12 +1199,20 @@ static int floppy_probe_device(const char *filename)
     if (fd < 0) {
         goto out;
     }
+    ret = fstat(fd, &st);
+    if (ret == -1) {
+        goto outc;
+    }
+    if (!S_ISBLK(st.st_mode)) {
+        goto outc;
+    }
 
     /* Attempt to detect via a floppy specific ioctl */
     ret = ioctl(fd, FDGETPRM, &fdparam);
     if (ret >= 0)
         prio = 100;
 
+outc:
     close(fd);
 out:
     return prio;
@@ -1290,17 +1301,26 @@ static int cdrom_probe_device(const char *filename)
 {
     int fd, ret;
     int prio = 0;
+    struct stat st;
 
     fd = open(filename, O_RDONLY | O_NONBLOCK);
     if (fd < 0) {
         goto out;
     }
+    ret = fstat(fd, &st);
+    if (ret == -1) {
+        goto outc;
+    }
+    if (!S_ISBLK(st.st_mode)) {
+        goto outc;
+    }
 
     /* Attempt to detect via a CDROM specific ioctl */
     ret = ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT);
     if (ret >= 0)
         prio = 100;
 
+outc:
     close(fd);
 out:
     return prio;
@@ -1309,11 +1329,15 @@ out:
 static int cdrom_is_inserted(BlockDriverState *bs)
 {
     BDRVRawState *s = bs->opaque;
+    struct stat st;
     int ret;
 
-    ret = ioctl(s->fd, CDROM_DRIVE_STATUS, CDSL_CURRENT);
-    if (ret == CDS_DISC_OK)
-        return 1;
+    ret = fstat(s->fd, &st);
+    if (!ret && S_ISBLK(st.st_mode)) {
+        ret = ioctl(s->fd, CDROM_DRIVE_STATUS, CDSL_CURRENT);
+        if (ret == CDS_DISC_OK)
+            return 1;
+    }
     return 0;
 }
 

Re: [Qemu-devel] [PATCH] block/raw-posix: Linux compat-ioctl warning workaround

[Kevin Wolf]

Am 29.06.2011 15:36, schrieb Johannes Stezenbach:
> On Linux x86_64 host with 32bit userspace, running
> qemu or even just "qemu-img create -f qcow2 some.img 1G"
> causes a kernel warning:
> 
> ioctl32(qemu-img:5296): Unknown cmd fd(3) cmd(00005326){t:'S';sz:0} arg(7fffffff) on some.img
> ioctl32(qemu-img:5296): Unknown cmd fd(3) cmd(801c0204){t:02;sz:28} arg(fff77350) on some.img
> 
> ioctl 00005326 is CDROM_DRIVE_STATUS,
> ioctl 801c0204 is FDGETPRM.
> 
> The warning appears because the Linux compat-ioctl handler for these
> ioctls only applies to block devices, while qemu also uses the ioctls on
> plain files.  Work around by calling fstat() the ensure the ioctls are
> only used on block devices.
> 
> Signed-off-by: Johannes Stezenbach <js@sig21.net>
> ---
> discussed in http://lkml.kernel.org/r/20110617090424.GA19345@sig21.net
> 
> I'll also send a Linux kernel patch but this is needed to
> fix the warning on old kernels.

I probably wouldn't have bothered to work around it for older kernels,
but anyway it shouldn't hurt to do so.

Generally the patch looks good to me, just some minor comments:

> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 4cd7d7a..10d56f0 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -46,6 +46,8 @@
>  #include <sys/dkio.h>
>  #endif
>  #ifdef __linux__
> +#include <sys/types.h>
> +#include <sys/stat.h>
>  #include <sys/ioctl.h>
>  #include <sys/param.h>
>  #include <linux/cdrom.h>
> @@ -1188,6 +1190,7 @@ static int floppy_probe_device(const char *filename)
>      int fd, ret;
>      int prio = 0;
>      struct floppy_struct fdparam;
> +    struct stat st;
>  
>      if (strstart(filename, "/dev/fd", NULL))
>          prio = 50;
> @@ -1196,12 +1199,20 @@ static int floppy_probe_device(const char *filename)
>      if (fd < 0) {
>          goto out;
>      }
> +    ret = fstat(fd, &st);
> +    if (ret == -1) {
> +        goto outc;
> +    }
> +    if (!S_ISBLK(st.st_mode)) {
> +        goto outc;
> +    }

Why not a single if (ret == -1 || !S_ISBLK(st.st_mode))?

>      /* Attempt to detect via a floppy specific ioctl */
>      ret = ioctl(fd, FDGETPRM, &fdparam);
>      if (ret >= 0)
>          prio = 100;
>  
> +outc:
>      close(fd);
>  out:
>      return prio;
> @@ -1290,17 +1301,26 @@ static int cdrom_probe_device(const char *filename)
>  {
>      int fd, ret;
>      int prio = 0;
> +    struct stat st;
>  
>      fd = open(filename, O_RDONLY | O_NONBLOCK);
>      if (fd < 0) {
>          goto out;
>      }
> +    ret = fstat(fd, &st);
> +    if (ret == -1) {
> +        goto outc;
> +    }
> +    if (!S_ISBLK(st.st_mode)) {
> +        goto outc;
> +    }

Same here.

>      /* Attempt to detect via a CDROM specific ioctl */
>      ret = ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT);
>      if (ret >= 0)
>          prio = 100;
>  
> +outc:
>      close(fd);
>  out:
>      return prio;
> @@ -1309,11 +1329,15 @@ out:
>  static int cdrom_is_inserted(BlockDriverState *bs)
>  {
>      BDRVRawState *s = bs->opaque;
> +    struct stat st;
>      int ret;
>  
> -    ret = ioctl(s->fd, CDROM_DRIVE_STATUS, CDSL_CURRENT);
> -    if (ret == CDS_DISC_OK)
> -        return 1;
> +    ret = fstat(s->fd, &st);
> +    if (!ret && S_ISBLK(st.st_mode)) {
> +        ret = ioctl(s->fd, CDROM_DRIVE_STATUS, CDSL_CURRENT);
> +        if (ret == CDS_DISC_OK)
> +            return 1;
> +    }
>      return 0;
>  }

I think this last hunk is unnecessary. cdrom_is_inserted should only be
called if cdrom_probe_device returned success before, so we have already
tested this.

Kevin