Page MenuHomeFreeBSD

Pass stack size, limit, and (no)overcommit flag to image using auxv, to avoid syscalls on jemalloc and libthr startup.
ClosedPublic

Authored by kib on Sep 12 2022, 7:48 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Jan 4, 2:31 PM
Unknown Object (File)
Dec 15 2022, 7:52 PM

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kib requested review of this revision.Sep 12 2022, 7:48 PM

I did not yet even compiled userspace.

This is outside of my areas of expertise. I'm afraid I won't be able to help with the review.

This is outside of my areas of expertise. I'm afraid I won't be able to help with the review.

I only added you as subscriber. In fact, my question would be, are there any other sysctls relevant for the image startup. I do not see anything else in truss output.

contrib/jemalloc/src/pages.c
441 ↗(On Diff #110477)

Does this need #ifdef FreeBSD && mumble to be acceptable upstream?
This comment applies generally through this file.

lib/libthr/thread/thr_stack.c
160 ↗(On Diff #110477)

Use nitem(mib) to allow this to all be on 1 line maybe?

kib marked 2 inline comments as done.Sep 12 2022, 8:29 PM
kib added inline comments.
contrib/jemalloc/src/pages.c
441 ↗(On Diff #110477)

It is already under some kind of #ifdef that implies BSD at least, and perhaps FreeBSD specifically. But I added one more #ifdef to hide ELF_BSDF_VMNOOVERCOMMIT.

kib marked an inline comment as done.

nitems(), #ifdef.

OK. This now looks good to my eye... assuming the values computed in the kernel are good (I'm less sure about those, to be honest, though they look like what I'd write).

This revision is now accepted and ready to land.Sep 12 2022, 9:10 PM

it can also shave the issetugid call, see https://reviews.freebsd.org/D23779

sys/kern/imgact_elf.c
1515 ↗(On Diff #110479)

there should be no need to take the lock here. instead i would assert curthread->td_limit == imgp->proc->p_limit. it matters because it is likely contend against the parent doing wait after forking

sys/vm/swap_pager.c
172–173

this should be __read_mostly

Mark vm_overcommit as __read_mostly.
Directly deref p_limit for AT_USRSTACKLIM.

This revision now requires review to proceed.Sep 13 2022, 10:22 AM
kib marked 2 inline comments as done.Sep 13 2022, 10:25 AM

Note that there is some change in the semantic, because AT_USRSTACKLIM is set at exec time, while getrlimit(2) is current. But I do not believe it matters for map_stacks_exec().
Even more so, I do not think the difference matter for vm_overcommit.

One more place in libthr

libthr: remove duplication for code calculating main thread stack base and size

Should auxv.3 be updated?

contrib/jemalloc/src/pages.c
441 ↗(On Diff #110477)

bsdflags might trigger some -Wunused warning if ELF_BSDF_VMNOOVERCOMMIT is not defined.

lib/libc/gen/elf_utils.c
97 ↗(On Diff #110529)

I think this can't work on CHERI, since it relies on being able to forge a pointer from u_longs provided by the kernel. Should we instead make the type of the USRSTACKBASE entry a uintptr_t instead? @jhb or @jrtc27 or @brooks might have an opinion. Though, it looks like this problem exists with KERN_USRSTACK today in CheriBSD, so maybe they don't care about supporting executable stacks.

lib/libthr/thread/thr_stack.c
157 ↗(On Diff #110529)

Should libthr panic if this call fails? It would provide a more useful failure mode if some W^X policy is enabled, no? Same for __libc_map_stacks_exec().

sys/vm/vm.h
172

These can be defined under _KERNEL, no?

kib marked 4 inline comments as done.Sep 15 2022, 10:10 AM
kib added inline comments.
lib/libc/gen/elf_utils.c
97 ↗(On Diff #110529)

Yes, KERN_USRSTACK is same. This is why I selected a_un.a_val for AT_USRSTACKLIM.

lib/libthr/thread/thr_stack.c
157 ↗(On Diff #110529)

No, I do not think so. I highly dislike system libraries abruptly terminating the process. If it can continue, it should.

sys/vm/vm.h
172

No, the bits define values for sysctl vm.overcommit, so I thought that the definitions are useful for userspace. In fact, jemalloc should be modified to use the symbols on sysctl fallback.

kib marked 3 inline comments as done.

Use SWAP_RESERVE_ symbols instead of magic in jemalloc
Fix potential unused variable warning in jemalloc
Document new AT_ values

Seems like a good change. We don't need usrstack to be a pointer on CheriBSD and having it be one is a least-privilege violation. We don't support an executable stack for the main thread and there isn't really any reason to do so for ABIs that don't have backwards compatibility concerns (the main reason to do so is to support nested functions and IIRC gcc now supports nested functions without exec stacks, but doing so is an ABI break).

sys/vm/swap_pager.c
269

I'd prefer all this whitespace churn was in a separate commit.

This revision is now accepted and ready to land.Sep 15 2022, 11:27 AM
kib marked an inline comment as done.Sep 15 2022, 11:38 AM
kib added inline comments.
sys/vm/swap_pager.c
269