[Qemu-devel] [PATCH 0/3] linux-user: Assorted fixed for libarchive

[Qemu-devel] [PATCH 0/3] linux-user: Assorted fixed for libarchive

[Alexander Graf]

Howdy,

While running libarchive's test suite in qemu-arm, I ran into a
few linux-user bugs and unimplemented ioctls.

The statfs one is really nasty. IMHO it's 1.2 material.

There's also a bug outstanding for libarchive where FIEMAP doesn't
seem to work properly. I haven't gotten around to track it down
yet and just disabled it for us locally, but if anyone who knows
that ioctl has some spare time, I'm quite sure there's something
lurking there.


Alex

Alexander Graf (3):
  linux-user: implement FS_IOC_GETFLAGS ioctl
  linux-user: implement FS_IOC_SETFLAGS ioctl
  linux-user: fix statfs

 linux-user/ioctls.h       |    2 ++
 linux-user/syscall.c      |   12 ++++++++++++
 linux-user/syscall_defs.h |    3 +++
 3 files changed, 17 insertions(+), 0 deletions(-)

[Qemu-devel] [PATCH 1/3] linux-user: implement FS_IOC_GETFLAGS ioctl

[Alexander Graf]

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 linux-user/ioctls.h       |    1 +
 linux-user/syscall_defs.h |    2 ++
 2 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h
index bb76c56..1b798b3 100644
--- a/linux-user/ioctls.h
+++ b/linux-user/ioctls.h
@@ -86,6 +86,7 @@
      IOCTL_SPECIAL(FS_IOC_FIEMAP, IOC_W | IOC_R, do_ioctl_fs_ioc_fiemap,
                    MK_PTR(MK_STRUCT(STRUCT_fiemap)))
 #endif
+  IOCTL(FS_IOC_GETFLAGS, IOC_R, MK_PTR(TYPE_INT))
 
   IOCTL(SIOCATMARK, 0, TYPE_NULL)
   IOCTL(SIOCADDRT, IOC_W, MK_PTR(MK_STRUCT(STRUCT_rtentry)))
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 3b7b1c3..67fbcab 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -2344,6 +2344,8 @@ struct target_eabi_flock64 {
 #define TARGET_MTIOCGET        TARGET_IOR('m', 2, struct mtget)
 #define TARGET_MTIOCPOS        TARGET_IOR('m', 3, struct mtpos)
 
+#define TARGET_FS_IOC_GETFLAGS TARGET_IORU('f', 1)
+
 struct target_sysinfo {
     abi_long uptime;                /* Seconds since boot */
     abi_ulong loads[3];             /* 1, 5, and 15 minute load averages */
-- 
1.6.0.2

[Qemu-devel] [PATCH 2/3] linux-user: implement FS_IOC_SETFLAGS ioctl

[Alexander Graf]

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 linux-user/ioctls.h       |    1 +
 linux-user/syscall_defs.h |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h
index 1b798b3..5027c74 100644
--- a/linux-user/ioctls.h
+++ b/linux-user/ioctls.h
@@ -87,6 +87,7 @@
                    MK_PTR(MK_STRUCT(STRUCT_fiemap)))
 #endif
   IOCTL(FS_IOC_GETFLAGS, IOC_R, MK_PTR(TYPE_INT))
+  IOCTL(FS_IOC_SETFLAGS, IOC_W, MK_PTR(TYPE_INT))
 
   IOCTL(SIOCATMARK, 0, TYPE_NULL)
   IOCTL(SIOCADDRT, IOC_W, MK_PTR(MK_STRUCT(STRUCT_rtentry)))
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 67fbcab..349229d 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -2345,6 +2345,7 @@ struct target_eabi_flock64 {
 #define TARGET_MTIOCPOS        TARGET_IOR('m', 3, struct mtpos)
 
 #define TARGET_FS_IOC_GETFLAGS TARGET_IORU('f', 1)
+#define TARGET_FS_IOC_SETFLAGS TARGET_IOWU('f', 2)
 
 struct target_sysinfo {
     abi_long uptime;                /* Seconds since boot */
-- 
1.6.0.2

[Qemu-devel] [PATCH 3/3] linux-user: fix statfs

[Alexander Graf]

The statfs syscall should always memset(0) its full struct extent before
writing to it. Newer versions of the syscall use one of the reserved fields
for flags, which would otherwise get stale values from uncleaned memory.

This fixes libarchive for me, which got confused about the return value of
pathconf("/", _PC_REC_XFER_ALIGN) otherwise, as it some times gave old pointers
as return value.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 linux-user/syscall.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index d19efb8..61f5718 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -6667,6 +6667,12 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
             __put_user(stfs.f_fsid.__val[0], &target_stfs->f_fsid.val[0]);
             __put_user(stfs.f_fsid.__val[1], &target_stfs->f_fsid.val[1]);
             __put_user(stfs.f_namelen, &target_stfs->f_namelen);
+            __put_user(stfs.f_frsize, &target_stfs->f_frsize);
+            __put_user(0, &target_stfs->f_spare[0]);
+            __put_user(0, &target_stfs->f_spare[1]);
+            __put_user(0, &target_stfs->f_spare[2]);
+            __put_user(0, &target_stfs->f_spare[3]);
+            __put_user(0, &target_stfs->f_spare[4]);
             unlock_user_struct(target_stfs, arg2, 1);
         }
         break;
@@ -6695,6 +6701,12 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
             __put_user(stfs.f_fsid.__val[0], &target_stfs->f_fsid.val[0]);
             __put_user(stfs.f_fsid.__val[1], &target_stfs->f_fsid.val[1]);
             __put_user(stfs.f_namelen, &target_stfs->f_namelen);
+            __put_user(stfs.f_frsize, &target_stfs->f_frsize);
+            __put_user(0, &target_stfs->f_spare[0]);
+            __put_user(0, &target_stfs->f_spare[1]);
+            __put_user(0, &target_stfs->f_spare[2]);
+            __put_user(0, &target_stfs->f_spare[3]);
+            __put_user(0, &target_stfs->f_spare[4]);
             unlock_user_struct(target_stfs, arg3, 1);
         }
         break;
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH 1/3] linux-user: implement FS_IOC_GETFLAGS ioctl

[Peter Maydell]

On 21 August 2012 13:43, Alexander Graf <agraf@suse.de> wrote:
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  linux-user/ioctls.h       |    1 +
>  linux-user/syscall_defs.h |    2 ++
>  2 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h
> index bb76c56..1b798b3 100644
> --- a/linux-user/ioctls.h
> +++ b/linux-user/ioctls.h
> @@ -86,6 +86,7 @@
>       IOCTL_SPECIAL(FS_IOC_FIEMAP, IOC_W | IOC_R, do_ioctl_fs_ioc_fiemap,
>                     MK_PTR(MK_STRUCT(STRUCT_fiemap)))
>  #endif
> +  IOCTL(FS_IOC_GETFLAGS, IOC_R, MK_PTR(TYPE_INT))
>
>    IOCTL(SIOCATMARK, 0, TYPE_NULL)
>    IOCTL(SIOCADDRT, IOC_W, MK_PTR(MK_STRUCT(STRUCT_rtentry)))
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index 3b7b1c3..67fbcab 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -2344,6 +2344,8 @@ struct target_eabi_flock64 {
>  #define TARGET_MTIOCGET        TARGET_IOR('m', 2, struct mtget)
>  #define TARGET_MTIOCPOS        TARGET_IOR('m', 3, struct mtpos)
>
> +#define TARGET_FS_IOC_GETFLAGS TARGET_IORU('f', 1)

This and the SETFLAGS one in the next patch fail the consistency
check that an x86_64-on-x86_64 linux-user binary performs:

cam-vm-266:precise:qemu$ ./x86_64-linux-user/qemu-x86_64 /bin/echo hello
ERROR: ioctl(FS_IOC_GETFLAGS): target=0x80046601 host=0x80086601
ERROR: ioctl(FS_IOC_SETFLAGS): target=0x40046602 host=0x40086602
hello

This is indicating that your ioctl definition is wrong:
> +  IOCTL(FS_IOC_GETFLAGS, IOC_R, MK_PTR(TYPE_INT))

...it should be TYPE_LONG.

Incidentally you could also set the target ioctl at compile
time rather than making syscall_init patch the size field
at runtime. I don't know whether one or the other is better
style... There's an argument for the 'automatic' one as
it enforces consistency and catches errors like the one above.
Anyway, the compile-time option woud be:

#define TARGET_FS_IOC_GETFLAGS TARGET_IOR('f', 1, abi_long)

(You could also squash patches 1 and 2 together IMHO.)

-- PMM

Re: [Qemu-devel] [PATCH 3/3] linux-user: fix statfs

[Peter Maydell]

On 21 August 2012 13:43, Alexander Graf <agraf@suse.de> wrote:
> The statfs syscall should always memset(0) its full struct extent before
> writing to it. Newer versions of the syscall use one of the reserved fields
> for flags, which would otherwise get stale values from uncleaned memory.
>
> This fixes libarchive for me, which got confused about the return value of
> pathconf("/", _PC_REC_XFER_ALIGN) otherwise, as it some times gave old pointers
> as return value.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  linux-user/syscall.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index d19efb8..61f5718 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -6667,6 +6667,12 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>              __put_user(stfs.f_fsid.__val[0], &target_stfs->f_fsid.val[0]);
>              __put_user(stfs.f_fsid.__val[1], &target_stfs->f_fsid.val[1]);
>              __put_user(stfs.f_namelen, &target_stfs->f_namelen);
> +            __put_user(stfs.f_frsize, &target_stfs->f_frsize);
> +            __put_user(0, &target_stfs->f_spare[0]);
> +            __put_user(0, &target_stfs->f_spare[1]);
> +            __put_user(0, &target_stfs->f_spare[2]);
> +            __put_user(0, &target_stfs->f_spare[3]);
> +            __put_user(0, &target_stfs->f_spare[4]);
>              unlock_user_struct(target_stfs, arg2, 1);
>          }
>          break;
> @@ -6695,6 +6701,12 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>              __put_user(stfs.f_fsid.__val[0], &target_stfs->f_fsid.val[0]);
>              __put_user(stfs.f_fsid.__val[1], &target_stfs->f_fsid.val[1]);
>              __put_user(stfs.f_namelen, &target_stfs->f_namelen);
> +            __put_user(stfs.f_frsize, &target_stfs->f_frsize);
> +            __put_user(0, &target_stfs->f_spare[0]);
> +            __put_user(0, &target_stfs->f_spare[1]);
> +            __put_user(0, &target_stfs->f_spare[2]);
> +            __put_user(0, &target_stfs->f_spare[3]);
> +            __put_user(0, &target_stfs->f_spare[4]);
>              unlock_user_struct(target_stfs, arg3, 1);
>          }
>          break;

For some targets there are 6 f_spare[] slots, not 5... I suggest
     memset(target_stfs->f_spare, 0, sizeof(target_stfs->f_spare));
instead (which matches how the kernel does this).

-- PMM

Re: [Qemu-devel] [PATCH 1/3] linux-user: implement FS_IOC_GETFLAGS ioctl

[Riku Voipio]

Hi Alexander,

On 21 August 2012 16:47, Peter Maydell <peter.maydell@linaro.org> wrote:
>> +#define TARGET_FS_IOC_GETFLAGS TARGET_IORU('f', 1)
>
> This and the SETFLAGS one in the next patch fail the consistency
> check that an x86_64-on-x86_64 linux-user binary performs:
>
> cam-vm-266:precise:qemu$ ./x86_64-linux-user/qemu-x86_64 /bin/echo hello
> ERROR: ioctl(FS_IOC_GETFLAGS): target=0x80046601 host=0x80086601
> ERROR: ioctl(FS_IOC_SETFLAGS): target=0x40046602 host=0x40086602
> hello
>
> This is indicating that your ioctl definition is wrong:
>> +  IOCTL(FS_IOC_GETFLAGS, IOC_R, MK_PTR(TYPE_INT))
>
> ...it should be TYPE_LONG.
>
> Incidentally you could also set the target ioctl at compile
> time rather than making syscall_init patch the size field
> at runtime. I don't know whether one or the other is better
> style... There's an argument for the 'automatic' one as
> it enforces consistency and catches errors like the one above.
> Anyway, the compile-time option woud be:
>
> #define TARGET_FS_IOC_GETFLAGS TARGET_IOR('f', 1, abi_long)
>
> (You could also squash patches 1 and 2 together IMHO.)

Has there been an updated patch with Peter's suggestions sent the list since?

Riku

Re: [Qemu-devel] [PATCH 1/3] linux-user: implement FS_IOC_GETFLAGS ioctl

[Alexander Graf]

On 11.10.2012, at 15:36, Riku Voipio wrote:

> Hi Alexander,
> 
> On 21 August 2012 16:47, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> +#define TARGET_FS_IOC_GETFLAGS TARGET_IORU('f', 1)
>> 
>> This and the SETFLAGS one in the next patch fail the consistency
>> check that an x86_64-on-x86_64 linux-user binary performs:
>> 
>> cam-vm-266:precise:qemu$ ./x86_64-linux-user/qemu-x86_64 /bin/echo hello
>> ERROR: ioctl(FS_IOC_GETFLAGS): target=0x80046601 host=0x80086601
>> ERROR: ioctl(FS_IOC_SETFLAGS): target=0x40046602 host=0x40086602
>> hello
>> 
>> This is indicating that your ioctl definition is wrong:
>>> +  IOCTL(FS_IOC_GETFLAGS, IOC_R, MK_PTR(TYPE_INT))
>> 
>> ...it should be TYPE_LONG.
>> 
>> Incidentally you could also set the target ioctl at compile
>> time rather than making syscall_init patch the size field
>> at runtime. I don't know whether one or the other is better
>> style... There's an argument for the 'automatic' one as
>> it enforces consistency and catches errors like the one above.
>> Anyway, the compile-time option woud be:
>> 
>> #define TARGET_FS_IOC_GETFLAGS TARGET_IOR('f', 1, abi_long)
>> 
>> (You could also squash patches 1 and 2 together IMHO.)
> 
> Has there been an updated patch with Peter's suggestions sent the list since?

Didn't get around to it yet, no.


Alex

> 
> Riku