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

[brooks]

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

[brooks]

• Rebase
• Catch up with changes in rS341263.

[jhb]

sys/kern/init_main.c

777 ↗

(On Diff #51374)

The reordering makes the diff a bit harder to read. Did you sort them alphabetically it seems?

782 ↗

(On Diff #51374)

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.

[kib]

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]

sys/kern/init_main.c

777 ↗

(On Diff #51374)

The they are reversed because the old code builds the argument backwards (from the top of the pseudo stack).

782 ↗

(On Diff #51374)

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.

[brooks]

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]

sys/kern/init_main.c

777 ↗

(On Diff #51374)

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.

782 ↗

(On Diff #51374)

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.

[kib]

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.

[brooks]

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

[brooks]

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

[brooks]

sys/kern/init_main.c

782 ↗

(On Diff #51374)

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

[kib]

sys/kern/init_main.c

797 ↗

(On Diff #51556)

error cannot be non-zero. You panic a line before.

[brooks]

• Remove redundent check (too much copy-and-paste).
• Fix typo.