qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] linux-user: ioctl() command type is int
@ 2015-05-21 22:18 Laurent Vivier
  2015-05-22 12:49 ` Peter Maydell
  2015-05-23 13:17 ` [Qemu-devel] [PATCH v2] " Laurent Vivier
  0 siblings, 2 replies; 8+ messages in thread
From: Laurent Vivier @ 2015-05-21 22:18 UTC (permalink / raw)
  To: Riku Voipio; +Cc: Ed Swierk, qemu-devel, Laurent Vivier

When executing a 64bit target chroot on 64bit host,
the ioctl() command can mismatch.

It seems the previous commit doesn't solve the problem in
my case:

	9c6bf9c7 linux-user: Fix ioctl cmd type mismatch on 64-bit targets

For example, a ppc64 chroot on an x86_64 host:

bash-4.3# ls
Unsupported ioctl: cmd=0x80087467
Unsupported ioctl: cmd=0x802c7415

The origin of the problem is in syscall.c:do_ioctl().

	static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)

In this case (ppc64) abi_long is long (on the x86_64), and

    cmd = 0x0000000080087467

then
	if (ie->target_cmd == cmd)

target_cmd is int, so target_cmd = 0x80087467
and to compare an int with a long, the sign is extended to 64bit,
so the comparison is:

	if (0xffffffff80087467 == 0x0000000080087467)

which doesn't match whereas it should.

This patch uses abi_int in the case of the target command type
instead of abi_long (and for consistency, update IOCTLEntry).

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 linux-user/syscall.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 1622ad6..68801cf 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -3293,8 +3293,8 @@ typedef abi_long do_ioctl_fn(const IOCTLEntry *ie, uint8_t *buf_temp,
                              int fd, abi_long cmd, abi_long arg);
 
 struct IOCTLEntry {
-    int target_cmd;
-    unsigned int host_cmd;
+    abi_int target_cmd;
+    int host_cmd;
     const char *name;
     int access;
     do_ioctl_fn *do_ioctl;
@@ -3849,7 +3849,7 @@ static IOCTLEntry ioctl_entries[] = {
 
 /* ??? Implement proper locking for ioctls.  */
 /* do_ioctl() Must return target values and target errnos. */
-static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)
+static abi_long do_ioctl(int fd, abi_int cmd, abi_long arg)
 {
     const IOCTLEntry *ie;
     const argtype *arg_type;
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH] linux-user: ioctl() command type is int
  2015-05-21 22:18 [Qemu-devel] [PATCH] linux-user: ioctl() command type is int Laurent Vivier
@ 2015-05-22 12:49 ` Peter Maydell
  2015-05-23 13:17 ` [Qemu-devel] [PATCH v2] " Laurent Vivier
  1 sibling, 0 replies; 8+ messages in thread
From: Peter Maydell @ 2015-05-22 12:49 UTC (permalink / raw)
  To: Laurent Vivier; +Cc: Ed Swierk, Riku Voipio, QEMU Developers

On 21 May 2015 at 23:18, Laurent Vivier <laurent@vivier.eu> wrote:
> When executing a 64bit target chroot on 64bit host,
> the ioctl() command can mismatch.
>
> It seems the previous commit doesn't solve the problem in
> my case:
>
>         9c6bf9c7 linux-user: Fix ioctl cmd type mismatch on 64-bit targets
>
> For example, a ppc64 chroot on an x86_64 host:
>
> bash-4.3# ls
> Unsupported ioctl: cmd=0x80087467
> Unsupported ioctl: cmd=0x802c7415
>
> The origin of the problem is in syscall.c:do_ioctl().
>
>         static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)
>
> In this case (ppc64) abi_long is long (on the x86_64), and
>
>     cmd = 0x0000000080087467
>
> then
>         if (ie->target_cmd == cmd)
>
> target_cmd is int, so target_cmd = 0x80087467
> and to compare an int with a long, the sign is extended to 64bit,
> so the comparison is:
>
>         if (0xffffffff80087467 == 0x0000000080087467)
>
> which doesn't match whereas it should.
>
> This patch uses abi_int in the case of the target command type
> instead of abi_long (and for consistency, update IOCTLEntry).
>

abi_int is really only needed in guest-layout structure
declarations, since it has alignment attributes but is
otherwise guaranteed to be an int32_t (and we assume all
over the place that int is 32 bits, so int is ok too).
So it's unnecessary in the function prototypes, and
I think actively harmful in the IOCTLEntry struct (since
that struct is not a shared-with-guest one).

I think also the do_ioctl_fn() prototype and all the
do_ioctl_* functions which are of that type need to have
the cmd parameter switched from abi_long. This avoids
potentially running into the same problem inside a
do_ioctl function if it does comparisons on the cmd.

thanks
-- PMM

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH v2] linux-user: ioctl() command type is int
  2015-05-21 22:18 [Qemu-devel] [PATCH] linux-user: ioctl() command type is int Laurent Vivier
  2015-05-22 12:49 ` Peter Maydell
@ 2015-05-23 13:17 ` Laurent Vivier
  2015-05-23 21:07   ` Peter Maydell
  2015-06-12 13:06   ` Riku Voipio
  1 sibling, 2 replies; 8+ messages in thread
From: Laurent Vivier @ 2015-05-23 13:17 UTC (permalink / raw)
  To: peter.maydell; +Cc: eswierk, riku.voipio, qemu-devel, Laurent Vivier

When executing a 64bit target chroot on 64bit host,
the ioctl() command can mismatch.

It seems the previous commit doesn't solve the problem in
my case:

	9c6bf9c7 linux-user: Fix ioctl cmd type mismatch on 64-bit targets

For example, a ppc64 chroot on an x86_64 host:

bash-4.3# ls
Unsupported ioctl: cmd=0x80087467
Unsupported ioctl: cmd=0x802c7415

The origin of the problem is in syscall.c:do_ioctl().

	static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)

In this case (ppc64) abi_long is long (on the x86_64), and

    cmd = 0x0000000080087467

then
	if (ie->target_cmd == cmd)

target_cmd is int, so target_cmd = 0x80087467
and to compare an int with a long, the sign is extended to 64bit,
so the comparison is:

	if (0xffffffff80087467 == 0x0000000080087467)

which doesn't match whereas it should.

This patch uses int in the case of the target command type
instead of abi_long (and for consistency, update IOCTLEntry).

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
v2: use int instead of abi_int, as it is recommended by Peter Maydell.

 linux-user/syscall.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 1622ad6..c28cd05 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -3290,11 +3290,11 @@ enum {
 typedef struct IOCTLEntry IOCTLEntry;
 
 typedef abi_long do_ioctl_fn(const IOCTLEntry *ie, uint8_t *buf_temp,
-                             int fd, abi_long cmd, abi_long arg);
+                             int fd, int cmd, abi_long arg);
 
 struct IOCTLEntry {
     int target_cmd;
-    unsigned int host_cmd;
+    int host_cmd;
     const char *name;
     int access;
     do_ioctl_fn *do_ioctl;
@@ -3316,7 +3316,7 @@ struct IOCTLEntry {
                             / sizeof(struct fiemap_extent))
 
 static abi_long do_ioctl_fs_ioc_fiemap(const IOCTLEntry *ie, uint8_t *buf_temp,
-                                       int fd, abi_long cmd, abi_long arg)
+                                       int fd, int cmd, abi_long arg)
 {
     /* The parameter for this ioctl is a struct fiemap followed
      * by an array of struct fiemap_extent whose size is set
@@ -3397,7 +3397,7 @@ static abi_long do_ioctl_fs_ioc_fiemap(const IOCTLEntry *ie, uint8_t *buf_temp,
 #endif
 
 static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
-                                int fd, abi_long cmd, abi_long arg)
+                                int fd, int cmd, abi_long arg)
 {
     const argtype *arg_type = ie->arg_type;
     int target_size;
@@ -3491,7 +3491,7 @@ static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
 }
 
 static abi_long do_ioctl_dm(const IOCTLEntry *ie, uint8_t *buf_temp, int fd,
-                            abi_long cmd, abi_long arg)
+                            int cmd, abi_long arg)
 {
     void *argptr;
     struct dm_ioctl *host_dm;
@@ -3716,7 +3716,7 @@ out:
 }
 
 static abi_long do_ioctl_blkpg(const IOCTLEntry *ie, uint8_t *buf_temp, int fd,
-                               abi_long cmd, abi_long arg)
+                               int cmd, abi_long arg)
 {
     void *argptr;
     int target_size;
@@ -3769,7 +3769,7 @@ out:
 }
 
 static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
-                                int fd, abi_long cmd, abi_long arg)
+                                int fd, int cmd, abi_long arg)
 {
     const argtype *arg_type = ie->arg_type;
     const StructEntry *se;
@@ -3832,7 +3832,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
 }
 
 static abi_long do_ioctl_kdsigaccept(const IOCTLEntry *ie, uint8_t *buf_temp,
-                                     int fd, abi_long cmd, abi_long arg)
+                                     int fd, int cmd, abi_long arg)
 {
     int sig = target_to_host_signal(arg);
     return get_errno(ioctl(fd, ie->host_cmd, sig));
@@ -3849,7 +3849,7 @@ static IOCTLEntry ioctl_entries[] = {
 
 /* ??? Implement proper locking for ioctls.  */
 /* do_ioctl() Must return target values and target errnos. */
-static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)
+static abi_long do_ioctl(int fd, int cmd, abi_long arg)
 {
     const IOCTLEntry *ie;
     const argtype *arg_type;
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v2] linux-user: ioctl() command type is int
  2015-05-23 13:17 ` [Qemu-devel] [PATCH v2] " Laurent Vivier
@ 2015-05-23 21:07   ` Peter Maydell
  2015-06-12 13:06   ` Riku Voipio
  1 sibling, 0 replies; 8+ messages in thread
From: Peter Maydell @ 2015-05-23 21:07 UTC (permalink / raw)
  To: Laurent Vivier; +Cc: eswierk, Riku Voipio, QEMU Developers

On 23 May 2015 at 14:17, Laurent Vivier <laurent@vivier.eu> wrote:
> When executing a 64bit target chroot on 64bit host,
> the ioctl() command can mismatch.
>
> It seems the previous commit doesn't solve the problem in
> my case:
>
>         9c6bf9c7 linux-user: Fix ioctl cmd type mismatch on 64-bit targets
>
> For example, a ppc64 chroot on an x86_64 host:
>
> bash-4.3# ls
> Unsupported ioctl: cmd=0x80087467
> Unsupported ioctl: cmd=0x802c7415
>
> The origin of the problem is in syscall.c:do_ioctl().
>
>         static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)
>
> In this case (ppc64) abi_long is long (on the x86_64), and
>
>     cmd = 0x0000000080087467
>
> then
>         if (ie->target_cmd == cmd)
>
> target_cmd is int, so target_cmd = 0x80087467
> and to compare an int with a long, the sign is extended to 64bit,
> so the comparison is:
>
>         if (0xffffffff80087467 == 0x0000000080087467)
>
> which doesn't match whereas it should.
>
> This patch uses int in the case of the target command type
> instead of abi_long (and for consistency, update IOCTLEntry).
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v2] linux-user: ioctl() command type is int
  2015-05-23 13:17 ` [Qemu-devel] [PATCH v2] " Laurent Vivier
  2015-05-23 21:07   ` Peter Maydell
@ 2015-06-12 13:06   ` Riku Voipio
  1 sibling, 0 replies; 8+ messages in thread
From: Riku Voipio @ 2015-06-12 13:06 UTC (permalink / raw)
  To: Laurent Vivier; +Cc: peter.maydell, eswierk, qemu-devel

On Saturday, May 23, 2015 4:17:05 PM EEST, Laurent Vivier wrote:
> When executing a 64bit target chroot on 64bit host,
> the ioctl() command can mismatch.
>
> It seems the previous commit doesn't solve the problem in
> my case:
>
> 	9c6bf9c7 linux-user: Fix ioctl cmd type mismatch on 64-bit targets
>
> For example, a ppc64 chroot on an x86_64 host:
>
> bash-4.3# ls
> Unsupported ioctl: cmd=0x80087467
> Unsupported ioctl: cmd=0x802c7415
>
> The origin of the problem is in syscall.c:do_ioctl().
>
> 	static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)
>
> In this case (ppc64) abi_long is long (on the x86_64), and
>
>     cmd = 0x0000000080087467
>
> then
> 	if (ie->target_cmd == cmd)
>
> target_cmd is int, so target_cmd = 0x80087467
> and to compare an int with a long, the sign is extended to 64bit,
> so the comparison is:
>
> 	if (0xffffffff80087467 == 0x0000000080087467)
>
> which doesn't match whereas it should.
>
> This patch uses int in the case of the target command type
> instead of abi_long (and for consistency, update IOCTLEntry).
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>

Applied to linux-user que,

Thanks

> ---
> v2: use int instead of abi_int, as it is recommended by Peter Maydell.
>
>  linux-user/syscall.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 1622ad6..c28cd05 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -3290,11 +3290,11 @@ enum {
>  typedef struct IOCTLEntry IOCTLEntry;
>  
>  typedef abi_long do_ioctl_fn(const IOCTLEntry *ie, uint8_t *buf_temp,
> -                             int fd, abi_long cmd, abi_long arg);
> +                             int fd, int cmd, abi_long arg);
>  
>  struct IOCTLEntry {
>      int target_cmd;
> -    unsigned int host_cmd;
> +    int host_cmd;
>      const char *name;
>      int access;
>      do_ioctl_fn *do_ioctl;
> @@ -3316,7 +3316,7 @@ struct IOCTLEntry {
>                              / sizeof(struct fiemap_extent))
>  
>  static abi_long do_ioctl_fs_ioc_fiemap(const IOCTLEntry *ie, 
> uint8_t *buf_temp,
> -                                       int fd, abi_long cmd, abi_long arg)
> +                                       int fd, int cmd, abi_long arg)
>  {
>      /* The parameter for this ioctl is a struct fiemap followed
>       * by an array of struct fiemap_extent whose size is set
> @@ -3397,7 +3397,7 @@ static abi_long 
> do_ioctl_fs_ioc_fiemap(const IOCTLEntry *ie, uint8_t *buf_temp,
>  #endif
>  
>  static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
> -                                int fd, abi_long cmd, abi_long arg)
> +                                int fd, int cmd, abi_long arg)
>  {
>      const argtype *arg_type = ie->arg_type;
>      int target_size;
> @@ -3491,7 +3491,7 @@ static abi_long do_ioctl_ifconf(const 
> IOCTLEntry *ie, uint8_t *buf_temp,
>  }
>  
>  static abi_long do_ioctl_dm(const IOCTLEntry *ie, uint8_t 
> *buf_temp, int fd,
> -                            abi_long cmd, abi_long arg)
> +                            int cmd, abi_long arg)
>  {
>      void *argptr;
>      struct dm_ioctl *host_dm;
> @@ -3716,7 +3716,7 @@ out:
>  }
>  
>  static abi_long do_ioctl_blkpg(const IOCTLEntry *ie, uint8_t 
> *buf_temp, int fd,
> -                               abi_long cmd, abi_long arg)
> +                               int cmd, abi_long arg)
>  {
>      void *argptr;
>      int target_size;
> @@ -3769,7 +3769,7 @@ out:
>  }
>  
>  static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
> -                                int fd, abi_long cmd, abi_long arg)
> +                                int fd, int cmd, abi_long arg)
>  {
>      const argtype *arg_type = ie->arg_type;
>      const StructEntry *se;
> @@ -3832,7 +3832,7 @@ static abi_long do_ioctl_rt(const 
> IOCTLEntry *ie, uint8_t *buf_temp,
>  }
>  
>  static abi_long do_ioctl_kdsigaccept(const IOCTLEntry *ie, 
> uint8_t *buf_temp,
> -                                     int fd, abi_long cmd, abi_long arg)
> +                                     int fd, int cmd, abi_long arg)
>  {
>      int sig = target_to_host_signal(arg);
>      return get_errno(ioctl(fd, ie->host_cmd, sig));
> @@ -3849,7 +3849,7 @@ static IOCTLEntry ioctl_entries[] = {
>  
>  /* ??? Implement proper locking for ioctls.  */
>  /* do_ioctl() Must return target values and target errnos. */
> -static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)
> +static abi_long do_ioctl(int fd, int cmd, abi_long arg)
>  {
>      const IOCTLEntry *ie;
>      const argtype *arg_type;

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH v2] linux-user: ioctl() command type is int
  2015-06-15 15:14 [Qemu-devel] [PULL 0/6] linux-user patches for 2.4 softfreeze Peter Maydell
@ 2015-06-15 22:35 ` Laurent Vivier
  2015-06-15 22:46   ` Eric Blake
  2015-06-16  6:57   ` Riku Voipio
  0 siblings, 2 replies; 8+ messages in thread
From: Laurent Vivier @ 2015-06-15 22:35 UTC (permalink / raw)
  To: peter.maydell, riku.voipio; +Cc: qemu-devel, Laurent Vivier

When executing a 64bit target chroot on 64bit host,
the ioctl() command can mismatch.

It seems the previous commit doesn't solve the problem in
my case:

	9c6bf9c7 linux-user: Fix ioctl cmd type mismatch on 64-bit targets

For example, a ppc64 chroot on an x86_64 host:

bash-4.3# ls
Unsupported ioctl: cmd=0x80087467
Unsupported ioctl: cmd=0x802c7415

The origin of the problem is in syscall.c:do_ioctl().

	static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)

In this case (ppc64) abi_long is long (on the x86_64), and

    cmd = 0x0000000080087467

then
	if (ie->target_cmd == cmd)

target_cmd is int, so target_cmd = 0x80087467
and to compare an int with a long, the sign is extended to 64bit,
so the comparison is:

	if (0xffffffff80087467 == 0x0000000080087467)

which doesn't match whereas it should.

This patch uses int in the case of the target command type
instead of abi_long.

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
v2: Don't modify IOCTLEntry type (useless and introduce clang errors)
 linux-user/syscall.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index b98b7e7..5a280c3 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -3645,7 +3645,7 @@ enum {
 typedef struct IOCTLEntry IOCTLEntry;
 
 typedef abi_long do_ioctl_fn(const IOCTLEntry *ie, uint8_t *buf_temp,
-                             int fd, abi_long cmd, abi_long arg);
+                             int fd, int cmd, abi_long arg);
 
 struct IOCTLEntry {
     int target_cmd;
@@ -3671,7 +3671,7 @@ struct IOCTLEntry {
                             / sizeof(struct fiemap_extent))
 
 static abi_long do_ioctl_fs_ioc_fiemap(const IOCTLEntry *ie, uint8_t *buf_temp,
-                                       int fd, abi_long cmd, abi_long arg)
+                                       int fd, int cmd, abi_long arg)
 {
     /* The parameter for this ioctl is a struct fiemap followed
      * by an array of struct fiemap_extent whose size is set
@@ -3752,7 +3752,7 @@ static abi_long do_ioctl_fs_ioc_fiemap(const IOCTLEntry *ie, uint8_t *buf_temp,
 #endif
 
 static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
-                                int fd, abi_long cmd, abi_long arg)
+                                int fd, int cmd, abi_long arg)
 {
     const argtype *arg_type = ie->arg_type;
     int target_size;
@@ -3846,7 +3846,7 @@ static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
 }
 
 static abi_long do_ioctl_dm(const IOCTLEntry *ie, uint8_t *buf_temp, int fd,
-                            abi_long cmd, abi_long arg)
+                            int cmd, abi_long arg)
 {
     void *argptr;
     struct dm_ioctl *host_dm;
@@ -4071,7 +4071,7 @@ out:
 }
 
 static abi_long do_ioctl_blkpg(const IOCTLEntry *ie, uint8_t *buf_temp, int fd,
-                               abi_long cmd, abi_long arg)
+                               int cmd, abi_long arg)
 {
     void *argptr;
     int target_size;
@@ -4124,7 +4124,7 @@ out:
 }
 
 static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
-                                int fd, abi_long cmd, abi_long arg)
+                                int fd, int cmd, abi_long arg)
 {
     const argtype *arg_type = ie->arg_type;
     const StructEntry *se;
@@ -4187,7 +4187,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
 }
 
 static abi_long do_ioctl_kdsigaccept(const IOCTLEntry *ie, uint8_t *buf_temp,
-                                     int fd, abi_long cmd, abi_long arg)
+                                     int fd, int cmd, abi_long arg)
 {
     int sig = target_to_host_signal(arg);
     return get_errno(ioctl(fd, ie->host_cmd, sig));
@@ -4204,7 +4204,7 @@ static IOCTLEntry ioctl_entries[] = {
 
 /* ??? Implement proper locking for ioctls.  */
 /* do_ioctl() Must return target values and target errnos. */
-static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)
+static abi_long do_ioctl(int fd, int cmd, abi_long arg)
 {
     const IOCTLEntry *ie;
     const argtype *arg_type;
-- 
2.4.3

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v2] linux-user: ioctl() command type is int
  2015-06-15 22:35 ` [Qemu-devel] [PATCH v2] linux-user: ioctl() command type is int Laurent Vivier
@ 2015-06-15 22:46   ` Eric Blake
  2015-06-16  6:57   ` Riku Voipio
  1 sibling, 0 replies; 8+ messages in thread
From: Eric Blake @ 2015-06-15 22:46 UTC (permalink / raw)
  To: Laurent Vivier, peter.maydell, riku.voipio; +Cc: qemu-devel

[-- Attachment #1: Type: text/plain, Size: 1295 bytes --]

On 06/15/2015 04:35 PM, Laurent Vivier wrote:
> When executing a 64bit target chroot on 64bit host,
> the ioctl() command can mismatch.
> 

> 
> The origin of the problem is in syscall.c:do_ioctl().
> 
> 	static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)

It's annoying that Linux picked ioctl(int, unsigned long request, ...),
particularly since POSIX picked ioctl(int, int request, ...) [1] and
therefore Linux is constrained to never accept a 'request' that doesn't
fit in 32 bits.  Especially so since the POSIX definition of ioctl()
applies only to the obsolete STREAMS interface that Linux never really
picked up on. (The gnulib project has determined ways to write an
ioctl() wrapper that always takes an int request, then widens to long as
necessary before calling the real syscall, with no ill effects [2])

[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/ioctl.html
[2] http://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/ioctl.c

However, I don't feel comfortable enough with this code to give a
competent review, only to offer up that bit of trivia and the vague
impression that it looks like you are safe in this patch.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v2] linux-user: ioctl() command type is int
  2015-06-15 22:35 ` [Qemu-devel] [PATCH v2] linux-user: ioctl() command type is int Laurent Vivier
  2015-06-15 22:46   ` Eric Blake
@ 2015-06-16  6:57   ` Riku Voipio
  1 sibling, 0 replies; 8+ messages in thread
From: Riku Voipio @ 2015-06-16  6:57 UTC (permalink / raw)
  To: Laurent Vivier; +Cc: Peter Maydell, qemu-devel qemu-devel

On 16 June 2015 at 01:35, Laurent Vivier <laurent@vivier.eu> wrote:
> When executing a 64bit target chroot on 64bit host,
> the ioctl() command can mismatch.
>
> It seems the previous commit doesn't solve the problem in
> my case:
>
>         9c6bf9c7 linux-user: Fix ioctl cmd type mismatch on 64-bit targets
>
> For example, a ppc64 chroot on an x86_64 host:
>
> bash-4.3# ls
> Unsupported ioctl: cmd=0x80087467
> Unsupported ioctl: cmd=0x802c7415
>
> The origin of the problem is in syscall.c:do_ioctl().
>
>         static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)
>
> In this case (ppc64) abi_long is long (on the x86_64), and
>
>     cmd = 0x0000000080087467
>
> then
>         if (ie->target_cmd == cmd)
>
> target_cmd is int, so target_cmd = 0x80087467
> and to compare an int with a long, the sign is extended to 64bit,
> so the comparison is:
>
>         if (0xffffffff80087467 == 0x0000000080087467)
>
> which doesn't match whereas it should.
>
> This patch uses int in the case of the target command type
> instead of abi_long.
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
> v2: Don't modify IOCTLEntry type (useless and introduce clang errors)

Thanks, tested to build with clang, will refresh pull request in a moment.

>  linux-user/syscall.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index b98b7e7..5a280c3 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -3645,7 +3645,7 @@ enum {
>  typedef struct IOCTLEntry IOCTLEntry;
>
>  typedef abi_long do_ioctl_fn(const IOCTLEntry *ie, uint8_t *buf_temp,
> -                             int fd, abi_long cmd, abi_long arg);
> +                             int fd, int cmd, abi_long arg);
>
>  struct IOCTLEntry {
>      int target_cmd;
> @@ -3671,7 +3671,7 @@ struct IOCTLEntry {
>                              / sizeof(struct fiemap_extent))
>
>  static abi_long do_ioctl_fs_ioc_fiemap(const IOCTLEntry *ie, uint8_t *buf_temp,
> -                                       int fd, abi_long cmd, abi_long arg)
> +                                       int fd, int cmd, abi_long arg)
>  {
>      /* The parameter for this ioctl is a struct fiemap followed
>       * by an array of struct fiemap_extent whose size is set
> @@ -3752,7 +3752,7 @@ static abi_long do_ioctl_fs_ioc_fiemap(const IOCTLEntry *ie, uint8_t *buf_temp,
>  #endif
>
>  static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
> -                                int fd, abi_long cmd, abi_long arg)
> +                                int fd, int cmd, abi_long arg)
>  {
>      const argtype *arg_type = ie->arg_type;
>      int target_size;
> @@ -3846,7 +3846,7 @@ static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
>  }
>
>  static abi_long do_ioctl_dm(const IOCTLEntry *ie, uint8_t *buf_temp, int fd,
> -                            abi_long cmd, abi_long arg)
> +                            int cmd, abi_long arg)
>  {
>      void *argptr;
>      struct dm_ioctl *host_dm;
> @@ -4071,7 +4071,7 @@ out:
>  }
>
>  static abi_long do_ioctl_blkpg(const IOCTLEntry *ie, uint8_t *buf_temp, int fd,
> -                               abi_long cmd, abi_long arg)
> +                               int cmd, abi_long arg)
>  {
>      void *argptr;
>      int target_size;
> @@ -4124,7 +4124,7 @@ out:
>  }
>
>  static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
> -                                int fd, abi_long cmd, abi_long arg)
> +                                int fd, int cmd, abi_long arg)
>  {
>      const argtype *arg_type = ie->arg_type;
>      const StructEntry *se;
> @@ -4187,7 +4187,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
>  }
>
>  static abi_long do_ioctl_kdsigaccept(const IOCTLEntry *ie, uint8_t *buf_temp,
> -                                     int fd, abi_long cmd, abi_long arg)
> +                                     int fd, int cmd, abi_long arg)
>  {
>      int sig = target_to_host_signal(arg);
>      return get_errno(ioctl(fd, ie->host_cmd, sig));
> @@ -4204,7 +4204,7 @@ static IOCTLEntry ioctl_entries[] = {
>
>  /* ??? Implement proper locking for ioctls.  */
>  /* do_ioctl() Must return target values and target errnos. */
> -static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)
> +static abi_long do_ioctl(int fd, int cmd, abi_long arg)
>  {
>      const IOCTLEntry *ie;
>      const argtype *arg_type;
> --
> 2.4.3
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2015-06-16  6:57 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-05-21 22:18 [Qemu-devel] [PATCH] linux-user: ioctl() command type is int Laurent Vivier
2015-05-22 12:49 ` Peter Maydell
2015-05-23 13:17 ` [Qemu-devel] [PATCH v2] " Laurent Vivier
2015-05-23 21:07   ` Peter Maydell
2015-06-12 13:06   ` Riku Voipio
  -- strict thread matches above, loose matches on Subject: below --
2015-06-15 15:14 [Qemu-devel] [PULL 0/6] linux-user patches for 2.4 softfreeze Peter Maydell
2015-06-15 22:35 ` [Qemu-devel] [PATCH v2] linux-user: ioctl() command type is int Laurent Vivier
2015-06-15 22:46   ` Eric Blake
2015-06-16  6:57   ` Riku Voipio

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