Details

Reviewers

kib
jhb

Commits

rS341449: Remove a needlessly clever hack to start init with sys_exec().

Summary

Construct a struct image_args with the help of new exec_args_*() helper
functions and call kern_execve().

The previous code mapped a page in userspace, copied arguments out
to it one at a time, and then constructed a struct execve_args all so
that sys_execve() can call exec_copyin_args() to copy the data back in
to a struct image_args.

Opencode the part of pre_execve()/post_execve() that releases a reference
to the initial vmspace. We don't need to stop threads like they do.

Obtained from: CheriBSD
Sponsored by: DARPA, AFRL
Depends on: D15468

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 21345
Build 20673: arc lint + arc unit

Event Timeline

brooks created this revision.May 18 2018, 12:09 AM

Harbormaster completed remote builds in B16656: Diff 42681.May 18 2018, 12:09 AM

Rebase
Catch up with changes in rS341263.

Harbormaster completed remote builds in B21272: Diff 51374.Nov 29 2018, 11:38 PM

brooks edited the summary of this revision. (Show Details)Nov 30 2018, 11:20 PM

brooks added reviewers: kib, jhb.

jhb added inline comments.Dec 1 2018, 12:08 AM

sys/kern/init_main.c
781–782	The reordering makes the diff a bit harder to read. Did you sort them alphabetically it seems?
783–788	This handling now seems kind of wrong. Previously this was adding the '-' in front of all of the options if there were any, now it is adding a second '-' so that you get '--' if there were no options. You could instead perhaps keep the 'options = 1' from the previous code and instead and perhaps just omit flags entirely if options is 0, e.g.: options = 0; flagp = &flags[0]; flagp++ = '-'; #ifdef BOOTCDROM flagp++ = 'C' options = 1; #endif /* other options / flagp++ = 0; KASSERT(...); if (options != 0) { error = exec_args_add_arg(&args, flags, UIO_SYSSPACE); if (error != 0) panic(...); } This seems cleaner than adding a dummy '--' option.

Not calling pre/post leave a stray reference on vmspace0. I do not claim that this is important, but it is certainly something to consider and at least comment about. Might be the stray TDP_EXECVMSPC on thread0 is worse.

brooks marked 2 inline comments as done.Dec 1 2018, 12:49 AM

brooks added inline comments.

sys/kern/init_main.c
781–782	The they are reversed because the old code builds the argument backwards (from the top of the pseudo stack).
783–788	That doesn't match my read of the old code. The old options==0 case adds the a second '-' if there weren't any options and the code at line 784 adds the first hyphen unconditionally.

In D15469#391417, @kib wrote:

Not calling pre/post leave a stray reference on vmspace0. I do not claim that this is important, but it is certainly something to consider and at least comment about. Might be the stray TDP_EXECVMSPC on thread0 is worse.

Hmm, I'll look into this more closely on Monday. It's not particularly hard to call them.

jhb added inline comments.Dec 1 2018, 1:12 AM

sys/kern/init_main.c
781–782	Ok. I don't think the order of the flags passed to init matters, so would probably have preserved the existing code order to minimize the diff, but either way is fine.
783–788	Huh, that seems kind of silly to pass --. I guess it was because the old code always copied out arg1 and didn't want to deal with having it be optional. Maybe as a followup we can remove the '--' as I do think it's just a kludge to avoid having arg1 being optional. The 'options = 1' bit would reduce the diff a little still.

In D15469#391421, @brooks wrote:

In D15469#391417, @kib wrote:

Not calling pre/post leave a stray reference on vmspace0. I do not claim that this is important, but it is certainly something to consider and at least comment about. Might be the stray TDP_EXECVMSPC on thread0 is worse.

Hmm, I'll look into this more closely on Monday. It's not particularly hard to call them.

No, I think that calling them on proc0 would have too much undesirable effects, we do not want to stop almost all kernel threads. You would need to open-code clearing of the flag and possible dereference of vmspace0.

In D15469#391506, @kib wrote:

In D15469#391421, @brooks wrote:

In D15469#391417, @kib wrote:

Not calling pre/post leave a stray reference on vmspace0. I do not claim that this is important, but it is certainly something to consider and at least comment about. Might be the stray TDP_EXECVMSPC on thread0 is worse.

Hmm, I'll look into this more closely on Monday. It's not particularly hard to call them.

No, I think that calling them on proc0 would have too much undesirable effects, we do not want to stop almost all kernel threads. You would need to open-code clearing of the flag and possible dereference of vmspace0.

I glanced at the pre/post code briefly on Friday and was thinking of open coding these bit as well. FWIW, we've been calling pre/post since they were added to sys_execve...

Rebase
Reduce diffs to master by retaining option handling order.
Free vmspace reference and clear TDP_EXECVMSPC after kern_execve().

Harbormaster completed remote builds in B21345: Diff 51556.Dec 3 2018, 10:09 PM

brooks marked 5 inline comments as done.Dec 3 2018, 10:48 PM

brooks added inline comments.

sys/kern/init_main.c
783–788	After this change, I'm going to do a pass removing BOOTCDROM (init warns and ignores about -C), the RB_FASTBOOT case (ifdef notyet since at least the 4.4lite import), and --. That will leave a single case of exec_args_add_arg(&args, "-s", UIS_SYSSPACE).