Page MenuHomeFreeBSD

amd64: stop using top of the thread' kernel stack for FPU user save area
ClosedPublic

Authored by kib on Sep 14 2021, 12:07 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 8, 7:16 PM
Unknown Object (File)
Tue, Oct 29, 8:18 AM
Unknown Object (File)
Fri, Oct 25, 4:47 PM
Unknown Object (File)
Sun, Oct 20, 6:11 AM
Unknown Object (File)
Oct 1 2024, 7:21 AM
Unknown Object (File)
Oct 1 2024, 7:19 AM
Unknown Object (File)
Oct 1 2024, 7:17 AM
Unknown Object (File)
Sep 30 2024, 12:35 PM
Subscribers

Details

Summary

Instead do one more allocation at the thread creation time. This frees a lot of space on the stack.

Also do not use alloca() for temporal storage in signal delivery sendsig() function and signal return syscall sys_sigreturn(). This saves equal amount of space, again by the cost of one more allocation at the thread creation time.

amd64: eliminate td_md.md_fpu_scratch

For signal send, copyout from the user FPU save area directly.

For sigreturn, we are in sleepable context and can do temporal allocation of the transient save area. We cannot copying from userspace directly to user save area because XSAVE state needs to be validated, also partial copyins can corrupt it.

amd64: move signal handling and register structures manipulations into sig_machdep.c from machdep.c which is too large pile of unrelated things. Some ptrace functions are moved from machdep.c to ptrace_machdep.c.

linux32: add a hack to avoid redefining the type of the savefpu tag when compiling in amd64 kernel environment with -m32. This is a temporal workaround for some future proper (but unclear) fix.

Per-commit view is available at https://kib.kiev.ua/git/gitweb.cgi?p=deviant3.git;a=shortlog;h=refs/heads/alloca

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

kib requested review of this revision.Sep 14 2021, 12:07 AM
kib created this revision.
mhorne added inline comments.
sys/amd64/amd64/ptrace_machdep.c
285

I think ptrace_machdep.c is the logical place for these functions to live, but it would be helpful to make the organization consistent across architectures, where possible.

The current situation is somewhat of a mess, but can you do the same move for at least i386?

kib marked an inline comment as done.Sep 14 2021, 2:05 PM
kib added inline comments.
sys/amd64/amd64/ptrace_machdep.c
285

I can do something similar to i386. There is it even more sense to move signal support to separate file, because there are more compat stuff in machdep.c (and not in ia32/...). I was somewhat hesitant to touch i386 due to general hostility against the arch.

But I will do, just not in this patch series, but later.

sys/amd64/amd64/machdep.c
1621

Why do we zero the entire save area for thread0 but not for other threads?

sys/amd64/amd64/sig_machdep.c
70

I'm fairly sure many of these are not needed, at least asan.h, csan.h, msan.h are not.

112

BTW, CS_SECURE and EFL_SECURE are duplicated in several places.

sys/amd64/amd64/vm_machdep.c
392

I would initialize these after td->td_frame, so the save area and related PCB initialization is done together. I also wonder if we really need a separate thread0_init_fpu_save().

kib marked 5 inline comments as done.Sep 14 2021, 4:26 PM
kib added inline comments.
sys/amd64/amd64/machdep.c
1621

This is just historic. I removed thread0_init_fpu_state() so this question should become not important.

kib marked an inline comment as done.

Use cpu_thread_alloc() for thread0.
Reorder statements in cpu_thread_alloc().
Centralize CS_SECURE.
Reduce includes pollution in sig_machdep.c.

sys/amd64/amd64/machdep.c
1567

Aren't we relying on the set_top_of_stack_td() call here?

kib marked an inline comment as done.Sep 14 2021, 5:58 PM
kib added inline comments.
sys/amd64/amd64/machdep.c
1567

Yes, it is done at line 1503 above.

kib marked an inline comment as done.Sep 14 2021, 5:58 PM
This revision is now accepted and ready to land.Sep 15 2021, 1:28 PM

I'd be happy btw with fixing i386 to stop using alloca() rather than my GCC warning hack. I do wonder why we can't just use the normal save area though?

sys/amd64/amd64/sig_machdep.c
144

I've wondered if we could just save the state to the "normal" save area for sendsig? Similarly, sigreturn could just copy into the normal save area? We could assert that you aren't using the save area for kernel state (which should never be true for sendsig and sigreturn)?

sys/amd64/amd64/sig_machdep.c
144

Might be, it would work. This will be a follow-up work, I do not want to do this in the current batch.

kib marked an inline comment as done.
kib edited the summary of this revision. (Show Details)

amd64: eliminate td_md.md_fpu_scratch
Also remove alloca() from ia32_signal.c.

This revision now requires review to proceed.Sep 15 2021, 7:11 PM
This revision is now accepted and ready to land.Sep 16 2021, 12:50 PM

One more pass over _SECURE macros, e.g. linuxolator has RFLAGS_SECURE etc, remove them as well.
Consistently use uprintf() to report weird situations in sigreturn.

This revision now requires review to proceed.Sep 16 2021, 1:56 PM
jrtc27 added inline comments.
sys/amd64/amd64/sig_machdep.c
347

Nothing below this point is about signals but about ptrace and exec so this feels like the wrong place for them (or the wrong name for the file)

This revision is now accepted and ready to land.Sep 20 2021, 6:59 PM
sys/amd64/amd64/sig_machdep.c
347

If you provide a sensitive and better name for the file, I am all for the change. Otherwise, we actually have a precedents like ia32_signal.c.

sys/amd64/amd64/sig_machdep.c
347

Nothing below this point is about signals but about ptrace and exec so this feels like the wrong place for them (or the wrong name for the file)

Hmm, it's hard to say for some things like get_mcontext as the ucontext is generally about signals (esp given that POSIX has deprecated all the swapcontext and related ilk). It's true that exec_setregs() isn't really signal specific. I think MIPS puts all this stuff in a 'pm_machdep.c' where the "pm" == "process management". Maybe 'proc_machdep.c' would be a better name?