Page MenuHomeFreeBSD

Replace exect() with a portable version
ClosedPublic

Authored by ali_mashtizadeh.com on Apr 6 2018, 3:17 PM.

Details

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

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

  • 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)

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.

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 added a comment.Apr 6 2018, 3:44 PM
  • 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 added a subscriber: rgrimes.Apr 6 2018, 3:49 PM

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

brooks added inline comments.Apr 6 2018, 3:53 PM
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 added a subscriber: andrew.Apr 6 2018, 3:58 PM

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 added a comment.Apr 6 2018, 4:34 PM

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());.

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 marked 3 inline comments as done.
  • 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

kib accepted this revision.Apr 6 2018, 7:52 PM
This revision is now accepted and ready to land.Apr 6 2018, 7:52 PM

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.

@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 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 added a comment.Apr 6 2018, 8:39 PM

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 added inline comments.Apr 6 2018, 8:43 PM
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?

In D14989#315514, @kib 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 added a comment.Apr 6 2018, 8:49 PM

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.

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?

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?

Could it be this works on i386 only?

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);
  • Remove the dead expression from i386/amd64 machdep
This revision now requires review to proceed.Apr 6 2018, 10:13 PM

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 added a comment.Apr 6 2018, 10:21 PM

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.

Backout kernel as kib is taking care of this.

  • Ignore EBUSY errno
ali_mashtizadeh.com marked an inline comment as done.Apr 7 2018, 1:03 AM

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

Should be done if everyone is happy!

  • Ignore EBUSY errno
andrew added a comment.Apr 7 2018, 6:42 AM

@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.

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 added a comment.Apr 7 2018, 5:02 PM

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.

  • Remove the warning message
cem added a subscriber: cem.Apr 8 2018, 1:54 AM
ali_mashtizadeh.com retitled this revision from Mark exect() as deprecated to Replace exect() with a portable version.Apr 9 2018, 6:07 PM
--- 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.

kib accepted this revision.Apr 9 2018, 6:31 PM
This revision is now accepted and ready to land.Apr 9 2018, 6:31 PM
brooks added inline comments.Apr 11 2018, 8:10 PM
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.

  • Update copyright
This revision now requires review to proceed.Apr 11 2018, 11:09 PM
ali_mashtizadeh.com marked an inline comment as done.EditedApr 11 2018, 11:10 PM

Done

brooks accepted this revision.Apr 12 2018, 12:00 AM

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.

This revision is now accepted and ready to land.Apr 12 2018, 12:00 AM
This revision now requires review to proceed.Apr 12 2018, 12:22 AM
brooks accepted this revision.Apr 12 2018, 5:25 PM
This revision is now accepted and ready to land.Apr 12 2018, 5:25 PM
This revision was automatically updated to reflect the committed changes.