From owner-dev-commits-src-main@freebsd.org Thu Sep 23 19:49:50 2021 Return-Path: Delivered-To: dev-commits-src-main@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id C00416ACAB1; Thu, 23 Sep 2021 19:49:50 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-lf1-x129.google.com (mail-lf1-x129.google.com [IPv6:2a00:1450:4864:20::129]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4HFm321gbTz4SRj; Thu, 23 Sep 2021 19:49:50 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-lf1-x129.google.com with SMTP id b20so30756974lfv.3; Thu, 23 Sep 2021 12:49:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=XiJw4+GFywfOxX/Q6qdOST9xQ20TtUjHiJ6sfqNR+6M=; b=NQA51JuU3Fr83LF+0Drfm25eW6G2EeRxjqPvcUHx85MQJZ48DUVj7n+/cRU9r57mh7 IOunsRbBWsCvcU3b2kHe8GwhRY0ZHs6Ezuoix4RZggoeh/kEocMMkIO9oIochcbSC2J8 RC6SLNo4T5NwAJIfnJk+IkC0IvpmeSKf+7qXG12YepGpclqTFHPN114MVBhZhBW1Y3b2 ePnzjjPk7wcrPDFbgdRsKcrI+0Hw+YLVr9ORZxaikrJFXs06gG9UJH4BfYJGKeIuMnHL AhAeXf/H+huoKmLchM0H6oAfgg6gz87q6cY2fNi9lxh9wLMsFuO409/bWa+wHj4rfZ7j oAEw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=XiJw4+GFywfOxX/Q6qdOST9xQ20TtUjHiJ6sfqNR+6M=; b=BXyozgW2qsJFH/Ch9X5JnEDHYUYIiB3j7XSCBhh4dScaVlu+2fZNh9fn5qikDOakXs X6WrH+ZUhL22Y6uhEzJH4Z1hcaHPeEOFiDBVmqa21KNiE+43uJvQqhddU+O9PVrKUeGR yp2DLEA1DL3EDJlBxsXdLiIBiPVvsqEkSa563y5dzMGBtPrUg2HNhi+ff3kWDJJVwQp4 WqIUY4OZA9FT6kYMIAVH8Kei8DMcgf553fHA8NbRV8JkOvEAitbze6RjsZkkpNC3LUhf VUczbgZZNVFhF38+7FYMtL47HSV453btPgoCOCSfXmUBj97p7gijPifR9sJAFijOwtKg wc/g== X-Gm-Message-State: AOAM533YhuKLp05d2XxvhTPqwSQMGW6obE3Bw2TZ5jmTK+tnrxiR6M23 pGlXa5mTYeXBkE5RwFJvLPiWZFScCFm6Hy8qeMpuyANx X-Google-Smtp-Source: ABdhPJzOYWzRZfYArTxw4NQWtwu1WUKu2qm0zTb90/s4/rlzci9i+oNIE24q4hgfuRfwIxlfgRVttVRu971H+l8RvtM= X-Received: by 2002:a19:ee12:: with SMTP id g18mr5692456lfb.300.1632426588797; Thu, 23 Sep 2021 12:49:48 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a2e:2f18:0:0:0:0:0 with HTTP; Thu, 23 Sep 2021 12:49:47 -0700 (PDT) In-Reply-To: <202109211721.18LHLFWM028927@gitrepo.freebsd.org> References: <202109211721.18LHLFWM028927@gitrepo.freebsd.org> From: Mateusz Guzik Date: Thu, 23 Sep 2021 21:49:47 +0200 Message-ID: Subject: Re: git: df8dd6025af8 - main - amd64: stop using top of the thread' kernel stack for FPU user save area To: Konstantin Belousov Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 4HFm321gbTz4SRj X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; dkim=pass header.d=gmail.com header.s=20210112 header.b=NQA51JuU; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (mx1.freebsd.org: domain of mjguzik@gmail.com designates 2a00:1450:4864:20::129 as permitted sender) smtp.mailfrom=mjguzik@gmail.com X-Spamd-Result: default: False [-4.00 / 15.00]; RCVD_TLS_ALL(0.00)[]; ARC_NA(0.00)[]; R_DKIM_ALLOW(-0.20)[gmail.com:s=20210112]; NEURAL_HAM_MEDIUM(-1.00)[-1.000]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[4]; FREEMAIL_FROM(0.00)[gmail.com]; TO_MATCH_ENVRCPT_ALL(0.00)[]; MIME_GOOD(-0.10)[text/plain]; R_SPF_ALLOW(-0.20)[+ip6:2a00:1450:4000::/36]; NEURAL_HAM_LONG(-1.00)[-1.000]; TO_DN_SOME(0.00)[]; MID_RHS_MATCH_FROMTLD(0.00)[]; DKIM_TRACE(0.00)[gmail.com:+]; DMARC_POLICY_ALLOW(-0.50)[gmail.com,none]; RCVD_IN_DNSWL_NONE(0.00)[2a00:1450:4864:20::129:from]; NEURAL_HAM_SHORT(-1.00)[-1.000]; FROM_EQ_ENVFROM(0.00)[]; MIME_TRACE(0.00)[0:+]; FREEMAIL_ENVFROM(0.00)[gmail.com]; ASN(0.00)[asn:15169, ipnet:2a00:1450::/32, country:US]; RCVD_COUNT_TWO(0.00)[2]; MAILMAN_DEST(0.00)[dev-commits-src-all,dev-commits-src-main]; DWL_DNSWL_NONE(0.00)[gmail.com:dkim] X-BeenThere: dev-commits-src-main@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for the main branch of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 23 Sep 2021 19:49:50 -0000 On 9/21/21, Konstantin Belousov wrote: > The branch main has been updated by kib: > > URL: > https://cgit.FreeBSD.org/src/commit/?id=df8dd6025af88a99d34f549fa9591a9b8f9b75b1 > > commit df8dd6025af88a99d34f549fa9591a9b8f9b75b1 > Author: Konstantin Belousov > AuthorDate: 2021-09-13 21:05:47 +0000 > Commit: Konstantin Belousov > CommitDate: 2021-09-21 17:20:15 +0000 > > amd64: stop using top of the thread' kernel stack for FPU user save > area > This causes KASAN to crash on boot: panic: ASan: Invalid access, 192-byte read at 0xffffffff84105f40, UseAfterScope(f8) cpuid = 0 time = 1 KDB: stack backtrace: db_trace_self_wrapper() at db_trace_self_wrapper+0xa5/frame 0xffffffff841056f0 kdb_backtrace() at kdb_backtrace+0xc9/frame 0xffffffff84105850 vpanic() at vpanic+0x248/frame 0xffffffff84105930 panic() at panic+0xb5/frame 0xffffffff841059f0 kasan_memmove() at kasan_memmove+0x313/frame 0xffffffff84105ac0 cpu_fork() at cpu_fork+0x19b/frame 0xffffffff84105b30 vm_forkproc() at vm_forkproc+0x18f/frame 0xffffffff84105b90 fork1() at fork1+0x1ff4/frame 0xffffffff84105d10 kproc_create() at kproc_create+0x166/frame 0xffffffff84105eb0 audit_worker_init() at audit_worker_init+0x41/frame 0xffffffff84105ed0 mi_startup() at mi_startup+0x340/frame 0xffffffff84105ff0 btext() at btext+0x22 > 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. > > A useful experiment now would be to reduce KSTACK_PAGES. > > Reviewed by: jhb, markj > Tested by: pho > Sponsored by: The FreeBSD Foundation > MFC after: 1 week > Differential revision: https://reviews.freebsd.org/D31954 > --- > sys/amd64/amd64/exec_machdep.c | 4 ++-- > sys/amd64/amd64/fpu.c | 2 ++ > sys/amd64/amd64/machdep.c | 14 -------------- > sys/amd64/amd64/vm_machdep.c | 22 +++++++++++++--------- > sys/amd64/ia32/ia32_signal.c | 6 +++--- > sys/amd64/include/proc.h | 2 ++ > sys/kern/kern_thread.c | 2 +- > 7 files changed, 23 insertions(+), 29 deletions(-) > > diff --git a/sys/amd64/amd64/exec_machdep.c > b/sys/amd64/amd64/exec_machdep.c > index 1297117638d6..48bda05f9685 100644 > --- a/sys/amd64/amd64/exec_machdep.c > +++ b/sys/amd64/amd64/exec_machdep.c > @@ -135,7 +135,7 @@ sendsig(sig_t catcher, ksiginfo_t *ksi, sigset_t *mask) > > if (cpu_max_ext_state_size > sizeof(struct savefpu) && use_xsave) { > xfpusave_len = cpu_max_ext_state_size - sizeof(struct savefpu); > - xfpusave = __builtin_alloca(xfpusave_len); > + xfpusave = (char *)td->td_md.md_fpu_scratch; > } else { > xfpusave_len = 0; > xfpusave = NULL; > @@ -674,7 +674,7 @@ set_mcontext(struct thread *td, mcontext_t *mcp) > if (mcp->mc_xfpustate_len > cpu_max_ext_state_size - > sizeof(struct savefpu)) > return (EINVAL); > - xfpustate = __builtin_alloca(mcp->mc_xfpustate_len); > + xfpustate = (char *)td->td_md.md_fpu_scratch; > ret = copyin((void *)mcp->mc_xfpustate, xfpustate, > mcp->mc_xfpustate_len); > if (ret != 0) > diff --git a/sys/amd64/amd64/fpu.c b/sys/amd64/amd64/fpu.c > index d7936b3b1922..24986958d4ca 100644 > --- a/sys/amd64/amd64/fpu.c > +++ b/sys/amd64/amd64/fpu.c > @@ -448,6 +448,8 @@ fpuinitstate(void *arg __unused) > xsave_area_elm_descr), M_DEVBUF, M_WAITOK | M_ZERO); > } > > + cpu_thread_alloc(&thread0); > + > saveintr = intr_disable(); > stop_emulating(); > > diff --git a/sys/amd64/amd64/machdep.c b/sys/amd64/amd64/machdep.c > index d4e2356a9ae1..5c9b64526609 100644 > --- a/sys/amd64/amd64/machdep.c > +++ b/sys/amd64/amd64/machdep.c > @@ -1258,7 +1258,6 @@ hammer_time(u_int64_t modulep, u_int64_t physfree) > caddr_t kmdp; > int gsel_tss, x; > struct pcpu *pc; > - struct xstate_hdr *xhdr; > uint64_t cr3, rsp0; > pml4_entry_t *pml4e; > pdp_entry_t *pdpe; > @@ -1564,19 +1563,6 @@ hammer_time(u_int64_t modulep, u_int64_t physfree) > msgbufinit(msgbufp, msgbufsize); > fpuinit(); > > - /* > - * Reinitialize thread0's stack base now that the xsave area size is > - * known. Set up thread0's pcb save area after fpuinit calculated fpu > - * save area size. Zero out the extended state header in fpu save area. > - */ > - set_top_of_stack_td(&thread0); > - thread0.td_pcb->pcb_save = get_pcb_user_save_td(&thread0); > - bzero(thread0.td_pcb->pcb_save, cpu_max_ext_state_size); > - if (use_xsave) { > - xhdr = (struct xstate_hdr *)(get_pcb_user_save_td(&thread0) + > - 1); > - xhdr->xstate_bv = xsave_mask; > - } > /* make an initial tss so cpu can get interrupt stack on syscall! */ > rsp0 = thread0.td_md.md_stack_base; > /* Ensure the stack is aligned to 16 bytes */ > diff --git a/sys/amd64/amd64/vm_machdep.c b/sys/amd64/amd64/vm_machdep.c > index 4567e6e0eb5d..e42d16d61b3a 100644 > --- a/sys/amd64/amd64/vm_machdep.c > +++ b/sys/amd64/amd64/vm_machdep.c > @@ -90,19 +90,17 @@ void > set_top_of_stack_td(struct thread *td) > { > td->td_md.md_stack_base = td->td_kstack + > - td->td_kstack_pages * PAGE_SIZE - > - roundup2(cpu_max_ext_state_size, XSAVE_AREA_ALIGN); > + td->td_kstack_pages * PAGE_SIZE; > } > > struct savefpu * > get_pcb_user_save_td(struct thread *td) > { > - vm_offset_t p; > - > - p = td->td_md.md_stack_base; > - KASSERT((p % XSAVE_AREA_ALIGN) == 0, > - ("Unaligned pcb_user_save area ptr %#lx td %p", p, td)); > - return ((struct savefpu *)p); > + KASSERT(((vm_offset_t)td->td_md.md_usr_fpu_save % > + XSAVE_AREA_ALIGN) == 0, > + ("Unaligned pcb_user_save area ptr %p td %p", > + td->td_md.md_usr_fpu_save, td)); > + return (td->td_md.md_usr_fpu_save); > } > > struct pcb * > @@ -393,6 +391,8 @@ cpu_thread_alloc(struct thread *td) > set_top_of_stack_td(td); > td->td_pcb = pcb = get_pcb_td(td); > td->td_frame = (struct trapframe *)td->td_md.md_stack_base - 1; > + td->td_md.md_usr_fpu_save = fpu_save_area_alloc(); > + td->td_md.md_fpu_scratch = fpu_save_area_alloc(); > pcb->pcb_save = get_pcb_user_save_pcb(pcb); > if (use_xsave) { > xhdr = (struct xstate_hdr *)(pcb->pcb_save + 1); > @@ -404,8 +404,12 @@ cpu_thread_alloc(struct thread *td) > void > cpu_thread_free(struct thread *td) > { > - > cpu_thread_clean(td); > + > + fpu_save_area_free(td->td_md.md_usr_fpu_save); > + td->td_md.md_usr_fpu_save = NULL; > + fpu_save_area_free(td->td_md.md_fpu_scratch); > + td->td_md.md_fpu_scratch = NULL; > } > > bool > diff --git a/sys/amd64/ia32/ia32_signal.c b/sys/amd64/ia32/ia32_signal.c > index 49b5797d68fd..9b67c7001a87 100644 > --- a/sys/amd64/ia32/ia32_signal.c > +++ b/sys/amd64/ia32/ia32_signal.c > @@ -210,7 +210,7 @@ ia32_set_mcontext(struct thread *td, struct > ia32_mcontext *mcp) > if (mcp->mc_xfpustate_len > cpu_max_ext_state_size - > sizeof(struct savefpu)) > return (EINVAL); > - xfpustate = __builtin_alloca(mcp->mc_xfpustate_len); > + xfpustate = (char *)td->td_md.md_fpu_scratch; > ret = copyin(PTRIN(mcp->mc_xfpustate), xfpustate, > mcp->mc_xfpustate_len); > if (ret != 0) > @@ -579,7 +579,7 @@ ia32_sendsig(sig_t catcher, ksiginfo_t *ksi, sigset_t > *mask) > > if (cpu_max_ext_state_size > sizeof(struct savefpu) && use_xsave) { > xfpusave_len = cpu_max_ext_state_size - sizeof(struct savefpu); > - xfpusave = __builtin_alloca(xfpusave_len); > + xfpusave = (char *)td->td_md.md_fpu_scratch; > } else { > xfpusave_len = 0; > xfpusave = NULL; > @@ -882,7 +882,7 @@ freebsd32_sigreturn(td, uap) > td->td_proc->p_pid, td->td_name, xfpustate_len); > return (EINVAL); > } > - xfpustate = __builtin_alloca(xfpustate_len); > + xfpustate = (char *)td->td_md.md_fpu_scratch; > error = copyin(PTRIN(ucp->uc_mcontext.mc_xfpustate), > xfpustate, xfpustate_len); > if (error != 0) { > diff --git a/sys/amd64/include/proc.h b/sys/amd64/include/proc.h > index 0f8cf50e326d..bd07f70f8d44 100644 > --- a/sys/amd64/include/proc.h > +++ b/sys/amd64/include/proc.h > @@ -75,6 +75,8 @@ struct mdthread { > int md_efirt_dis_pf; /* (k) */ > struct pcb md_pcb; > vm_offset_t md_stack_base; > + struct savefpu *md_usr_fpu_save; > + struct savefpu *md_fpu_scratch; > }; > > struct mdproc { > diff --git a/sys/kern/kern_thread.c b/sys/kern/kern_thread.c > index 65c5cc65c87e..62f939406374 100644 > --- a/sys/kern/kern_thread.c > +++ b/sys/kern/kern_thread.c > @@ -91,7 +91,7 @@ _Static_assert(offsetof(struct thread, td_pflags) == > 0x110, > "struct thread KBI td_pflags"); > _Static_assert(offsetof(struct thread, td_frame) == 0x4a8, > "struct thread KBI td_frame"); > -_Static_assert(offsetof(struct thread, td_emuldata) == 0x6b0, > +_Static_assert(offsetof(struct thread, td_emuldata) == 0x6c0, > "struct thread KBI td_emuldata"); > _Static_assert(offsetof(struct proc, p_flag) == 0xb8, > "struct proc KBI p_flag"); > -- Mateusz Guzik