Replace exect() with a portable version

[ali_mashtizadeh.com]

Summary

Originally, on the VAX exect() enable tracing once the new executable image
was loaded. This was possible because tracing was controllable through
user space code by setting the PSL_T flag. The following instruction is a
system call that activated tracing (as all instructions do) by copying PSL_T
to PSL_TP (trace pending). The first instruction of the new executable image
would trigger a trace fault.

This is not portable to all platforms and the behavior was replaced with
ptrace(PT_TRACE_ME, ...) since FreeBSD forked off of the CSRG repository.
Platforms either incorrectly call execve(), trigger trace faults inside
the original executable, or do contain an implementation of this function.
The system call has been similarly made deprecated (NetBSD and OpenBSD)
or removed (OS X) in other platforms.

Test Plan

make universe, view man page

[ali_mashtizadeh.com]

• universe_i386_i386 ---

i386.i386 buildworld completed on Fri Apr 6 04:52:06 EDT 2018

• universe_i386_kernels ---
• universe_kernconf_i386_GENERIC ---
• universe_kernconf_i386_GENERIC-NODEBUG ---
• universe_kernconf_i386_LINT ---
• universe_kernconf_i386_LINT-NOINET ---
• universe_kernconf_i386_LINT-NOINET6 ---
• universe_kernconf_i386_LINT-NOIP ---
• universe_kernconf_i386_PAE ---
• universe_arm64_aarch64 ---

arm64.aarch64 buildworld completed on Fri Apr 6 04:57:10 EDT 2018

• universe_arm64_kernels ---
• universe_kernconf_arm64_GENERIC ---
• universe_kernconf_arm64_GENERIC-NODEBUG ---
• universe_kernconf_arm64_GENERIC-UP ---
• universe_amd64_done ---

amd64 completed on Fri Apr 6 05:16:08 EDT 2018

• universe_arm64_done ---

arm64 completed on Fri Apr 6 05:35:21 EDT 2018

• universe_i386_done ---

i386 completed on Fri Apr 6 05:43:02 EDT 2018

• universe_epilogue --- --------------------------------------------------------------

make universe completed on Fri Apr 6 05:43:02 EDT 2018

(started Thu Apr  5 20:37:43 EDT 2018)

---

[ali_mashtizadeh.com]

This is the last bit of cleanup I sent a mail about this on the mailing list to look for objections. I will either today or tomorrow send all three of you a sketch of my plans for libsys.

[ali_mashtizadeh.com]

The netbsd test suite contains a test for exect but that should go away whenever someone pulls the latest version.

See contrib/netbsd-tests/lib/libc/gen/exect/t_exect.c

[brooks]

In D14989#315333, @ali_mashtizadeh.com wrote:

• universe_i386_i386 ---

It looks like you are running make universe. One thing to note is that you must read the output files to determine status. If you replace universe with tinderbox in every command it will tell you which ones failed.

[rgrimes]

What are the plans/methods to be used for stable/11 informing consumers that this feature is being deprecated?

[brooks]

lib/libc/gen/Symbol.map

108 ↗

(On Diff #41182)

_exect and __sys_exect symbols would also need to be provided since they were once exposed.

lib/libc/gen/exec.3

206 ↗

(On Diff #41182)

Line break after . and after 2 in the next line. You'd also want to bump the doc data .Dd above.

lib/libc/gen/exect.c

39 ↗

(On Diff #41182)

Would it make sense to to call ptrace(PT_TRACE_ME,...); here or is the result not the same.

Also, a weird style(9) nit. There should be a blank line between the (nonexistant) arguments and the return.

[andrew]

Doesn't this add it on architectures where it wasn't implemented? It doesn't seem to be implemented on arm, arm64, or riscv. It's implemented on mips, but is not in the Symbols.map file so will not be exposed in libc.so.7 and could be removed.

[kib]

I must note that i386 and amd64 (both native and compat32) execve(2) syscalls explicitly support inheriting PSL_T from the previosly executing program to the new one. So exect() behaves as designed, assuming the tracing faults fo not kill the process.

lib/libc/gen/Symbol.map

108 ↗

(On Diff #41182)

If the symbols were not exported from the public namespace FBSD_X.Y, there is no need to bother. FBSDprivate_1.0 is not supposed to be ABI stable.

lib/libc/gen/exect.c

40 ↗

(On Diff #41182)

Also, return (execve());.

[ali_mashtizadeh.com]

In D14989#315376, @kib wrote:

I must note that i386 and amd64 (both native and compat32) execve(2) syscalls explicitly support inheriting PSL_T from the previosly executing program to the new one. So exect() behaves as designed, assuming the tracing faults fo not kill the process.

On the i386/amd64 the first fault is on the system call instruction rather than the first instruction of the new image. If we switch to calling ptrace(PT_TRACE_ME, ...) + execve it will more closely emulate the original behavior on the VAX.

lib/libc/gen/exect.c

39 ↗

(On Diff #41182)

This was one of my suggestions on my email. It more closely matches the behavior of exect, except for the additional error cases.

[ali_mashtizadeh.com]

• Address feedback

Now exect() calls ptrace(PT_TRACE_ME, ...) and I fixed the style as best as I
could. Wasn't sure if the extra newline I put is okay.

Updated man page

[ali_mashtizadeh.com]

I'm waiting for make tinderbox to complete.

With this approach an application that used both ptrace(PT_TRACE_ME,...) and exect() will get an EBUSY error code. Otherwise this nearly replicates the documented/original intended behavior.

[ali_mashtizadeh.com]

@rgrimes With this version of the patch do we need to do anything about stable/11? The behavior now should most closely match the documentation.

@andrew It is exposed in unistd.h and the documentation doesn't say it's specific to a particular platform.

Just a reminder about the netbsd test case since no one responded to that.

[rgrimes]

In D14989#315495, @ali_mashtizadeh.com wrote:

@rgrimes With this version of the patch do we need to do anything about stable/11? The behavior now should most closely match the documentation.

It still appears that you deprecated a function, you can not just do that in stable/11 as that breaks ABI It is also desirable to have a change to stable/11 that makes use of this function emit a deprecation warning.
It may be possible to wrap this change in #if FreeBSD_Version < 12000xxx #else #endif and have code that can be merged with removal of the ugly #ifdef flagged for after stable/11 eol.
Or a direct commit to stable/11 only doing the notice of deprecation can be done.
I am okay with this change landing in -current as it stands, but it can not be MFC'ed, nor has proper deprecation notice been done.

[kib]

In D14989#315498, @rgrimes wrote:

It still appears that you deprecated a function, you can not just do that in stable/11 as that breaks ABI It is also desirable to have a change to stable/11 that makes use of this function emit a deprecation warning.

What ABI does it break ?

[brooks]

lib/libc/gen/exect.c

37 ↗

(On Diff #41200)

This doesn't tell the programmer who runs into this (someone with -Wl,--fatal-warnings set) anything useful. I wonder if it makes sense to add an example conversion to the manpage and refer to it.

43 ↗

(On Diff #41200)

Maybe ignore EBUSY here?

[rgrimes]

In D14989#315514, @kib wrote:

In D14989#315498, @rgrimes wrote:

It still appears that you deprecated a function, you can not just do that in stable/11 as that breaks ABI It is also desirable to have a change to stable/11 that makes use of this function emit a deprecation warning.

What ABI does it break ?

Doesnt it remove exect() from libc? Or am I missing that we would still have a 100% call compatible exect() in libc after this change?

[kib]

In D14989#315520, @rgrimes wrote:

Doesnt it remove exect() from libc? Or am I missing that we would still have a 100% call compatible exect() in libc after this change?

No, it does not. New exect() implementation does not core dumps if the process is not traced and not installed SIGTRAP handler, instead it reports the debug events to the parent.

[ali_mashtizadeh.com]

Well I made a few test cases to verify to myself all the differences and it turns out exect() doesn't even work on 11. It generates a trace fault in the original image and then stops generating faults in the new image.

exec_setregs() zeros out the registers then tries to maintain the value of regs->tf_eflags. Nothing in eflags appears to be preserved.
https://github.com/freebsd/freebsd/blob/master/sys/amd64/amd64/machdep.c#L596
https://github.com/freebsd/freebsd/blob/master/sys/i386/i386/machdep.c#L1130

https://svnweb.freebsd.org/base/head/sys/i386/i386/machdep.c?r1=4193&r2=4201

Unless something else has changed it seems we have a 23 year old regression?

[ali_mashtizadeh.com]

In D14989#315532, @ali_mashtizadeh.com wrote:

Well I made a few test cases to verify to myself all the differences and it turns out exect() doesn't even work on 11. It generates a trace fault in the original image and then stops generating faults in the new image.

exec_setregs() zeros out the registers then tries to maintain the value of regs->tf_eflags. Nothing in eflags appears to be preserved.
https://github.com/freebsd/freebsd/blob/master/sys/amd64/amd64/machdep.c#L596
https://github.com/freebsd/freebsd/blob/master/sys/i386/i386/machdep.c#L1130

https://svnweb.freebsd.org/base/head/sys/i386/i386/machdep.c?r1=4193&r2=4201

Unless something else has changed it seems we have a 23 year old regression?

Should I remove the dead kernel code as part of this change?

[rgrimes]

Could it be this works on i386 only?

[brooks]

In D14989#315535, @rgrimes wrote:

Could it be this works on i386 only?

Nope, it's totally broken. Here's i386 code Ali pointed to:

	bzero((char *)regs, sizeof(struct trapframe));
	regs->tf_eip = imgp->entry_addr;
	regs->tf_esp = stack;
	regs->tf_eflags = PSL_USER | (regs->tf_eflags & PSL_T);

[ali_mashtizadeh.com]

• Remove the dead expression from i386/amd64 machdep

[brooks]

In D14989#315533, @ali_mashtizadeh.com wrote:

In D14989#315532, @ali_mashtizadeh.com wrote:

Well I made a few test cases to verify to myself all the differences and it turns out exect() doesn't even work on 11. It generates a trace fault in the original image and then stops generating faults in the new image.

exec_setregs() zeros out the registers then tries to maintain the value of regs->tf_eflags. Nothing in eflags appears to be preserved.
https://github.com/freebsd/freebsd/blob/master/sys/amd64/amd64/machdep.c#L596
https://github.com/freebsd/freebsd/blob/master/sys/i386/i386/machdep.c#L1130

https://svnweb.freebsd.org/base/head/sys/i386/i386/machdep.c?r1=4193&r2=4201

Unless something else has changed it seems we have a 23 year old regression?

Should I remove the dead kernel code as part of this change?

Yeah, we might as well remove the pointless code that has never worked.

Given that exect() has not worked as documented for any in-tree architecture since at least 1994 I think it's safe to provide a good-faith implementation of the intent and move on.

[kib]

In D14989#315545, @brooks wrote:

In D14989#315533, @ali_mashtizadeh.com wrote:

Should I remove the dead kernel code as part of this change?

Yeah, we might as well remove the pointless code that has never worked.

I was sure that I already fixed the issue. It was pointed out some time ago to me by bde. See D14995.

[ali_mashtizadeh.com]

Backout kernel as kib is taking care of this.

[ali_mashtizadeh.com]

• Ignore EBUSY errno

[ali_mashtizadeh.com]

Running make tinderbox. I've backout the kernel change and added the code to ignore EBUSY.

Should be done if everyone is happy!

[ali_mashtizadeh.com]

• Ignore EBUSY errno

[andrew]

In D14989#315495, @ali_mashtizadeh.com wrote:

@andrew It is exposed in unistd.h and the documentation doesn't say it's specific to a particular platform.

Then we should fix the header and documentation rather than adding a deprecated function on architectures where it never existed.

[ali_mashtizadeh.com]

With this version of the code the functionality matches the documentation. We could remove the warn_references and leave the manual suggestion that this functionality has been replaced by ptrace. I left the warning from the NetBSD code but there's not much point if we have something that works.

[kib]

In D14989#315748, @ali_mashtizadeh.com wrote:

With this version of the code the functionality matches the documentation. We could remove the warn_references and leave the manual suggestion that this functionality has been replaced by ptrace. I left the warning from the NetBSD code but there's not much point if we have something that works.

I agree that the warning is pointless. More, I am not sure why do we need to put this function into obsoleted category. It is implemented using existing interfaces which are not going away. I do not see why other arches cannot get this functionality due to the cleanup.

[ali_mashtizadeh.com]

• Remove the warning message

[ali_mashtizadeh.com]

--- universe_kernconf_mips_PB92 ---
--- universe_kernconf_mips_QCA953X_BASE ---
--- universe_kernconf_mips_RSPRO_STANDALONE ---
--- universe_i386_kernels ---
--- universe_kernconf_i386_GENERIC-NODEBUG ---
--- universe_kernconf_i386_LINT ---
--- universe_kernconf_i386_LINT-NOINET ---
--- universe_kernconf_i386_LINT-NOINET6 ---
--- universe_kernconf_i386_LINT-NOIP ---
--- universe_kernconf_i386_PAE ---
--- universe_mips_kernels ---
--- universe_kernconf_mips_SENTRY5 ---
--- universe_kernconf_mips_SWARM ---
--- universe_powerpc_kernels ---
--- universe_kernconf_powerpc_LINT ---
powerpc LINT kernel failed, check _.powerpc.LINT for details
--- universe_mips_kernels ---
--- universe_kernconf_mips_SWARM_SMP ---
--- universe_kernconf_mips_SWARM64 ---
--- universe_kernconf_mips_SWARM64_SMP ---
--- universe_kernconf_mips_XLP ---
--- universe_kernconf_mips_XLP64 ---
--- universe_kernconf_mips_XLPN32 ---
--- universe_arm_done ---
>> arm completed on Mon Apr  9 13:19:21 EDT 2018
--- universe_powerpc_kernels ---
--- universe_kernconf_powerpc_LINT64 ---
powerpc LINT64 kernel failed, check _.powerpc.LINT64 for details
--- universe_powerpc_done ---
>> powerpc completed on Mon Apr  9 13:19:25 EDT 2018
--- universe_mips_done ---
>> mips completed on Mon Apr  9 13:19:35 EDT 2018
--- universe_i386_done ---
>> i386 completed on Mon Apr  9 13:20:13 EDT 2018
--- universe_epilogue ---
--------------------------------------------------------------
>>> make universe completed on Mon Apr  9 13:20:13 EDT 2018
                      (started Mon Apr  9 13:01:48 EDT 2018)
--------------------------------------------------------------
Tinderbox failed:
arm.arm buildworld failed, check _.arm.arm.buildworld for details
arm.armeb buildworld failed, check _.arm.armeb.buildworld for details
powerpc LINT kernel failed, check _.powerpc.LINT for details
powerpc LINT64 kernel failed, check _.powerpc.LINT64 for details

The build failures are unrelated from some kernel and userspace code. Feel free to land this change whenever someone has given their okay again.

[brooks]

lib/libc/gen/exect.c

4 ↗

(On Diff #41241)

As you have now written the majority of code in this program, does it make more sense to put your copyright on it instead of TNF? It's not as though the calling of execve() could be anything else.

[ali_mashtizadeh.com]

• Update copyright

[ali_mashtizadeh.com]

Done

[brooks]

I believe this won't apply due to my removal of the arch specific setlogin.S files. If you don't get to it by the time I look at it tomorrow, I'll fix it up when I commit.

[ali_mashtizadeh.com]

Rebase