Page MenuHomeFreeBSD
Differential D15485

Avoid two suword() calls per auxarg entry.
ClosedPublic
Actions

Authored by brooks on May 18 2018, 9:12 PM.
Tags
None
Referenced Files
Unknown Object (File)
Dec 31 2023, 4:44 AM
Unknown Object (File)
Dec 25 2023, 6:11 AM
Unknown Object (File)
Dec 23 2023, 4:27 AM
Unknown Object (File)
Dec 3 2023, 4:21 AM
Unknown Object (File)
Nov 23 2023, 4:03 PM
Unknown Object (File)
Nov 23 2023, 7:02 AM
Unknown Object (File)
Nov 19 2023, 1:40 PM
Unknown Object (File)
Nov 6 2023, 10:54 PM
View All 37 Files
Subscribers
jkim

Details

Reviewers
jhb
emaste
kib
imp
Commits
rS334165: Avoid two suword() calls per auxarg entry.
Summary

Instead, construct an auxargs array and copy it out all at once.

Use an array of Elf_Auxinfo rather than pairs of Elf_Addr * to represent
the array. This is the correct type where pairs of words just happend
to work. To reduce the size of the diff, AUXARGS_ENTRY is altered to act
on this array rather than introducing a new macro.

Return errors on copyout() and suword() failures and handle them in the caller.

Incidentally fixes AT_RANDOM and AT_EXECFN in 32-bit linux on amd64
which incorrectly used AUXARG_ENTRY instead of AUXARGS_ENTRY_32
(now removed).

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 16769
Build 16656: arc lint + arc unit

Event Timeline

brooks created this revision.May 18 2018, 9:12 PM
Harbormaster completed remote builds in B16681: Diff 42715.May 18 2018, 9:12 PM
brooks added reviewers: jhb, emaste, kib, imp.May 18 2018, 9:16 PM

This patch compiles (if you don't build linux emulation), but is incomplete. I've tested a variant on CheriBSD. In this review I'm looking for feedback on the approach before altering AUXARGS_ENTRY_32 and handling linux emulation. This is certainly more efficient and might be measurably so on i386 with 4/4.

kib added inline comments.May 18 2018, 10:09 PM
sys/kern/imgact_elf.c
1151

Why printing it out ? Other copyouts into the new process address space fail silently.

I think all code should be same in this regard, i.e. we should uprintf() the failure and return some error from execve(), so that the process terminates with a signal.

brooks added inline comments.May 21 2018, 4:10 PM
sys/kern/imgact_elf.c
1151

I put that in while I was debugging on CheriBSD. I'll update to return an error.

brooks updated this revision to Diff 42813.May 21 2018, 8:50 PM
  • Return error on copyout/suword failure in freebsd_fixup.
Harbormaster completed remote builds in B16743: Diff 42813.May 21 2018, 8:50 PM
kib accepted this revision.May 21 2018, 9:33 PM
kib added inline comments.
sys/kern/imgact_elf.c
1101–1103

Merge this and previous line ?

1106–1107

() can be dropped. I am not sure about split of 2 into 1 and 1, but I do not object.

1153–1154

I think cast can be dropped.

sys/kern/kern_exec.c
696–699

() around == are not needed.

This revision is now accepted and ready to land.May 21 2018, 9:33 PM
emaste added inline comments.May 22 2018, 12:48 AM
sys/kern/imgact_elf.c
1106–1107

To me two separate + 1s makes a more clear suggestion that there's one NULL terminator per list.

sys/sys/imgact_elf.h
41

(pos)++->a_un.a_val reads a bit awkwardly

brooks updated this revision to Diff 42856.May 22 2018, 8:40 PM
  • Cleanups suggested by kib.
  • Break out the increment of pos.
  • Extend to cover linux compat and eliminate AUXARGS_ENTRY_32.
This revision now requires review to proceed.May 22 2018, 8:40 PM
Harbormaster completed remote builds in B16764: Diff 42856.May 22 2018, 8:40 PM
brooks edited the summary of this revision. (Show Details)May 22 2018, 8:41 PM
brooks marked 8 inline comments as done.May 22 2018, 10:20 PM

Trivial testing done with amd64, freebsd32 on amd64, and linux64 on amd64.

brooks updated this revision to Diff 42862.May 23 2018, 12:08 AM
  • Fix build on i386.
Harbormaster completed remote builds in B16769: Diff 42862.May 23 2018, 12:08 AM
jhb added inline comments.May 23 2018, 5:26 PM
sys/amd64/linux32/linux32_sysvec.c
210

Use the two + 1's here as well?

sys/kern/imgact_elf.c
1101

My only question is if the stack usage is ok. On LP64 with our current AT_COUNT of 27 (which might very well grow), this uses 432 bytes. With CHERI 128 that is up 864 bytes (and CHERI 256 is almost 2K). Even in the LP64 case I think that's perhaps a big chunk of stack space.

brooks added inline comments.May 23 2018, 6:13 PM
sys/kern/imgact_elf.c
1101

I was debating which was best and I agree 432 is large and AT_COUNT is at least 31 on CHERI where I've punted argc, argv, and envv (as well as envc because, why not) to auxargs. I'll make the switch. I also noticed while doing so that I'd failed to zero argarray so there was a large kernel stack leak.

brooks updated this revision to Diff 42891.May 23 2018, 6:23 PM
  • Allocate argarray with malloc() and zero to prevent leak.
  • Use argc + 1 + envc + 1 idiom one more place
Harbormaster completed remote builds in B16788: Diff 42891.May 23 2018, 6:23 PM
brooks marked 3 inline comments as done.May 23 2018, 10:20 PM
kib accepted this revision.May 23 2018, 10:35 PM
This revision is now accepted and ready to land.May 23 2018, 10:35 PM
brooks edited the summary of this revision. (Show Details)May 24 2018, 4:00 PM
brooks edited the summary of this revision. (Show Details)May 24 2018, 4:23 PM
Closed by commit rS334165: Avoid two suword() calls per auxarg entry. (authored by brooks). · Explain WhyMay 24 2018, 4:25 PM
This revision was automatically updated to reflect the committed changes.
jkim added a subscriber: jkim.Jun 12 2018, 8:28 PM

Unfortunately, this change broke 32-bit Linuxulator on amd64. Please see PR228790.

jkim added a comment.Jun 12 2018, 11:14 PM

I found there's a copy-and-pasto and it should be fixed with r335020.

Revision Contents
Changeset List

PathSize
sys/
amd64/
linux/
16 lines
linux32/
60 lines
i386/
linux/
16 lines
kern/
15 lines
7 lines
sys/
3 lines

Diff 42862

View Options

sys/amd64/linux/linux_sysvec.c

Loading...
View Options

sys/amd64/linux32/linux32_sysvec.c

Loading...
View Options

sys/i386/linux/linux_sysvec.c

Loading...
View Options

sys/kern/imgact_elf.c

Loading...
View Options

sys/kern/kern_exec.c

Loading...
View Options

sys/sys/imgact_elf.h

Loading...