[Qemu-devel] [PATCH 0/5] Fixes for linux-user

[Qemu-devel] [PATCH 0/5] Fixes for linux-user

[riku.voipio]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 511 bytes --]

From: Riku Voipio <riku.voipio@iki.fi>

Minor lingering fixes for linux-user usage. Except for the
configure check removal, all have been in the list before.

Arnaud Patard (2):
  linux-user: increment MAX_ARG_PAGES
  linux-user: check some parameters for some socket syscalls.

Riku Voipio (2):
  linux-user: do not avoid dumping of qemu itself
  configure: remove bogus linux-user check

vibi sreenivasan (1):
  linux-user/syscall.c: remove warning: ‘array’ may be used
    uninitialized in this function

[Qemu-devel] [PATCH 1/5] linux-user: increment MAX_ARG_PAGES

[riku.voipio]

From: Arnaud Patard <arnaud.patard@rtp-net.org>

From: Arnaud Patard <arnaud.patard@rtp-net.org>

There's a error When doing something like that :
find / -type f -print0 | xargs -0 echo

[ done in a arm chroot with qemu-arm and linux binfmt stuff or with
find / -type f -print0 | qemu-arm -L <path> <path>/usr/bin/xargs -0
echo ]

Doing this outsite qemu is fine. The problem was the huge number of
parameters. Increasing MAX_ARG_PAGES is fixing that.

While I was at it, I've modified linux-user/main.c to report error code
of loader_exec. It helps to debug/know what's wrong.

Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
Signed-off-by: Riku Voipio <riku.voipio@iki.fi>
---
 linux-user/main.c |    8 +++++---
 linux-user/qemu.h |    2 +-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index 7eabd0c..345d338 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -2372,6 +2372,7 @@ int main(int argc, char **argv, char **envp)
     envlist_t *envlist = NULL;
     const char *argv0 = NULL;
     int i;
+    int ret;
 
     if (argc <= 1)
         usage();
@@ -2576,9 +2577,10 @@ int main(int argc, char **argv, char **envp)
     env->opaque = ts;
     task_settid(ts);
 
-    if (loader_exec(filename, target_argv, target_environ, regs,
-        info, &bprm) != 0) {
-        printf("Error loading %s\n", filename);
+    ret = loader_exec(filename, target_argv, target_environ, regs,
+        info, &bprm);
+    if (ret != 0) {
+        printf("Error %d while loading %s\n", ret, filename);
         _exit(1);
     }
 
diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index e04a31c..83ad443 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -140,7 +140,7 @@ extern const char *qemu_uname_release;
  * and envelope for the new program. 32 should suffice, this gives
  * a maximum env+arg of 128kB w/4KB pages!
  */
-#define MAX_ARG_PAGES 32
+#define MAX_ARG_PAGES 33
 
 /*
  * This structure is used to hold the arguments that are
-- 
1.6.2.1

[Qemu-devel] [PATCH 2/5] linux-user: check some parameters for some socket syscalls.

[riku.voipio]

From: Arnaud Patard <arnaud.patard@rtp-net.org>

From: Arnaud Patard <arnaud.patard@rtp-net.org>

This patch is fixing following issues :

- commit 8fea36025b9d6d360ff3b78f88a84ccf221807e8 was applied to
  do_getsockname instead of do_accept.
- Some syscalls were not checking properly the memory addresses passed
  as argument
- Add check before syscalls made for cases like do_getpeername() where
  we're using the address parameter after doing the syscall
- Fix do_accept to return EINVAL instead of EFAULT when parameters
  invalid to match with linux behaviour

Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
Signed-off-by: Riku Voipio <riku.voipio@iki.fi>
---
 linux-user/syscall.c |   42 ++++++++++++++++++++++++++++++++++--------
 1 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 11564fd..a96e86a 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1498,13 +1498,17 @@ static abi_long do_bind(int sockfd, abi_ulong target_addr,
                         socklen_t addrlen)
 {
     void *addr;
+    abi_long ret;
 
     if (addrlen < 0)
         return -TARGET_EINVAL;
 
     addr = alloca(addrlen+1);
 
-    target_to_host_sockaddr(addr, target_addr, addrlen);
+    ret = target_to_host_sockaddr(addr, target_addr, addrlen);
+    if (ret)
+        return ret;
+
     return get_errno(bind(sockfd, addr, addrlen));
 }
 
@@ -1513,13 +1517,17 @@ static abi_long do_connect(int sockfd, abi_ulong target_addr,
                            socklen_t addrlen)
 {
     void *addr;
+    abi_long ret;
 
     if (addrlen < 0)
         return -TARGET_EINVAL;
 
     addr = alloca(addrlen);
 
-    target_to_host_sockaddr(addr, target_addr, addrlen);
+    ret = target_to_host_sockaddr(addr, target_addr, addrlen);
+    if (ret)
+        return ret;
+
     return get_errno(connect(sockfd, addr, addrlen));
 }
 
@@ -1543,8 +1551,12 @@ static abi_long do_sendrecvmsg(int fd, abi_ulong target_msg,
     if (msgp->msg_name) {
         msg.msg_namelen = tswap32(msgp->msg_namelen);
         msg.msg_name = alloca(msg.msg_namelen);
-        target_to_host_sockaddr(msg.msg_name, tswapl(msgp->msg_name),
+        ret = target_to_host_sockaddr(msg.msg_name, tswapl(msgp->msg_name),
                                 msg.msg_namelen);
+        if (ret) {
+            unlock_user_struct(msgp, target_msg, send ? 0 : 1);
+            return ret;
+        }
     } else {
         msg.msg_name = NULL;
         msg.msg_namelen = 0;
@@ -1586,12 +1598,19 @@ static abi_long do_accept(int fd, abi_ulong target_addr,
     void *addr;
     abi_long ret;
 
+    if (target_addr == 0)
+       return get_errno(accept(fd, NULL, NULL));
+
+    /* linux returns EINVAL if addrlen pointer is invalid */
     if (get_user_u32(addrlen, target_addrlen_addr))
-        return -TARGET_EFAULT;
+        return -TARGET_EINVAL;
 
     if (addrlen < 0)
         return -TARGET_EINVAL;
 
+    if (!access_ok(VERIFY_WRITE, target_addr, addrlen))
+        return -TARGET_EINVAL;
+
     addr = alloca(addrlen);
 
     ret = get_errno(accept(fd, addr, &addrlen));
@@ -1617,6 +1636,9 @@ static abi_long do_getpeername(int fd, abi_ulong target_addr,
     if (addrlen < 0)
         return -TARGET_EINVAL;
 
+    if (!access_ok(VERIFY_WRITE, target_addr, addrlen))
+        return -TARGET_EFAULT;
+
     addr = alloca(addrlen);
 
     ret = get_errno(getpeername(fd, addr, &addrlen));
@@ -1636,15 +1658,15 @@ static abi_long do_getsockname(int fd, abi_ulong target_addr,
     void *addr;
     abi_long ret;
 
-    if (target_addr == 0)
-       return get_errno(accept(fd, NULL, NULL));
-
     if (get_user_u32(addrlen, target_addrlen_addr))
         return -TARGET_EFAULT;
 
     if (addrlen < 0)
         return -TARGET_EINVAL;
 
+    if (!access_ok(VERIFY_WRITE, target_addr, addrlen))
+        return -TARGET_EFAULT;
+
     addr = alloca(addrlen);
 
     ret = get_errno(getsockname(fd, addr, &addrlen));
@@ -1688,7 +1710,11 @@ static abi_long do_sendto(int fd, abi_ulong msg, size_t len, int flags,
         return -TARGET_EFAULT;
     if (target_addr) {
         addr = alloca(addrlen);
-        target_to_host_sockaddr(addr, target_addr, addrlen);
+        ret = target_to_host_sockaddr(addr, target_addr, addrlen);
+        if (ret) {
+            unlock_user(host_msg, msg, 0);
+            return ret;
+        }
         ret = get_errno(sendto(fd, host_msg, len, flags, addr, addrlen));
     } else {
         ret = get_errno(send(fd, host_msg, len, flags));
-- 
1.6.2.1

[Qemu-devel] [PATCH 3/5] linux-user: do not avoid dumping of qemu itself

[riku.voipio]

From: Riku Voipio <riku.voipio@iki.fi>

Previously we disabled dumping of qemu itself if we already
created coredump of the target process. This broke the abort01
LTP test and any application that used WCOREDUMP to find out
the child process dumped core. Remove this feature.

Signed-off-by: Riku Voipio <riku.voipio@iki.fi>
---
 linux-user/signal.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 6a34171..e1b6458 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -383,12 +383,6 @@ static void QEMU_NORETURN force_sig(int sig)
             ((*ts->bprm->core_dump)(sig, thread_env) == 0);
     }
     if (core_dumped) {
-        /* we already dumped the core of target process, we don't want
-         * a coredump of qemu itself */
-        struct rlimit nodump;
-        getrlimit(RLIMIT_CORE, &nodump);
-        nodump.rlim_cur=0;
-        setrlimit(RLIMIT_CORE, &nodump);
         (void) fprintf(stderr, "qemu: uncaught target signal %d (%s) - %s\n",
             sig, strsignal(host_sig), "core dumped" );
     }
-- 
1.6.2.1

Re: [Qemu-devel] [PATCH 3/5] linux-user: do not avoid dumping of qemu itself

[Paul Brook]

On Tuesday 30 June 2009, riku.voipio@iki.fi wrote:
> From: Riku Voipio <riku.voipio@iki.fi>
>
> Previously we disabled dumping of qemu itself if we already
> created coredump of the target process. This broke the abort01
> LTP test and any application that used WCOREDUMP to find out
> the child process dumped core. Remove this feature.

Isn't this going to overwrite the guest core dump we just wrote?

It sounds like WCOREDUMP is one of those things that just isn't going to work. 
i.e. we have to just accept the limitation and xfail the test.

Paul

Re: [Qemu-devel] [PATCH 3/5] linux-user: do not avoid dumping of qemu itself

[Riku Voipio]

On Tue, Jun 30, 2009 at 03:52:12PM +0100, Paul Brook wrote:
> On Tuesday 30 June 2009, riku.voipio@iki.fi wrote:
> > From: Riku Voipio <riku.voipio@iki.fi>
> >
> > Previously we disabled dumping of qemu itself if we already
> > created coredump of the target process. This broke the abort01
> > LTP test and any application that used WCOREDUMP to find out
> > the child process dumped core. Remove this feature.

> Isn't this going to overwrite the guest core dump we just wrote?

No, since we make the guest dump as qemu_binary_date-time_pid.core,
while host core will be core or whatever is /proc/sys/kernel/core_pattern.
However, if we replace our adhoc naming with reading core_pattern, we
would indeed get overwritten.

> It sounds like WCOREDUMP is one of those things that just isn't going to work.
> i.e. we have to just accept the limitation and xfail the test.

If this how people feel, I'll be glad to drop this patch.

Riku

Re: [Qemu-devel] [PATCH 3/5] linux-user: do not avoid dumping of qemu itself

[Arnaud Patard]

Riku Voipio <riku.voipio@iki.fi> writes:

Hi,

>> It sounds like WCOREDUMP is one of those things that just isn't going to work.
>> i.e. we have to just accept the limitation and xfail the test.
>
> If this how people feel, I'll be glad to drop this patch.

well, as I said either we fix it or we accept that the emulation is not
perfect and judge that this feature is worth it. Feel free to choose
what you feel is best, I don't care that much about it. What I care is
to know if what looks like a bug (here some failures in ltp syscall tests)
is a bug or a feature :)

Arnaud

Re: [Qemu-devel] [PATCH 3/5] linux-user: do not avoid dumping of qemu itself

[Paul Brook]

> >> It sounds like WCOREDUMP is one of those things that just isn't going to
> >> work. i.e. we have to just accept the limitation and xfail the test.
> >
> > If this how people feel, I'll be glad to drop this patch.
>
> well, as I said either we fix it or we accept that the emulation is not
> perfect and judge that this feature is worth it.

It's not quite that simple.

I think this is a case of someone blindly "fixing" a testsuite without any 
consideration of what is actually being implemented.

IMO a host core dump is for most purposes useless[1], and dumping guest state 
to a different location is a bug.  Given the choice between dumping guest core 
in the "normal" location and setting the WCOREDUMP flag, the former seems much 
more useful.

Paul

[1] A host core dump may be useful for debugging qemu itself, but that's a 
fairly specialized corner case, and not necessarily something we want to be 
exposing to users.

Re: [Qemu-devel] [PATCH 3/5] linux-user: do not avoid dumping of qemu itself

[Jamie Lokier]

Paul Brook wrote:
> > >> It sounds like WCOREDUMP is one of those things that just isn't going to
> > >> work. i.e. we have to just accept the limitation and xfail the test.
> > >
> > > If this how people feel, I'll be glad to drop this patch.
> >
> > well, as I said either we fix it or we accept that the emulation is not
> > perfect and judge that this feature is worth it.
> 
> It's not quite that simple.
> 
> I think this is a case of someone blindly "fixing" a testsuite without any 
> consideration of what is actually being implemented.
> 
> IMO a host core dump is for most purposes useless[1], and dumping
> guest state to a different location is a bug.  Given the choice
> between dumping guest core in the "normal" location and setting the
> WCOREDUMP flag, the former seems much more useful.

It's a really minor corner case, but I wonder if it's worth adding a
prctl() option or some bit in the info passed to sigqueue(), to the
Linux kernel to set the WCOREDUMP flag on exit.  Meaning "app has
coredumped itself".

> [1] A host core dump may be useful for debugging qemu itself, but that's a 
> fairly specialized corner case, and not necessarily something we want to be 
> exposing to users.

It would make sense to set RLIMIT_CORE to zero very early in
qemu-user, and then someone debugging qemu-user can easily change
RLIMIT_CORE from gdb while it is running.

-- Jamie

Re: [Qemu-devel] [PATCH 3/5] linux-user: do not avoid dumping of qemu itself

[Paul Brook]

> > [1] A host core dump may be useful for debugging qemu itself, but that's
> > a fairly specialized corner case, and not necessarily something we want
> > to be exposing to users.
>
> It would make sense to set RLIMIT_CORE to zero very early in
> qemu-user, and then someone debugging qemu-user can easily change
> RLIMIT_CORE from gdb while it is running.

Sounds reasonable, as long as you're careful to avoid breaking guest 
applications that call {get,set}rlimit(RLIMIT_CORE).

Paul

Re: [Qemu-devel] [PATCH 3/5] linux-user: do not avoid dumping of qemu itself

[Riku Voipio]

On Thu, Jul 02, 2009 at 02:19:29PM +0100, Paul Brook wrote:
> > > [1] A host core dump may be useful for debugging qemu itself, but that's
> > > a fairly specialized corner case, and not necessarily something we want
> > > to be exposing to users.

> > It would make sense to set RLIMIT_CORE to zero very early in
> > qemu-user, and then someone debugging qemu-user can easily change
> > RLIMIT_CORE from gdb while it is running.

> Sounds reasonable, as long as you're careful to avoid breaking guest 
> applications that call {get,set}rlimit(RLIMIT_CORE).

The host process coredump is still not very useful, as kernel creates it
only after we come out of our signal handler? Also, I don't think there
is any real world application that would behave unexpectedly if one
of it's child processes dies without the coredump bit being set...

For now, I'll drop this patch and ask pulling the rest in.

Re: [Qemu-devel] [PATCH 3/5] linux-user: do not avoid dumping of qemu itself

[Jamie Lokier]

Riku Voipio wrote:
> On Thu, Jul 02, 2009 at 02:19:29PM +0100, Paul Brook wrote:
> > > > [1] A host core dump may be useful for debugging qemu itself, but that's
> > > > a fairly specialized corner case, and not necessarily something we want
> > > > to be exposing to users.
> 
> > > It would make sense to set RLIMIT_CORE to zero very early in
> > > qemu-user, and then someone debugging qemu-user can easily change
> > > RLIMIT_CORE from gdb while it is running.
> 
> > Sounds reasonable, as long as you're careful to avoid breaking guest 
> > applications that call {get,set}rlimit(RLIMIT_CORE).
> 
> The host process coredump is still not very useful, as kernel creates it
> only after we come out of our signal handler?

The kernel will create the host coredump when the host process dies.
That is, when it receives a fatal unhandled signal.

Generally I'd expect we shouldn't create a coredump if QEMU sends
itself a signal deliberately, just to indicate that the process
terminates with a guest signal.  But it would be a bit more useful
when QEMU receives a signal due to a bug in QEMU (when debugging sets
RLIMIT_CORE).  There might not be a clean way to distinguish the two
conditions.

> Also, I don't think there
> is any real world application that would behave unexpectedly if one
> of it's child processes dies without the coredump bit being set...

I agree, I've never heard of any.  Quite a few will behave
differently, in a minor way, such as the different termination message
shown by shells.

-- Jamie

Re: [Qemu-devel] [PATCH 3/5] linux-user: do not avoid dumping of qemu itself

[Jamie Lokier]

Paul Brook wrote:
> > > [1] A host core dump may be useful for debugging qemu itself, but that's
> > > a fairly specialized corner case, and not necessarily something we want
> > > to be exposing to users.
> >
> > It would make sense to set RLIMIT_CORE to zero very early in
> > qemu-user, and then someone debugging qemu-user can easily change
> > RLIMIT_CORE from gdb while it is running.
> 
> Sounds reasonable, as long as you're careful to avoid breaking guest 
> applications that call {get,set}rlimit(RLIMIT_CORE).

They should not be sent to the host, but affect guest core dumping
instead.  (Um, probably).

-- Jamie

[Qemu-devel] [PATCH 4/5] linux-user/syscall.c: remove warning: ‘array’ may be used uninitialized in this function

[riku.voipio]

From: vibi sreenivasan <vibi_sreenivasan@cms.com>

From: vibi sreenivasan <vibi_sreenivasan@cms.com>

Removes the following warning

CC    i386-linux-user/syscall.o
cc1: warnings being treated as errors
/media/nfs/qemu/linux-user/syscall.c: In function ‘do_syscall’:
/media/nfs/qemu/linux-user/syscall.c:2219: warning: ‘array’ may be used uninitialized in this function

Signed-off-by: Vibi Sreenivasan <vibi_sreenivasan@cms.com>
Signed-off-by: Riku Voipio <riku.voipio@iki.fi>
---
 linux-user/syscall.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index a96e86a..6aaf9ca 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -2241,7 +2241,7 @@ static inline abi_long do_semctl(int semid, int semnum, int cmd,
 {
     union semun arg;
     struct semid_ds dsarg;
-    unsigned short *array;
+    unsigned short *array = NULL;
     struct seminfo seminfo;
     abi_long ret = -TARGET_EINVAL;
     abi_long err;
-- 
1.6.2.1

[Qemu-devel] [PATCH 5/5] configure: remove bogus linux-user check

[riku.voipio]

From: Riku Voipio <riku.voipio@iki.fi>

linux-user=yes is not a reliable identifier that linux-user targets
have been selected. user targets can be selected via --target-list
as well.

Signed-off-by: Riku Voipio <riku.voipio@iki.fi>
---
 configure |   16 ++++++----------
 1 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/configure b/configure
index 33e3c41..caa5a52 100755
--- a/configure
+++ b/configure
@@ -1245,8 +1245,7 @@ fi
 # have syscall stubs for these implemented.
 #
 atfile=no
-if [ "$linux_user" = "yes" ] ; then
-  cat > $TMPC << EOF
+cat > $TMPC << EOF
 #define _ATFILE_SOURCE
 #include <sys/types.h>
 #include <fcntl.h>
@@ -1259,9 +1258,8 @@ main(void)
 	return (unlinkat(AT_FDCWD, "nonexistent_file", 0));
 }
 EOF
-  if $cc $ARCH_CFLAGS -o $TMPE $TMPC 2> /dev/null > /dev/null ; then
-    atfile=yes
-  fi
+if $cc $ARCH_CFLAGS -o $TMPE $TMPC 2> /dev/null > /dev/null ; then
+  atfile=yes
 fi
 
 # Check for inotify functions when we are building linux-user
@@ -1270,8 +1268,7 @@ fi
 # don't provide them even if kernel supports them.
 #
 inotify=no
-if [ "$linux_user" = "yes" ] ; then
-  cat > $TMPC << EOF
+cat > $TMPC << EOF
 #include <sys/inotify.h>
 
 int
@@ -1281,9 +1278,8 @@ main(void)
 	return inotify_init();
 }
 EOF
-  if $cc $ARCH_CFLAGS -o $TMPE $TMPC 2> /dev/null > /dev/null ; then
-    inotify=yes
-  fi
+if $cc $ARCH_CFLAGS -o $TMPE $TMPC 2> /dev/null > /dev/null ; then
+  inotify=yes
 fi
 
 # check if utimensat and futimens are supported
-- 
1.6.2.1