Re: [PATCH 2/2] linux-user: Add strace support for printing arguments of ioctl()

[Laurent Vivier]

Le 12/06/2020 à 18:40, Filip Bozuta a écrit :
> From: Filip Bozuta <Filip.Bozuta@syrmia.com>
> 
> This patch implements functionality for strace argument printing for ioctls.
> When running ioctls through qemu with "-strace", they get printed in format:
> 
>     "ioctl(fd_num,0x*,0x*) = ret_value"
> 
> where the request code an the ioctl's third argument get printed in a 
> hexadicemal
> format. This patch changes that by enabling strace to print both the request 
> code
> name and the contents of the third argument. For example, when running ioctl
> RTC_SET_TIME with "-strace", with changes from this patch, it gets printed in
> this way:
> 
>     "ioctl(3,RTC_SET_TIME,{12,13,15,20,10,119,0,0,0}) = 0"
> 
> In case of IOC_R type ioctls, the contents of the third argument get printed
> after the return value, and the argument inside the ioctl call gets printed
> as pointer in hexadecimal format. For example, when running RTC_RD_TIME with
> "-strace", with changes from this patch, it gets printed in this way:
> 
>     "ioctl(3,RTC_RD_TIME,0x40800374) = 0 ({22,9,13,11,5,120,0,0,0})"
> 
> In case of IOC_RW type ioctls, the contents of the third argument get printed
> both inside the ioctl call and after the return value.
> 
> Implementation notes:
> 
>     Functions "print_ioctl()" and "print_syscall_ret_ioctl()", that are 
> defined
>     in "strace.c", are listed in file "strace.list" as "call" and "result"
>     value for ioctl. Structure definition "IOCTLEntry" as well as predefined
>     values for IOC_R, IOC_W and IOC_RW were cut and pasted from file 
> "syscall.c"
>     to file "qemu.h" so that they can be used by these functions to print the
>     contents of the third ioctl argument. Also, the "static" identifier for 
> array
>     "ioctl_entries[]" was removed and this array was declared as "extern" in 
> "qemu.h"
>     so that it can also be used by these functions. To decode the structure 
> type
>     of the ioctl third argument, function "thunk_print()" was defined in file
>     "thunk.c" and its definition is somewhat simillar to that of function
>     "thunk_convert()".
> 
> Signed-off-by: Filip Bozuta <Filip.Bozuta@syrmia.com>
> ---
>  include/exec/user/thunk.h |   1 +
>  linux-user/qemu.h         |  20 +++++
>  linux-user/strace.c       | 120 +++++++++++++++++++++++++++++
>  linux-user/strace.list    |   3 +-
>  linux-user/syscall.c      |  20 +----
>  thunk.c                   | 154 ++++++++++++++++++++++++++++++++++++++
>  6 files changed, 298 insertions(+), 20 deletions(-)
> 
> diff --git a/include/exec/user/thunk.h b/include/exec/user/thunk.h
> index eae2c27f99..7992475c9f 100644
> --- a/include/exec/user/thunk.h
> +++ b/include/exec/user/thunk.h
> @@ -73,6 +73,7 @@ void thunk_register_struct_direct(int id, const char *name,
>                                    const StructEntry *se1);
>  const argtype *thunk_convert(void *dst, const void *src,
>                               const argtype *type_ptr, int to_host);
> +const argtype *thunk_print(void *arg, const argtype *type_ptr);
>  
>  extern StructEntry *struct_entries;
>  
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index be67391ba4..5c964389c1 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -184,6 +184,26 @@ struct linux_binprm {
>          int (*core_dump)(int, const CPUArchState *); /* coredump routine */
>  };
>  
> +typedef struct IOCTLEntry IOCTLEntry;
> +
> +typedef abi_long do_ioctl_fn(const IOCTLEntry *ie, uint8_t *buf_temp,
> +                             int fd, int cmd, abi_long arg);
> +
> +struct IOCTLEntry {
> +    int target_cmd;
> +    unsigned int host_cmd;
> +    const char *name;
> +    int access;
> +    do_ioctl_fn *do_ioctl;
> +    const argtype arg_type[5];
> +};
> +
> +extern IOCTLEntry ioctl_entries[];
> +
> +#define IOC_R 0x0001
> +#define IOC_W 0x0002
> +#define IOC_RW (IOC_R | IOC_W)
> +
>  void do_init_thread(struct target_pt_regs *regs, struct image_info *infop);
>  abi_ulong loader_build_argptr(int envc, int argc, abi_ulong sp,
>                                abi_ulong stringp, int push_ptr);
> diff --git a/linux-user/strace.c b/linux-user/strace.c
> index 5f370256e3..8de8f242ae 100644
> --- a/linux-user/strace.c
> +++ b/linux-user/strace.c
> @@ -869,6 +869,51 @@ print_syscall_ret_listxattr(const struct syscallname 
> *name, abi_long ret,
>  #define print_syscall_ret_flistxattr     print_syscall_ret_listxattr
>  #endif
>  
> +#ifdef TARGET_NR_ioctl
> +static void
> +print_syscall_ret_ioctl(const struct syscallname *name, abi_long ret,
> +                        abi_long arg0, abi_long arg1, abi_long arg2,
> +                        abi_long arg3, abi_long arg4, abi_long arg5)
> +{
> +    const char *errstr = NULL;
> +
> +    qemu_log(" = ");
> +    if (ret < 0) {
> +        qemu_log("-1 errno=%d", errno);
> +        errstr = target_strerror(-ret);
> +        if (errstr) {
> +            qemu_log(" (%s)", errstr);
> +        }

You should use the error function you used in your provious series.
You can explain it is based on it by adding a Based-on: tag with the
mail message id:

Based-on: <20200611155109.3648-1-filip.bozuta@syrmia.com>

> +    } else {
> +        qemu_log(TARGET_ABI_FMT_ld, ret);
> +
> +        const IOCTLEntry *ie;
> +        const argtype *arg_type;
> +        void *argptr;
> +        int target_size;
> +
> +        for (ie = ioctl_entries; ie->target_cmd != 0; ie++) {
> +            if (ie->target_cmd == arg1) {
> +                break;
> +            }
> +        }
> +
> +        if (ie->target_cmd == arg1 &&
> +           (ie->access == IOC_R || ie->access == IOC_RW)) {
> +            arg_type = ie->arg_type;
> +            qemu_log(" (");
> +            arg_type++;
> +            target_size = thunk_type_size(arg_type, 0);
> +            argptr = lock_user(VERIFY_READ, arg2, target_size, 1);
> +            thunk_print(argptr, arg_type);
> +            unlock_user(argptr, arg2, target_size);
> +            qemu_log(")");
> +        }
> +    }
> +    qemu_log("\n");
> +}
> +#endif
> +
>  UNUSED static struct flags access_flags[] = {
>      FLAG_GENERIC(F_OK),
>      FLAG_GENERIC(R_OK),
> @@ -3008,6 +3053,81 @@ print_statx(const struct syscallname *name,
>  }
>  #endif
>  
> +#ifdef TARGET_NR_ioctl
> +static void
> +print_ioctl(const struct syscallname *name,
> +            abi_long arg0, abi_long arg1, abi_long arg2,
> +            abi_long arg3, abi_long arg4, abi_long arg5)
> +{
> +    print_syscall_prologue(name);
> +    print_raw_param("%d", arg0, 0);
> +
> +    const IOCTLEntry *ie;
> +    const argtype *arg_type;
> +    void *argptr;
> +    int target_size;
> +
> +    for (ie = ioctl_entries; ie->target_cmd != 0; ie++) {
> +        if (ie->target_cmd == arg1) {
> +            break;
> +        }
> +    }
> +
> +    if (ie->target_cmd == 0) {
> +        print_raw_param("%#x", arg1, 0);
> +        print_raw_param("%#x", arg2, 1);
> +    } else {
> +        qemu_log("%s", ie->name);
> +        arg_type = ie->arg_type;
> +
> +        if (arg_type[0] != TYPE_NULL) {
> +            qemu_log(",");
> +
> +            switch (arg_type[0]) {
> +            case TYPE_PTRVOID:
> +                print_pointer(arg2, 1);
> +                break;
> +            case TYPE_CHAR:
> +            case TYPE_SHORT:
> +            case TYPE_INT:
> +                print_raw_param("%d", arg2, 1);
> +                break;
> +            case TYPE_LONG:
> +                print_raw_param(TARGET_ABI_FMT_ld, arg2, 1);
> +                break;
> +            case TYPE_ULONG:
> +                print_raw_param(TARGET_ABI_FMT_lu, arg2, 1);
> +                break;
> +            case TYPE_LONGLONG:
> +                print_raw_param("%lld", arg2, 1);
> +                break;
> +            case TYPE_ULONGLONG:
> +                print_raw_param("%llu", arg2, 1);
> +                break;

I think the ULONGLONG and LONGLONG are not possible because arg2 is an
abi_long and is not read from memory.

Thanks,
Laurent