svn commit: r215664 - in head/sys: compat/linux kern

[Alexander Leidinger]

Quoting Kostik Belousov <kostikbel at 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