Re: [Qemu-devel] [PATCH v2] linux-user: add signalfd/signalfd4 syscalls

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

On Sat, Sep 05, 2015 at 12:03:11AM +0200, Laurent Vivier wrote:
> 
> 
> Le 04/09/2015 15:35, Peter Maydell a écrit :
> > On 3 September 2015 at 00:58, Laurent Vivier <laurent@vivier.eu> wrote:
> >> This patch introduces a system very similar to the one used in the kernel
> >> to attach specific functions to a given file descriptor.
> >>
> >> In this case, we attach a specific "host_to_target()" translator to the fd
> >> returned by signalfd() to be able to byte-swap the signalfd_siginfo
> >> structure provided by read().
> >>
> >> This patch allows to execute the example program given by
> >> man signalfd(2):
> >>
> >>  #define _GNU_SOURCE
> >>  #define _FILE_OFFSET_BITS 64
> >>  #include <stdio.h>
> >>  #include <time.h>
> >>  #include <stdlib.h>
> >>  #include <unistd.h>
> >>  #include <sys/resource.h>
> >>
> >>  #define errExit(msg)     do { perror(msg); exit(EXIT_FAILURE); } while (0)
> >>
> >>  int
> >>  main(int argc, char *argv[])
> >>  {
> >>      sigset_t mask;
> >>      int sfd;
> >>      struct signalfd_siginfo fdsi;
> >>      ssize_t s;
> >>
> >>      sigemptyset(&mask);
> >>      sigaddset(&mask, SIGINT);
> >>      sigaddset(&mask, SIGQUIT);
> >>
> >>      /* Block signals so that they aren't handled
> >>         according to their default dispositions */
> >>
> >>      if (sigprocmask(SIG_BLOCK, &mask, NULL) == -1)
> >>          handle_error("sigprocmask");
> >>
> >>      sfd = signalfd(-1, &mask, 0);
> >>      if (sfd == -1)
> >>          handle_error("signalfd");
> >>
> >>      for (;;) {
> >>          s = read(sfd, &fdsi, sizeof(struct signalfd_siginfo));
> >>          if (s != sizeof(struct signalfd_siginfo))
> >>              handle_error("read");
> >>
> >>          if (fdsi.ssi_signo == SIGINT) {
> >>              printf("Got SIGINT\n");
> >>          } else if (fdsi.ssi_signo == SIGQUIT) {
> >>              printf("Got SIGQUIT\n");
> >>              exit(EXIT_SUCCESS);
> >>          } else {
> >>              printf("Read unexpected signal\n");
> >>          }
> >>      }
> >>  }
> >>
> >>  $ ./signalfd_demo
> >>  ^CGot SIGINT
> >>  ^CGot SIGINT
> >>  ^\Got SIGQUIT
> >>
> >> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> >> ---
> >> v2: Update commit message with example from man page
> >>     Use CamelCase for struct
> >>     Allocate entries in the fd array on demand
> >>     Clear fd entries on open(), close(),...
> >>     Swap ssi_errno
> >>     Try to manage dup() and O_CLOEXEC cases
> >>     Fix signalfd() parameters
> >>     merge signalfd() and signalfd4()
> >>     Change the API to only provide functions to process
> >>     data stream.
> >>
> >>     I don't add ssi_addr_lsb in host_to_target_signalfd_siginfo()
> >>     because it is not in /usr/include/sys/signalfd.h
> >>
> >>     TargetFdTrans has an unused field, target_to_host, for symmetry
> >>     and which could used later with netlink() functions.
> >>
> >>  linux-user/syscall.c | 182 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 182 insertions(+)
> >>
> >> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> >> index f62c698..a1cacea 100644
> >> --- a/linux-user/syscall.c
> >> +++ b/linux-user/syscall.c
> >> @@ -60,6 +60,7 @@ int __clone2(int (*fn)(void *), void *child_stack_base,
> >>  #include <sys/statfs.h>
> >>  #include <utime.h>
> >>  #include <sys/sysinfo.h>
> >> +#include <sys/signalfd.h>
> >>  //#include <sys/user.h>
> >>  #include <netinet/ip.h>
> >>  #include <netinet/tcp.h>
> >> @@ -294,6 +295,67 @@ static bitmask_transtbl fcntl_flags_tbl[] = {
> >>    { 0, 0, 0, 0 }
> >>  };
> >>
> >> +typedef abi_long (*TargetFdFunc)(void *, size_t);
> >> +struct TargetFdTrans {
> >> +    TargetFdFunc host_to_target;
> >> +    TargetFdFunc target_to_host;
> >> +};
> >> +typedef struct TargetFdTrans TargetFdTrans;
> > 
> > It's more usual to just combine the struct definition with
> > the typedef:
> >   typedef struct TargetFdTrans {
> >       ...
> >   } TargetFdTrans;
> 
> ok
> 
> >> +struct TargetFdEntry {
> >> +    TargetFdTrans *trans;
> >> +    int flags;
> >> +};
> >> +typedef struct TargetFdEntry TargetFdEntry;
> >> +
> >> +static TargetFdEntry *target_fd_trans;
> >> +
> >> +static int target_fd_max;
> >> +
> >> +static TargetFdFunc fd_trans_host_to_target(int fd)
> >> +{
> >> +    return fd < target_fd_max && target_fd_trans[fd].trans ?
> >> +           target_fd_trans[fd].trans->host_to_target : NULL;
> > 
> > I think if you have to split a ?: expression onto two lines it's
> > a sign it would be clearer as an if ().
> 
> ok
> 
> > 
> >> +}
> >> +
> >> +static void fd_trans_register(int fd, int flags, TargetFdTrans *trans)
> >> +{
> >> +    if (fd >= target_fd_max) {
> >> +        target_fd_max = ((fd >> 6) + 1) << 6; /* by slice of 64 entries */
> >> +        target_fd_trans = g_realloc(target_fd_trans,
> >> +                                    target_fd_max * sizeof(TargetFdEntry));
> > 
> > g_realloc() doesn't zero the extra allocated memory, so you need
> > to do it manually here.
> 
> ok
> 
> >> +    }
> >> +    target_fd_trans[fd].flags = flags;
> >> +    target_fd_trans[fd].trans = trans;
> >> +}
> >> +
> >> +static void fd_trans_unregister(int fd)
> >> +{
> >> +    if (fd < target_fd_max) {
> >> +        target_fd_trans[fd].trans = NULL;
> >> +        target_fd_trans[fd].flags = 0;
> >> +    }
> >> +}
> >> +
> >> +static void fd_trans_dup(int oldfd, int newfd, int flags)
> >> +{
> >> +    fd_trans_unregister(newfd);
> >> +    if (oldfd < target_fd_max && target_fd_trans[oldfd].trans) {
> >> +        fd_trans_register(newfd, flags, target_fd_trans[oldfd].trans);
> >> +    }
> >> +}
> >> +
> >> +static void fd_trans_close_on_exec(void)
> >> +{
> >> +    int fd;
> >> +
> >> +    for (fd = 0; fd < target_fd_max; fd++) {
> >> +        if (target_fd_trans[fd].flags & O_CLOEXEC) {
> >> +            fd_trans_unregister(fd);
> >> +        }
> >> +    }
> >> +}
> > 
> > I think this one's going to turn out to be unneeded -- see
> > comment later on.
> > 
> >> +
> >>  static int sys_getcwd1(char *buf, size_t size)
> >>  {
> >>    if (getcwd(buf, size) == NULL) {
> >> @@ -5246,6 +5308,78 @@ static int do_futex(target_ulong uaddr, int op, int val, target_ulong timeout,
> >>          return -TARGET_ENOSYS;
> >>      }
> >>  }
> >> +#if defined(TARGET_NR_signalfd) || defined(TARGET_NR_signalfd4)
> >> +
> >> +/* signalfd siginfo conversion */
> >> +
> >> +static void
> >> +host_to_target_signalfd_siginfo(struct signalfd_siginfo *tinfo,
> >> +                                const struct signalfd_siginfo *info)
> >> +{
> >> +    int sig = host_to_target_signal(info->ssi_signo);
> >> +    tinfo->ssi_signo = tswap32(sig);
> >> +    tinfo->ssi_errno = tswap32(tinfo->ssi_errno);
> >> +    tinfo->ssi_code = tswap32(info->ssi_code);
> >> +    tinfo->ssi_pid =  tswap32(info->ssi_pid);
> >> +    tinfo->ssi_uid =  tswap32(info->ssi_uid);
> >> +    tinfo->ssi_fd =  tswap32(info->ssi_fd);
> >> +    tinfo->ssi_tid =  tswap32(info->ssi_tid);
> >> +    tinfo->ssi_band =  tswap32(info->ssi_band);
> >> +    tinfo->ssi_overrun =  tswap32(info->ssi_overrun);
> >> +    tinfo->ssi_trapno =  tswap32(info->ssi_trapno);
> >> +    tinfo->ssi_status =  tswap32(info->ssi_status);
> >> +    tinfo->ssi_int =  tswap32(info->ssi_int);
> >> +    tinfo->ssi_ptr =  tswap64(info->ssi_ptr);
> >> +    tinfo->ssi_utime =  tswap64(info->ssi_utime);
> >> +    tinfo->ssi_stime =  tswap64(info->ssi_stime);
> >> +    tinfo->ssi_addr =  tswap64(info->ssi_addr);
> > 
> > Some of these lines have a stray extra space after the '='.
> > 
> > I said in review on v1 that you were missing
> >    tinfo->ssi_addr_lsb = tswap16(info->ssi_addr_lsb);
> > and it's still not here.
> > 
> > Or are you worried about older system include headers not having
> > that field? (looks like it got added to the kernel in 2010 or so).
> > If so we could swap it manually, though that would be a bit tedious.
> 
> My fedora 22 (2015) doesn't have this field in
> /usr/include/sys/signalfd.h, but it is in /usr/include/linux/signalfd.h.
 
> But unfortunately, the first file is the one we use (the second, I
> guess, is for the kernel). Or did I miss something ?

Was a conclusion reached here? A quick codesearch.debian.net search 
doesn't find any userspace code using ssi_addr_lsb.

> >> +}
> >> +
> >> +static abi_long host_to_target_signalfd(void *buf, size_t len)
> >> +{
> >> +    int i;
> >> +
> >> +    for (i = 0; i < len; i += sizeof(struct signalfd_siginfo)) {
> >> +        host_to_target_signalfd_siginfo(buf + i, buf + i);
> >> +    }
> >> +
> >> +    return len;
> >> +}
> >> +
> >> +static TargetFdTrans target_signalfd_trans = {
> >> +    .host_to_target = host_to_target_signalfd,
> >> +};
> >> +
> >> +static abi_long do_signalfd4(int fd, abi_long mask, int flags)
> >> +{
> >> +    int host_flags = flags & (~(TARGET_O_NONBLOCK | TARGET_O_CLOEXEC));
> > 
> > This doesn't look right -- we shouldn't be just passing
> > through target flags we don't recognise. There are only
> > two flags we know about and we should just deal with those,
> > something like:
> > 
> >      if (flags & ~(TARGET_O_NONBLOCK | TARGET_CLOEXEC)) {
> >          return -TARGET_EINVAL;
> >      }
> >      host_flags = target_to_host_bitmask(flags, fcntl_flags_tbl);
> > 
> >> +    target_sigset_t *target_mask;
> >> +    sigset_t host_mask;
> >> +    abi_long ret;
> >> +
> >> +    if (!lock_user_struct(VERIFY_READ, target_mask, mask, 1)) {
> >> +        return -TARGET_EFAULT;
> >> +    }
> >> +
> >> +    target_to_host_sigset(&host_mask, target_mask);
> >> +
> >> +    if (flags & TARGET_O_NONBLOCK) {
> >> +        host_flags |= O_NONBLOCK;
> >> +    }
> >> +    if (flags & TARGET_O_CLOEXEC) {
> >> +        host_flags |= O_CLOEXEC;
> >> +    }
> >> +
> >> +    ret = get_errno(signalfd(fd, &host_mask, host_flags));
> >> +    if (ret >= 0) {
> >> +        fd_trans_register(ret, host_flags, &target_signalfd_trans);
> >> +    }
> >> +
> >> +    unlock_user_struct(target_mask, mask, 0);
> >> +
> >> +    return ret;
> >> +}
> >> +#endif
> > 
> >> @@ -5830,6 +5978,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
> >>                      break;
> >>                  unlock_user(*q, addr, 0);
> >>              }
> >> +            if (ret >= 0) {
> >> +                fd_trans_close_on_exec();
> >> +            }
> > 
> > This is execve, right? We can't possibly get here if exec succeeded...
> 
> You're right...
> 
> > 
> >>          }
> >>          break;
> > 
> > thanks
> 
> Thank you for the review...
> 
> > -- PMM
> > 
>