Re: [Qemu-devel] [PULL 06/22] linux-user: Fix syslog() syscall support

[Riku Voipio <riku.voipio@iki.fi>]

On Mon, Oct 17, 2016 at 04:24:24PM +0300, riku.voipio@linaro.org wrote:
> From: Aleksandar Markovic <aleksandar.markovic@imgtec.com>
> 
> There are currently several problems related to syslog() support.
> 
> For example, if the second argument "bufp" of target syslog() syscall
> is NULL, the current implementation always returns error code EFAULT.
> However, NULL is a perfectly valid value for the second argument for
> many use cases of this syscall. This is, for example, visible from
> this excerpt of man page for syslog(2):
> 
> > EINVAL Bad arguments (e.g., bad type; or for type 2, 3, or 4, buf is
> >        NULL, or len is less than zero; or for type 8, the level is
> >        outside the range 1 to 8).
> 
> Moreover, the argument "bufp" is ignored for all cases of values of the
> first argument, except 2, 3 and 4. This means that for such cases
> (the first argument is not 2, 3 or 4), there is no need to pass "buf"
> between host and target, and it can be set to NULL while calling host's
> syslog(), without loss of emulation accuracy.
> 
> Note also that if "bufp" is NULL and the first argument is 2, 3 or 4, the
> correct returned error code is EINVAL, not EFAULT.
> 
> All these details are reflected in this patch.
> 
> "#ifdef TARGET_NR_syslog" is also proprerly inserted when needed.
> 
> Support for Qemu's "-strace" switch for syslog() syscall is included too.
> 
> LTP tests syslog11 and syslog12 pass with this patch (while fail without
> it), on any platform.
> 
> Signed-off-by: Aleksandar Markovic <aleksandar.markovic@imgtec.com>
> Signed-off-by: Riku Voipio <riku.voipio@linaro.org>
> ---
>  linux-user/strace.c       | 72 +++++++++++++++++++++++++++++++++++++++++++++++
>  linux-user/strace.list    |  2 +-
>  linux-user/syscall.c      | 49 ++++++++++++++++++++++++++++----
>  linux-user/syscall_defs.h | 25 ++++++++++++++++
>  4 files changed, 141 insertions(+), 7 deletions(-)
> 
> diff --git a/linux-user/strace.c b/linux-user/strace.c
> index a0e45b5..679f840 100644
> --- a/linux-user/strace.c
> +++ b/linux-user/strace.c
> @@ -1827,6 +1827,78 @@ print_rt_sigprocmask(const struct syscallname *name,
>  }
>  #endif
>  
> +#ifdef TARGET_NR_syslog
> +static void
> +print_syslog_action(abi_ulong arg, int last)
> +{
> +    const char *type;
> +
> +    switch (arg) {
> +        case TARGET_SYSLOG_ACTION_CLOSE: {
> +            type = "SYSLOG_ACTION_CLOSE";
> +            break;
> +        }
> +        case TARGET_SYSLOG_ACTION_OPEN: {
> +            type = "SYSLOG_ACTION_OPEN";
> +            break;
> +        }
> +        case TARGET_SYSLOG_ACTION_READ: {
> +            type = "SYSLOG_ACTION_READ";
> +            break;
> +        }
> +        case TARGET_SYSLOG_ACTION_READ_ALL: {
> +            type = "SYSLOG_ACTION_READ_ALL";
> +            break;
> +        }
> +        case TARGET_SYSLOG_ACTION_READ_CLEAR: {
> +            type = "SYSLOG_ACTION_READ_CLEAR";
> +            break;
> +        }
> +        case TARGET_SYSLOG_ACTION_CLEAR: {
> +            type = "SYSLOG_ACTION_CLEAR";
> +            break;
> +        }
> +        case TARGET_SYSLOG_ACTION_CONSOLE_OFF: {
> +            type = "SYSLOG_ACTION_CONSOLE_OFF";
> +            break;
> +        }
> +        case TARGET_SYSLOG_ACTION_CONSOLE_ON: {
> +            type = "SYSLOG_ACTION_CONSOLE_ON";
> +            break;
> +        }
> +        case TARGET_SYSLOG_ACTION_CONSOLE_LEVEL: {
> +            type = "SYSLOG_ACTION_CONSOLE_LEVEL";
> +            break;
> +        }
> +        case TARGET_SYSLOG_ACTION_SIZE_UNREAD: {
> +            type = "SYSLOG_ACTION_SIZE_UNREAD";
> +            break;
> +        }
> +        case TARGET_SYSLOG_ACTION_SIZE_BUFFER: {
> +            type = "SYSLOG_ACTION_SIZE_BUFFER";
> +            break;
> +        }
> +        default: {
> +            print_raw_param("%ld", arg, last);
> +            return;
> +        }
> +    }
> +    gemu_log("%s%s", type, get_comma(last));
> +}
> +
> +static void
> +print_syslog(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_syslog_action(arg0, 0);
> +    print_pointer(arg1, 0);
> +    print_raw_param("%d", arg2, 1);
> +    print_syscall_epilogue(name);
> +}
> +#endif
> +
>  #ifdef TARGET_NR_mknod
>  static void
>  print_mknod(const struct syscallname *name,
> diff --git a/linux-user/strace.list b/linux-user/strace.list
> index f6dd044..2c7ad2b 100644
> --- a/linux-user/strace.list
> +++ b/linux-user/strace.list
> @@ -1486,7 +1486,7 @@
>  { TARGET_NR_sys_kexec_load, "sys_kexec_load" , NULL, NULL, NULL },
>  #endif
>  #ifdef TARGET_NR_syslog
> -{ TARGET_NR_syslog, "syslog" , NULL, NULL, NULL },
> +{ TARGET_NR_syslog, "syslog" , NULL, print_syslog, NULL },
>  #endif
>  #ifdef TARGET_NR_sysmips
>  { TARGET_NR_sysmips, "sysmips" , NULL, NULL, NULL },
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 05b4c41..a3e7d51 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -9339,14 +9339,51 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>          ret = do_setsockopt(arg1, arg2, arg3, arg4, (socklen_t) arg5);
>          break;
>  #endif
> -
> +#if defined(TARGET_NR_syslog)
>      case TARGET_NR_syslog:
> -        if (!(p = lock_user_string(arg2)))
> -            goto efault;
> -        ret = get_errno(sys_syslog((int)arg1, p, (int)arg3));
> -        unlock_user(p, arg2, 0);
> -        break;
> +        {
> +            int len = arg2;
>  
> +            switch (arg1) {
> +            case TARGET_SYSLOG_ACTION_CLOSE:         /* Close log */
> +            case TARGET_SYSLOG_ACTION_OPEN:          /* Open log */
> +            case TARGET_SYSLOG_ACTION_CLEAR:         /* Clear ring buffer */
> +            case TARGET_SYSLOG_ACTION_CONSOLE_OFF:   /* Disable logging */
> +            case TARGET_SYSLOG_ACTION_CONSOLE_ON:    /* Enable logging */
> +            case TARGET_SYSLOG_ACTION_CONSOLE_LEVEL: /* Set messages level */
> +            case TARGET_SYSLOG_ACTION_SIZE_UNREAD:   /* Number of chars */
> +            case TARGET_SYSLOG_ACTION_SIZE_BUFFER:   /* Size of the buffer */
> +                {
> +                    ret = get_errno(sys_syslog((int)arg1, NULL, (int)arg3));
> +                }
> +                break;
> +            case TARGET_SYSLOG_ACTION_READ:          /* Read from log */
> +            case TARGET_SYSLOG_ACTION_READ_CLEAR:    /* Read/clear msgs */
> +            case TARGET_SYSLOG_ACTION_READ_ALL:      /* Read last messages */
> +                {
> +                    if (len < 0) {
> +                        ret = -TARGET_EINVAL;
> +                        goto fail;
> +                    }
> +                    if (len == 0) {
> +                        break;
> +                    }
> +                    p = lock_user(VERIFY_WRITE, arg2, arg3, 0);
> +                    if (!p) {
> +                        ret = -TARGET_EINVAL;
> +                        goto fail;
> +                    }
> +                    ret = get_errno(sys_syslog((int)arg1, p, (int)arg3));

The error paths above are incorrect, compared to:

http://lxr.free-electrons.com/source/kernel/printk/printk.c#L1465

I'll squash in the following change to match the kernel behaviour:

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 2072b1f..dfc483c 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -9377,16 +9377,17 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
             case TARGET_SYSLOG_ACTION_READ_CLEAR:    /* Read/clear msgs */
             case TARGET_SYSLOG_ACTION_READ_ALL:      /* Read last messages */
                 {
+                    ret = -TARGET_EINVAL;
                     if (len < 0) {
-                        ret = -TARGET_EINVAL;
                         goto fail;
                     }
+                    ret = 0;
                     if (len == 0) {
                         break;
                     }
                     p = lock_user(VERIFY_WRITE, arg2, arg3, 0);
                     if (!p) {
-                        ret = -TARGET_EINVAL;
+                        ret = -TARGET_EFAULT;
                         goto fail;
                     }
                     ret = get_errno(sys_syslog((int)arg1, p, (int)arg3));



> +                    unlock_user(p, arg2, arg3);
> +                }
> +                break;
> +            default:
> +                ret = -EINVAL;
> +                break;
> +            }
> +        }
> +        break;
> +#endif
>      case TARGET_NR_setitimer:
>          {
>              struct itimerval value, ovalue, *pvalue;
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index adb7153..61270ef 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -2688,4 +2688,29 @@ struct target_user_cap_data {
>      uint32_t inheritable;
>  };
>  
> +/* from kernel's include/linux/syslog.h */
> +
> +/* Close the log.  Currently a NOP. */
> +#define TARGET_SYSLOG_ACTION_CLOSE          0
> +/* Open the log. Currently a NOP. */
> +#define TARGET_SYSLOG_ACTION_OPEN           1
> +/* Read from the log. */
> +#define TARGET_SYSLOG_ACTION_READ           2
> +/* Read all messages remaining in the ring buffer. */
> +#define TARGET_SYSLOG_ACTION_READ_ALL       3
> +/* Read and clear all messages remaining in the ring buffer */
> +#define TARGET_SYSLOG_ACTION_READ_CLEAR     4
> +/* Clear ring buffer. */
> +#define TARGET_SYSLOG_ACTION_CLEAR          5
> +/* Disable printk's to console */
> +#define TARGET_SYSLOG_ACTION_CONSOLE_OFF    6
> +/* Enable printk's to console */
> +#define TARGET_SYSLOG_ACTION_CONSOLE_ON     7
> +/* Set level of messages printed to console */
> +#define TARGET_SYSLOG_ACTION_CONSOLE_LEVEL  8
> +/* Return number of unread characters in the log buffer */
> +#define TARGET_SYSLOG_ACTION_SIZE_UNREAD    9
> +/* Return size of the log buffer */
> +#define TARGET_SYSLOG_ACTION_SIZE_BUFFER   10
> +
>  #endif
> -- 
> 2.1.4
> 
>