Page MenuHomeFreeBSD
Differential D36540

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

Authored by kib on Sep 12 2022, 7:48 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 7, 4:15 AM
Unknown Object (File)
Sat, Nov 2, 8:24 PM
Unknown Object (File)
Wed, Oct 23, 7:01 PM
Unknown Object (File)
Wed, Oct 23, 7:01 PM
Unknown Object (File)
Wed, Oct 23, 6:44 PM
Unknown Object (File)
Wed, Oct 23, 12:09 PM
Unknown Object (File)
Wed, Oct 23, 2:24 AM
Unknown Object (File)
Oct 11 2024, 2:53 AM
View All Files
Subscribers
asomers
brooks
imp
jhb
jrtc27
mjg

Details

Reviewers
imp
markj
mjg
emaste
brooks
Commits
rGdaa85548d5cf: rtld: teach LD_SHOW_AUXV about AT_USRSTACK*
rGe03c7f500541: libthr: extract code to get main stack base and size into helpers
rGe2879ece4314: libc, libthr: use AT_USRSTACK{BASE,LIM} instead of sysctl("kern.usrstack") and…
rGebf7a01594ee: libthr: use nitems() for mib length
rG0ae364adcd8d: jemalloc: use auxv ELF_BSDF_VMNOOVERCOMMIT instead of sysctl("vm.overcommit")
rG62b4fb22df04: auxv.3: Document AT_USRSTACKBASE and AT_USRSTACKLIM
rG1d280f21427f: procstat(1): print AT_USRSTACKBASE and AT_USRSTACKLIM
rG8f2668b0605e: _elf_aux_info(3): add support for AT_USRSTACK{BASE,LIM}
rGff41239f587f: Add AT_USRSTACK{BASE, LIM} AT vectors, and ELF_BSDF_VMNOOVERCOMMIT flag
rG87384c51e047: jemalloc: use symbolic definitions for bits in vm_overcommit
rG26eed2aa06a2: swap_pager: style, wrap long lines
rGccdaa1ab1c5f: vm_overcommit: put into __read_mostly section
rGa6cc4c6e98eb: vm: make vm.overcommit available externally

Diff Detail

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

Event Timeline

kib created this revision.Sep 12 2022, 7:48 PM
Herald added a subscriber: imp. · View Herald TranscriptSep 12 2022, 7:48 PM
kib requested review of this revision.Sep 12 2022, 7:48 PM

I did not yet even compiled userspace.

asomers added a comment.Sep 12 2022, 8:05 PM

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

kib added a comment.Sep 12 2022, 8:09 PM
In D36540#829769, @asomers wrote:

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.

kib updated this revision to Diff 110477.Sep 12 2022, 8:09 PM

Fix compilation.

imp added inline comments.Sep 12 2022, 8:19 PM
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 updated this revision to Diff 110478.Sep 12 2022, 8:20 PM

Fix map_stack_exec()s.

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 updated this revision to Diff 110479.Sep 12 2022, 8:30 PM
kib marked an inline comment as done.

nitems(), #ifdef.

imp added a comment.Sep 12 2022, 9:10 PM

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

imp accepted this revision.Sep 12 2022, 9:10 PM
This revision is now accepted and ready to land.Sep 12 2022, 9:10 PM
mjg added a comment.Sep 12 2022, 9:15 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

kib updated this revision to Diff 110502.Sep 13 2022, 10:22 AM

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 added reviewers: markj, mjg, emaste.Sep 13 2022, 10:23 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.

kib updated this revision to Diff 110527.Sep 13 2022, 8:51 PM

One more place in libthr

kib updated this revision to Diff 110529.Sep 13 2022, 10:15 PM

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

markj added subscribers: jrtc27, brooks, jhb.Sep 15 2022, 8:36 AM

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 updated this revision to Diff 110597.Sep 15 2022, 10:12 AM
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

kib updated this revision to Diff 110598.Sep 15 2022, 10:15 AM

Update 'procstat auxv'

brooks accepted this revision.Sep 15 2022, 11:27 AM

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

It is, please see https://kib.kiev.ua/git/gitweb.cgi?p=deviant3.git;a=shortlog;h=refs/heads/at_vm for per-commit split

Closed by commit rGa6cc4c6e98eb: vm: make vm.overcommit available externally (authored by kib). · Explain WhySep 16 2022, 8:26 PM
This revision was automatically updated to reflect the committed changes.
kib marked an inline comment as done.
kib added a commit: rGa6cc4c6e98eb: vm: make vm.overcommit available externally.
kib added a commit: rGccdaa1ab1c5f: vm_overcommit: put into __read_mostly section.
kib added a commit: rG26eed2aa06a2: swap_pager: style, wrap long lines.
kib added a commit: rG87384c51e047: jemalloc: use symbolic definitions for bits in vm_overcommit.
kib added a commit: rGff41239f587f: Add AT_USRSTACK{BASE, LIM} AT vectors, and ELF_BSDF_VMNOOVERCOMMIT flag.
kib added a commit: rG8f2668b0605e: _elf_aux_info(3): add support for AT_USRSTACK{BASE,LIM}.
kib added a commit: rG1d280f21427f: procstat(1): print AT_USRSTACKBASE and AT_USRSTACKLIM.
kib added a commit: rG62b4fb22df04: auxv.3: Document AT_USRSTACKBASE and AT_USRSTACKLIM.
kib added a commit: rG0ae364adcd8d: jemalloc: use auxv ELF_BSDF_VMNOOVERCOMMIT instead of sysctl("vm.overcommit").
kib added a commit: rGebf7a01594ee: libthr: use nitems() for mib length.
kib added a commit: rGe2879ece4314: libc, libthr: use AT_USRSTACK{BASE,LIM} instead of sysctl("kern.usrstack") and….
kib added a commit: rGe03c7f500541: libthr: extract code to get main stack base and size into helpers.
kib added a commit: rGdaa85548d5cf: rtld: teach LD_SHOW_AUXV about AT_USRSTACK*.

Revision Contents
Changeset List

PathSize
sys/
vm/
11 lines
6 lines

Diff 110669

View Options

sys/vm/swap_pager.c

Loading...
View Options

sys/vm/vm.h

Loading...