Copy out aux args after the argv and environment vectors.

[jhb]

Summary

Partially revert r354741 and r354754 and go back to allocating a
fixed-size chunk of stack space for the auxiliary vector. Keep
sv_copyout_auxargs but change it to accept the address at the end of
the environment vector as an input stack address and no longer
allocate room on the stack. It is now called at the end of
copyout_strings after the argv and environment vectors have been
copied out.

This should fix a regression in r354754 that broke the stack alignment
for newer Linux amd64 binaries (and probably broke Linux arm64 as
well).

Test Plan

• tested amd64, linux64 (only linux-base-c7), and i386 under amd64

[kib]

For what object the stack alignment was broken ?

[jhb]

sys/amd64/linux/linux_sysvec.c

354 ↗

(On Diff #65281)

The executables that depend on this (newer than ones in linux-base-c7) were broken by my earlier change. The reason is that the earlier change I made stopped taking the length of argv/envv into account when deciding on how to "super" align the stack. arm64 arguably does this cleaner than amd64 since it accounts for the 'argc' before argv on the stack in copyout_strings instead of fixup_elf and thus can do a simple alignment on 'vectp' after subtracting out the argv and env sizes. I would probably fix linux64 on amd64 to do the same so that the stack is setup in one place instead of split between functions.

arm64 was similarly also broken by my change (depending on the number of argv and envv entries) because the auxv vector might have had an extra pointer of alignment padding after the end of envv and the start of auxv that binaries would not have expected.

[trasz]

I don't quite understand it (nor do I understand the root of the problem in the first place, apart from symptoms), but the patch fixes the problem in my testing.

[jhb]

The problem is that vectp is initially 8 byte aligned. We then subtract space for the argv and envv arrays (plus NULLs). Suppose you have no environment and only argv[0] with NULL, then you would subtract 3 pointers from vectp. You need one more word for argc itself that is below argv[0]. glibc wants that initial stack pointer 16-byte aligned (the address of the argc). I had moved the alignment check earlier before we subtracted argc and envc from vectp, so it only worked if you had an odd number of pointers (argc + envc + 2 was odd). If you had an even number of pointers, it ended up misaligned. The old code fixed the alignment after moving vectp down for the argv/envv arrays and thus took their odd/even count into account. This change goes back to aligning vectp after moving vectp down for argv/envv.

The arm64 Linux bits are a bit clearer I think as they move vectp down over argc as well and then 16-byte align that resulting via the STACKALIGN macro rather than forcing "odd" 8-byte alignment because it knows elf_fixup will subtract 8 more bytes for argc later as the amd64 one does.

[kib]

sys/kern/kern_exec.c

1668 ↗

(On Diff #65281)

I do not like that FreeBSD ABI thing (AT_COUNT) appears in kern_exec.c, even in exec_copyout_strings(). Might be add sv method to prepare auxv then ?

1740 ↗

(On Diff #65281)

Or, if we pin exec_copyout_strings() to FreeBSD ABI, then the indirection through sv_ method looks redundant.

[kib]

sys/kern/kern_exec.c

1668 ↗

(On Diff #65281)

I think sv_atcount integer member of sysentvec would be enough.

[jhb]

sys/kern/kern_exec.c

1668 ↗

(On Diff #65281)

Well, it would more be the size since different ABIs use different structures (e.g. freebsd32). I did think about doing that. It would mean touching all the ABIs again, but I could do it that way.

1740 ↗

(On Diff #65281)

exec_copyout_strings is only used for FreeBSD ABIs, and only the "native" ABI (not freebsd32) at that. OTOH, we might be able to share the code with freebsd32 (and freebsd64 in cheribsd) if we did something similar to imgact_elfN.c. This could perhaps just call __elfN(freebsd_copyout_auxargs) directly instead of having the sv_ member if we think that is best. We don't do that with other __elfN functions here though, so sv_auxargs_size might be the better of those two options.

[kib]

sys/kern/kern_exec.c

1668 ↗

(On Diff #65281)

After all, I think that it should be fine for now as is. We already have the knowledge of auxv which is format-specific, in supposedly generic method.

1740 ↗

(On Diff #65281)

Do we consider a.out as native ?

[jhb]

sys/kern/kern_exec.c

1740 ↗

(On Diff #65281)

That is true, and it doesn't do auxargs, so you are right in that regard. I don't mind doing sv_auxargs_size if you think that is cleaner, though I might do it as a followup if that is ok.

The way I would do it is each elf_machdep.c would have:

.sv_auxargs_size = AT_COUNT * sizeof(Elf_Auxinfo),

and then in exec_copyout_strings you would replace the first 'imgp->auxargs' check with:

destp -= igmp->sysent->sv_auxargs_size;
destp = rounddown(destp, sizeof(void *));

and the last new hunk would be:

if (imgp->sysent->sv_copyout_auxargs != NULL) {
    vectp++;
    error = imgp->sysent->sv_copyout_auxargs(imgp, (uintptr_t)vectp);
    if (error != 0)
        return (error);
}

[kib]

sys/kern/kern_exec.c

1740 ↗

(On Diff #65281)

And for a.out the size would be 0. But we already short-circuit it due to the check for imgp->auxargs != NULL, I believe.

I think now that the patch is good as is, I already accepted the review.