Quoting Kostik Belousov <kostik...@gmail.com> (from Mon, 22 Nov 2010 11:31:34 +0200):

On Mon, Nov 22, 2010 at 09:07:00AM +0000, Alexander Leidinger wrote:
Author: netchild
Date: Mon Nov 22 09:06:59 2010
New Revision: 215664
URL: http://svn.freebsd.org/changeset/base/215664

Log:
  By using the 32-bit Linux version of Sun's Java Development Kit 1.6
  on FreeBSD (amd64), invocations of "javac" (or "java") eventually
  end with the output of "Killed" and exit code 137.


@@ -196,6 +198,12 @@ linux_proc_exit(void *arg __unused, stru
        } else
                EMUL_SHARED_WUNLOCK(&emul_shared_lock);

+       if ((shared_flags & EMUL_SHARED_HASXSTAT) != 0) {
+               PROC_LOCK(p);
+               p->p_xstat = shared_xstat;
+               PROC_UNLOCK(p);
+       }
Why is process lock taken there ? The assignment to u_short inside the
properly aligned structure is atomic on all supported architectures, and
the thread that should see side-effect of assignment is the same thread
that does assignment.

Change below.

+
        if (child_clear_tid != NULL) {
                struct linux_sys_futex_args cup;
                int null = 0;
@@ -257,6 +265,9 @@ linux_proc_exec(void *arg __unused, stru
        if (__predict_false(imgp->sysent == &elf_linux_sysvec
            && p->p_sysent != &elf_linux_sysvec))
                linux_proc_init(FIRST_THREAD_IN_PROC(p), p->p_pid, 0);
+       if (__predict_false(p->p_sysent == &elf_linux_sysvec))
+               /* Kill threads regardless of imgp->sysent value */
+               linux_kill_threads(FIRST_THREAD_IN_PROC(p), SIGKILL);
This is better expressed by
        if ((p->p_sysent->sv_flags & SV_ABI_MASK) == SV_ABI_LINUX)

Is this OK for you?
---snip---
Index: compat/linux/linux_emul.c
===================================================================
--- compat/linux/linux_emul.c   (Revision 215664)
+++ compat/linux/linux_emul.c   (Arbeitskopie)
@@ -198,11 +198,8 @@
        } else
                EMUL_SHARED_WUNLOCK(&emul_shared_lock);

-       if ((shared_flags & EMUL_SHARED_HASXSTAT) != 0) {
-               PROC_LOCK(p);
+       if ((shared_flags & EMUL_SHARED_HASXSTAT) != 0)
                p->p_xstat = shared_xstat;
-               PROC_UNLOCK(p);
-       }

        if (child_clear_tid != NULL) {
                struct linux_sys_futex_args cup;
@@ -265,7 +262,8 @@
        if (__predict_false(imgp->sysent == &elf_linux_sysvec
            && p->p_sysent != &elf_linux_sysvec))
                linux_proc_init(FIRST_THREAD_IN_PROC(p), p->p_pid, 0);
-       if (__predict_false(p->p_sysent == &elf_linux_sysvec))
+       if (__predict_false((p->p_sysent->sv_flags & SV_ABI_MASK) ==
+           SV_ABI_LINUX))
                /* Kill threads regardless of imgp->sysent value */
                linux_kill_threads(FIRST_THREAD_IN_PROC(p), SIGKILL);
        if (__predict_false(imgp->sysent != &elf_linux_sysvec
---snip---

Regardless of this mostly cosmetic issue, this is racy. Other
linux thread in the same process might do an execve(3).
More, if execve(3) call fails, then you return into the process
that lacks all threads except the one that called execve(3).

How critical is this in your opinion (relative to the issue this patch is fixing)? Do you prefer a backout or do you think the probability that the someone wins the race is low enough?

Do you see a solution for the race?

Bye,
Alexander.

--
http://www.Leidinger.net  Alexander @ Leidinger.net: PGP ID = B0063FE7
http://www.FreeBSD.org     netchild @ FreeBSD.org  : PGP ID = 72077137
This generation doesn't have emotional baggage.
We have emotional moving vans.
                -- Bruce Feirstein

_______________________________________________
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Reply via email to