Re: updating COMPAT_LINUX for linux 2.6.x support (take 2)

[Chuck Silvers <chuq%chuq.com@localhost>]

On Mon, Jun 14, 2010 at 03:25:56PM +0000, Andrew Doran wrote:
> On Sun, Jun 13, 2010 at 10:59:45PM -0700, Chuck Silvers wrote:
> > hi folks,
> > 
> > ok, more progress.  linux32 is working now and I fixed a few other bugs
> > along the way.
> > 
> > the updated diff is in
> > ftp://ftp.netbsd.org/pub/NetBSD/misc/chs/linux/diff.linux-nptl-take2.36
> 
> - mips pcb_tls. Can you use curlwp->l_private instead and add a 
>   cpu_lwp_setprivate() ala i386 to handle this?  As it would be the same
>   mechanism that we'd use for native TLS.  There is a __HAVE flag for
>   this in machine/types.h as far as I remember, see sys_lwp.c.  I created
>   patches for a bunch of other architectures to do this, mjf@ is sitting
>   on them I think.

ok, I did that for all the platforms where I added TLS code for linux.
actually, I created an lwp_setprivate() which sets l->l_private and calls
cpu_lwp_setprivate() if there is one, and changed all the linux TLS code
and sys__lwp_setprivate() to use that.  for mips, there isn't any
hardware register so we don't need a cpu_lwp_setprivate() there.


> - In x86 sys_machdep.c, I'd feel better if wrmsr() and set of pcb_gs etc 
>   were bracketed with kpreempt_disable()/kpreempt_enable().  Likewise
>   memcpy() to pcb_gsd and friends in Linux compat code.
> 
> - For the Linux compat setting of %gs/%fs, I'd rather this was done via
>   a function call into native x86 code because in the past we've ended
>   up with stragglers in this code, where someone working on compat does
>   the wrong thing or where someone working on x86 fails to update the
>   compat code. Not a strong opinion just a preference.

with the above change to use lwp_setprivate(), the linux code is now
free of anything that fiddles with TLS thread state directly.


> - FYI I think I disabled Linux ptrace() because I was concerned about
>   potential security issues and bitrot in the code. Dunno if that's
>   still the case.

you're right, more would be needed there to be safe.
I put the checks to disallow ptrace on multi-LWP processes
back the way they were (and added one on powerpc, where it was missing).


> - Re: the Linux +ucas_int() hack, preemption implies MULTIPROCESSOR
>   so the kpreempt toggles aren't needed.. Maybe worthwhile as a sort
>   of documentation though.
>   We may context switch during the copyout so I don't see how this
>   can be atomic.  If copyin() is somehow wacky I guess we could switch
>   there too.  Any reason not to say "implement user space CAS or your
>   port loses Linux emulation"?

interesting, I didn't realize that preemption wasn't supported on
non-MP kernels.

I think the hacky version is actually safe.  the copyin() will have
fetched the page into memory and created a pmap mapping for it.
in a UP kernel, nothing can happen between the copyin() and the copyout()
that could change the page's contents or invalidate the mapping,
so the copyout() can't fault and the value that copyin() read can't
have changed before the copyout().

as for just disabling COMPAT_LINUX on these platforms,
I really don't like disabling other people's features
just because it would be convenient for me.


> - The dup code for fork1() code makes me uncomfortable.  Maybe it's
>   worthwhile changing our native code so that LIDs are always allocated
>   from the PID table or something along those lines?  Tend to think these
>   should be globally unique with the system and not just within a process.
>   Could also be of potential help with things like inter-process pthread
>   objects in shared memory.

could you be more specific about the duplicated code that concerns you?
do you mean the "Set the new LWP running" chunk at the end of
linux_clone_nptl()?  that's copied from sys__lwp_create() rather than
fork1(), BTW.

there are 7 callers (8 with my new one) of lwp_create(),
and all of them have different code for making the new LWP runnable
(or not) afterward.  do you suppose it would be worthwhile to
collect all of that together into an lwp_launch() or somesuch,
with a bunch of flags to select the behaviour?

I'd rather not get into changing LID allocation behaviour of native
processes as part of this linux work.


> - When resetting l_lid, for safety we should hold p_lock (allthough 
>   during early fork we'd probably get away with it due to the process
>   being SIDL).
>   If we take an approach like the above then we wouldn't need to reset
>   l_lid at all.

I added a flag to lwp_create() to have it allocate a new PID to use for
the LID of the new thread.  now the only places that the LID of an existing
LWP is changed are in execve1() (which already did that) and in the
linux fork and exec callbacks (at which point the process has only 1 LWP).
do we need to take p_lock in these places?


> > there is one odd thing that I haven't been able to figure out:
> > I don't think we should need to reload %gs in INTRFASTEXIT on amd64
> > since swapgs will put back the user's gs.base value.
> > but if I take out the reload of %gs and run the 32-bit pthread tests,
> > within a few minutes the system will either reset or lock up.
> > anyone have any idea why this would be?  I'd appreciate it if
> > someone could look over the asm code changes in general.
> 
> No clear idea about the %gs issue, sorry.

well, it doesn't break anything that's currently working
and it won't cause a problem for TLS, so I'll just leave it
that the way I have it for now.


the updated diff is at:
ftp://ftp.netbsd.org/pub/NetBSD/misc/chs/linux/diff.linux-nptl-take2.39


-Chuck