[Qemu-devel] [PATCH 0/2] fix incorrect target ioctl numbers

[Qemu-devel] [PATCH 0/2] fix incorrect target ioctl numbers

[Peter Maydell]

Some of our TARGET_* constant definitions for ioctls were
wrong because the ioctl number is based on the sizeof()
the type passed to the TARGET_IO* macros, and we were
passing a host type rather than a target type.
This was originally reported as a bug where the
FS_IOC_{GET,SET}FLAGS ioctls weren't working for 32-bit
arm guests on x86-64 hosts.

I did a quick audit of all the uses of the TARGET_IO* macros
in syscall_defs.h, and:
 * FS_IOC_GETFLAGS/SETFLAGS are indeed wrong
 * 3 ioctls to do with magtapes are also wrong
 * TARGET_FS_IOC_FIEMAP, TARGET_FICLONERANGE and
   TARGET_SOUND_MIXER_INFO take a host struct which
   is defined such that it's the same size for all archs
 * lots and lots of ioctls use 'int', which is OK as for
   us 'abi_int' is always 32 bits (it might have different
   alignment requirements, but they don't matter for this
   purpose)
 * TARGET_SIOCPGRP takes a pid_t, which is always 'int'

This patchset fixes the bugs in the first two bullet
points, and leaves everything else alone since it doesn't
actually manifest as wrong behaviour.

Patch 1 is tested by the LTP 'setxattrs3' test case.
There's no LTP test case for the magtape ioctls, so that
patch change is untested.


Peter Maydell (2):
  linux-user: Fix target FS_IOC_GETFLAGS and FS_IOC_SETFLAGS numbers
  linux-user: Fix TARGET_MTIOCTOP/MTIOCGET/MTIOCPOS values

 linux-user/syscall_defs.h | 35 ++++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] linux-user: Fix target FS_IOC_GETFLAGS and FS_IOC_SETFLAGS numbers

[Peter Maydell]

We were defining TARGET_FS_IOC_GETFLAGS and TARGET_FS_IOC_SETFLAGS
using the host 'long' type in the size field, which meant that
they had the wrong values if the host and guest had different
sized longs. Switch to abi_long instead.

This fixes a bug where these ioctls don't work on 32-bit guests
on 64-bit hosts (and makes the LTP test 'setxattr03' pass
where it did not previously.)

Reported-by: pgndev <pgnet.dev@gmail.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/syscall_defs.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 40c5027..f7cc9f9 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -1101,8 +1101,8 @@ struct target_pollfd {
 /* Note that the ioctl numbers claim type "long" but the actual type
  * used by the kernel is "int".
  */
-#define TARGET_FS_IOC_GETFLAGS TARGET_IOR('f', 1, long)
-#define TARGET_FS_IOC_SETFLAGS TARGET_IOW('f', 2, long)
+#define TARGET_FS_IOC_GETFLAGS TARGET_IOR('f', 1, abi_long)
+#define TARGET_FS_IOC_SETFLAGS TARGET_IOW('f', 2, abi_long)
 
 #define TARGET_FS_IOC_FIEMAP TARGET_IOWR('f',11,struct fiemap)
 
-- 
2.7.4

[Qemu-devel] [PATCH 2/2] linux-user: Fix TARGET_MTIOCTOP/MTIOCGET/MTIOCPOS values

[Peter Maydell]

The TARGET_MTIOCTOP/TARGET_MTIOCGET/TARGET_MTIOCPOS values
were being defined in terms of host struct types, but
these structures are such that their size might differ
on different hosts. Switch to using a target struct
definition instead.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/syscall_defs.h | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index f7cc9f9..16d56fa 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -2706,9 +2706,34 @@ struct target_f_owner_ex {
 #define TARGET_VFAT_IOCTL_READDIR_BOTH    TARGET_IORU('r', 1)
 #define TARGET_VFAT_IOCTL_READDIR_SHORT   TARGET_IORU('r', 2)
 
-#define TARGET_MTIOCTOP        TARGET_IOW('m', 1, struct mtop)
-#define TARGET_MTIOCGET        TARGET_IOR('m', 2, struct mtget)
-#define TARGET_MTIOCPOS        TARGET_IOR('m', 3, struct mtpos)
+struct target_mtop {
+    abi_short mt_op;
+    abi_int mt_count;
+};
+
+#if defined(TARGET_SPARC) || defined(TARGET_MIPS)
+typedef abi_long target_kernel_daddr_t;
+#else
+typedef abi_int target_kernel_daddr_t;
+#endif
+
+struct target_mtget {
+    abi_long mt_type;
+    abi_long mt_resid;
+    abi_long mt_dsreg;
+    abi_long mt_gstat;
+    abi_long mt_erreg;
+    target_kernel_daddr_t mt_fileno;
+    target_kernel_daddr_t mt_blkno;
+};
+
+struct target_mtpos {
+    abi_long mt_blkno;
+};
+
+#define TARGET_MTIOCTOP        TARGET_IOW('m', 1, struct target_mtop)
+#define TARGET_MTIOCGET        TARGET_IOR('m', 2, struct target_mtget)
+#define TARGET_MTIOCPOS        TARGET_IOR('m', 3, struct target_mtpos)
 
 struct target_sysinfo {
     abi_long uptime;                /* Seconds since boot */
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/2] linux-user: Fix target FS_IOC_GETFLAGS and FS_IOC_SETFLAGS numbers

[Laurent Vivier]

Le 12/10/2017 à 17:30, Peter Maydell a écrit :
> We were defining TARGET_FS_IOC_GETFLAGS and TARGET_FS_IOC_SETFLAGS
> using the host 'long' type in the size field, which meant that
> they had the wrong values if the host and guest had different
> sized longs. Switch to abi_long instead.
> 
> This fixes a bug where these ioctls don't work on 32-bit guests
> on 64-bit hosts (and makes the LTP test 'setxattr03' pass
> where it did not previously.)
> 
> Reported-by: pgndev <pgnet.dev@gmail.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/syscall_defs.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index 40c5027..f7cc9f9 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -1101,8 +1101,8 @@ struct target_pollfd {
>  /* Note that the ioctl numbers claim type "long" but the actual type
>   * used by the kernel is "int".
>   */
> -#define TARGET_FS_IOC_GETFLAGS TARGET_IOR('f', 1, long)
> -#define TARGET_FS_IOC_SETFLAGS TARGET_IOW('f', 2, long)
> +#define TARGET_FS_IOC_GETFLAGS TARGET_IOR('f', 1, abi_long)
> +#define TARGET_FS_IOC_SETFLAGS TARGET_IOW('f', 2, abi_long)
>  
>  #define TARGET_FS_IOC_FIEMAP TARGET_IOWR('f',11,struct fiemap)
>  
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [Qemu-devel] [PATCH 2/2] linux-user: Fix TARGET_MTIOCTOP/MTIOCGET/MTIOCPOS values

[Laurent Vivier]

Le 12/10/2017 à 17:30, Peter Maydell a écrit :
> The TARGET_MTIOCTOP/TARGET_MTIOCGET/TARGET_MTIOCPOS values
> were being defined in terms of host struct types, but
> these structures are such that their size might differ
> on different hosts. Switch to using a target struct
> definition instead.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/syscall_defs.h | 31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index f7cc9f9..16d56fa 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -2706,9 +2706,34 @@ struct target_f_owner_ex {
>  #define TARGET_VFAT_IOCTL_READDIR_BOTH    TARGET_IORU('r', 1)
>  #define TARGET_VFAT_IOCTL_READDIR_SHORT   TARGET_IORU('r', 2)
>  
> -#define TARGET_MTIOCTOP        TARGET_IOW('m', 1, struct mtop)
> -#define TARGET_MTIOCGET        TARGET_IOR('m', 2, struct mtget)
> -#define TARGET_MTIOCPOS        TARGET_IOR('m', 3, struct mtpos)
> +struct target_mtop {
> +    abi_short mt_op;
> +    abi_int mt_count;
> +};
> +
> +#if defined(TARGET_SPARC) || defined(TARGET_MIPS)
> +typedef abi_long target_kernel_daddr_t;
> +#else
> +typedef abi_int target_kernel_daddr_t;
> +#endif

Perhaps you can add these ones into include/exec/user/abitypes.h ?

> +struct target_mtget {
> +    abi_long mt_type;
> +    abi_long mt_resid;
> +    abi_long mt_dsreg;
> +    abi_long mt_gstat;
> +    abi_long mt_erreg;
> +    target_kernel_daddr_t mt_fileno;
> +    target_kernel_daddr_t mt_blkno;
> +};

I think you need to update STRUCT(mtget, ...) in
linux-user/syscall_types.h to reflect the size difference for MIPS and
SPARC.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 2/2] linux-user: Fix TARGET_MTIOCTOP/MTIOCGET/MTIOCPOS values

[Peter Maydell]

On 12 October 2017 at 17:49, Laurent Vivier <laurent@vivier.eu> wrote:
> Le 12/10/2017 à 17:30, Peter Maydell a écrit :
>> +#if defined(TARGET_SPARC) || defined(TARGET_MIPS)
>> +typedef abi_long target_kernel_daddr_t;
>> +#else
>> +typedef abi_int target_kernel_daddr_t;
>> +#endif
>
> Perhaps you can add these ones into include/exec/user/abitypes.h ?

I don't think they belong there -- that file is for basic
CPU ABI dependent types, not things which are just part of
the kernel interface.

>> +struct target_mtget {
>> +    abi_long mt_type;
>> +    abi_long mt_resid;
>> +    abi_long mt_dsreg;
>> +    abi_long mt_gstat;
>> +    abi_long mt_erreg;
>> +    target_kernel_daddr_t mt_fileno;
>> +    target_kernel_daddr_t mt_blkno;
>> +};
>
> I think you need to update STRUCT(mtget, ...) in
> linux-user/syscall_types.h to reflect the size difference for MIPS and
> SPARC.

I thought about that but I wasn't feeling too enthusiastic
due to not having a test case... I suppose it's better to
change them both though.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] linux-user: Fix TARGET_MTIOCTOP/MTIOCGET/MTIOCPOS values

[Laurent Vivier]

Le 12/10/2017 à 18:53, Peter Maydell a écrit :
> On 12 October 2017 at 17:49, Laurent Vivier <laurent@vivier.eu> wrote:
>> Le 12/10/2017 à 17:30, Peter Maydell a écrit :
>>> +#if defined(TARGET_SPARC) || defined(TARGET_MIPS)
>>> +typedef abi_long target_kernel_daddr_t;
>>> +#else
>>> +typedef abi_int target_kernel_daddr_t;
>>> +#endif
>>
>> Perhaps you can add these ones into include/exec/user/abitypes.h ?
> 
> I don't think they belong there -- that file is for basic
> CPU ABI dependent types, not things which are just part of
> the kernel interface.

I agree

Laurent

Re: [Qemu-devel] [PATCH 1/2] linux-user: Fix target FS_IOC_GETFLAGS and FS_IOC_SETFLAGS numbers

[Riku Voipio]

On Thu, Oct 12, 2017 at 04:30:44PM +0100, Peter Maydell wrote:
> We were defining TARGET_FS_IOC_GETFLAGS and TARGET_FS_IOC_SETFLAGS
> using the host 'long' type in the size field, which meant that
> they had the wrong values if the host and guest had different
> sized longs. Switch to abi_long instead.
> 
> This fixes a bug where these ioctls don't work on 32-bit guests
> on 64-bit hosts (and makes the LTP test 'setxattr03' pass
> where it did not previously.)

Applied to linux-user, thanks
 
> Reported-by: pgndev <pgnet.dev@gmail.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/syscall_defs.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index 40c5027..f7cc9f9 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -1101,8 +1101,8 @@ struct target_pollfd {
>  /* Note that the ioctl numbers claim type "long" but the actual type
>   * used by the kernel is "int".
>   */
> -#define TARGET_FS_IOC_GETFLAGS TARGET_IOR('f', 1, long)
> -#define TARGET_FS_IOC_SETFLAGS TARGET_IOW('f', 2, long)
> +#define TARGET_FS_IOC_GETFLAGS TARGET_IOR('f', 1, abi_long)
> +#define TARGET_FS_IOC_SETFLAGS TARGET_IOW('f', 2, abi_long)
>  
>  #define TARGET_FS_IOC_FIEMAP TARGET_IOWR('f',11,struct fiemap)
>  
> -- 
> 2.7.4
> 

Re: [Qemu-devel] [PATCH 2/2] linux-user: Fix TARGET_MTIOCTOP/MTIOCGET/MTIOCPOS values

[Riku Voipio]

On Thu, Oct 12, 2017 at 07:08:55PM +0200, Laurent Vivier wrote:
> Le 12/10/2017 à 18:53, Peter Maydell a écrit :
> > On 12 October 2017 at 17:49, Laurent Vivier <laurent@vivier.eu> wrote:
> >> Le 12/10/2017 à 17:30, Peter Maydell a écrit :
> >>> +#if defined(TARGET_SPARC) || defined(TARGET_MIPS)
> >>> +typedef abi_long target_kernel_daddr_t;
> >>> +#else
> >>> +typedef abi_int target_kernel_daddr_t;
> >>> +#endif
> >>
> >> Perhaps you can add these ones into include/exec/user/abitypes.h ?
> > 
> > I don't think they belong there -- that file is for basic
> > CPU ABI dependent types, not things which are just part of
> > the kernel interface.

> I agree

So we should go with the patch as-is?

Riku

Re: [Qemu-devel] [PATCH 2/2] linux-user: Fix TARGET_MTIOCTOP/MTIOCGET/MTIOCPOS values

[Laurent Vivier]

Le 16/10/2017 à 15:06, Riku Voipio a écrit :
> On Thu, Oct 12, 2017 at 07:08:55PM +0200, Laurent Vivier wrote:
>> Le 12/10/2017 à 18:53, Peter Maydell a écrit :
>>> On 12 October 2017 at 17:49, Laurent Vivier <laurent@vivier.eu> wrote:
>>>> Le 12/10/2017 à 17:30, Peter Maydell a écrit :
>>>>> +#if defined(TARGET_SPARC) || defined(TARGET_MIPS)
>>>>> +typedef abi_long target_kernel_daddr_t;
>>>>> +#else
>>>>> +typedef abi_int target_kernel_daddr_t;
>>>>> +#endif
>>>>
>>>> Perhaps you can add these ones into include/exec/user/abitypes.h ?
>>>
>>> I don't think they belong there -- that file is for basic
>>> CPU ABI dependent types, not things which are just part of
>>> the kernel interface.
> 
>> I agree
> 
> So we should go with the patch as-is?

For this part, yes, but I think for the other comment, STRUCT(mtget,
...) needs to be updated.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 2/2] linux-user: Fix TARGET_MTIOCTOP/MTIOCGET/MTIOCPOS values

[Peter Maydell]

On 16 October 2017 at 14:09, Laurent Vivier <laurent@vivier.eu> wrote:
> For this part, yes, but I think for the other comment, STRUCT(mtget,
> ...) needs to be updated.

Looking more closely I don't think it is as simple as just
adjusting the STRUCT() definition. For a basic type that can
be different on host and guest, I think we'd need to handle
it the way we do the TYPE_OLDDEVT, with special support
in the thunk code for figuring out how large it is and so on.
That seems like a lot of work for a magtape ioctl that I
suspect nobody's using and which we can't test...

Applying this patch as-is would at least fix the ioctl
for everything except MIPS and SPARC hosts and guests.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] linux-user: Fix TARGET_MTIOCTOP/MTIOCGET/MTIOCPOS values

[Laurent Vivier]

Le 16/10/2017 à 15:26, Peter Maydell a écrit :
> On 16 October 2017 at 14:09, Laurent Vivier <laurent@vivier.eu> wrote:
>> For this part, yes, but I think for the other comment, STRUCT(mtget,
>> ...) needs to be updated.
> 
> Looking more closely I don't think it is as simple as just
> adjusting the STRUCT() definition. For a basic type that can
> be different on host and guest, I think we'd need to handle
> it the way we do the TYPE_OLDDEVT, with special support
> in the thunk code for figuring out how large it is and so on.
> That seems like a lot of work for a magtape ioctl that I
> suspect nobody's using and which we can't test...

I agree.

> Applying this patch as-is would at least fix the ioctl
> for everything except MIPS and SPARC hosts and guests.

Yes, so I think it can be applied as-is.

Thanks,
Laurent