From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6C505CFA746 for ; Fri, 4 Oct 2024 04:20:19 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4C0946B0418; Fri, 4 Oct 2024 00:20:18 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 44A006B041A; Fri, 4 Oct 2024 00:20:18 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2C4A76B041B; Fri, 4 Oct 2024 00:20:18 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 0A7836B0418 for ; Fri, 4 Oct 2024 00:20:18 -0400 (EDT) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 60F59C1054 for ; Fri, 4 Oct 2024 04:20:17 +0000 (UTC) X-FDA: 82634617674.12.FE50BAC Received: from mail-oa1-f45.google.com (mail-oa1-f45.google.com [209.85.160.45]) by imf05.hostedemail.com (Postfix) with ESMTP id 8FE94100006 for ; Fri, 4 Oct 2024 04:20:15 +0000 (UTC) Authentication-Results: imf05.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b=Kf7R3JwK; dmarc=pass (policy=none) header.from=chromium.org; spf=pass (imf05.hostedemail.com: domain of jeffxu@chromium.org designates 209.85.160.45 as permitted sender) smtp.mailfrom=jeffxu@chromium.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1728015593; a=rsa-sha256; cv=none; b=fQ2CMt38UL/BCF8aXVtvij4AMuMZpXn1fj/pCUv4fTc1RwecropSxgHBQdxg9/oFvLH0cP CE7veyDNI07gd1JJuMU5E/6/xq6dPZ7HZCRQs0dcE1xf45D50fwdKG81srW7AugjP43HIA pthoaX/l4rTOE7g0Xcr/Ck7yTHYWhXA= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b=Kf7R3JwK; dmarc=pass (policy=none) header.from=chromium.org; spf=pass (imf05.hostedemail.com: domain of jeffxu@chromium.org designates 209.85.160.45 as permitted sender) smtp.mailfrom=jeffxu@chromium.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1728015593; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=9D6a3sjkcgkIB7stodJ55CiLUsQO/kpb3sBj9c0NCvU=; b=wNqh/BbqEoUKIz/+ljnSVz4PAYtnEbOU6dSlci67yDTAUDNbJWuBa/t6iVqqQU4Yu763IP i5To2H/tJb2mADiCuJYJgLxINV4p3rG5x/tDlojtI3vfp3vCGDm5JFQe30WXR3ELMcETxZ 3MAsirv8BCuVSI/nm/iX/v5EgyEUul8= Received: by mail-oa1-f45.google.com with SMTP id 586e51a60fabf-268d0979e90so46760fac.3 for ; Thu, 03 Oct 2024 21:20:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1728015614; x=1728620414; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=9D6a3sjkcgkIB7stodJ55CiLUsQO/kpb3sBj9c0NCvU=; b=Kf7R3JwK4VjrEv9Fx+k0yJBzxYdaP9VUc5iY8EoRqy2LMCYJNdRrmCWNqlKZRORVcG KwRadb3xiD1rm7OJSgagwk6SL5ZaJczotVGIyOsL6FiBKUGYMzsD3iF9nozhSDU3nfZ+ 75e4Rk8g/rscwtS+dboEJC/H6HaBxQ+3yKFUY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1728015614; x=1728620414; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=9D6a3sjkcgkIB7stodJ55CiLUsQO/kpb3sBj9c0NCvU=; b=bGf5XQ6bd3WR2GrEv618EMoklMdrmfh3sykBu9FMtcFxm70AEMlhfT/+ORhvaSpjiJ PDibgQd0u/oeWNo7726oXjo/M/Uj7jmEcwspRpDvPFWIjOxboNmi1Od8CurcuZ5ZVhx9 l9cHMDf7l4lf7/jkzcpo7hXnZgSaogEDDHrtZHMq9GqzkPCAZOMnhZ08+Ls52pp2jfML K1cVnq9DO+LCgZBnc/3LWP1cW94uWiuVqRpBKlrM9jP5Rf9ZZ3Vd9HJl3Ayp0cfyFrWI EbYcSPnGpR+I2jkUG4ORVWMK2S197NpIT/mWxD6e2DilbsD2mqSObDNYLzagMKcUIslv efBA== X-Forwarded-Encrypted: i=1; AJvYcCUgHly/zunh3FHeQLZqVEah/bIB/M9we4H6NEB/lUIpd0mZbb5Gd5RWL0Y7yiLupbJ7mBxIjzR6WA==@kvack.org X-Gm-Message-State: AOJu0YwDVfVVroW2axpXDYEqKZUlqmUJ2IuLzJX7pKQecgDuNJsQGDA/ 35rNWFsKqAjWz0Hp7HVMuVZaOZ+8EjMBkPRCCpLRhLyQWiK/vhbXQ8bEJrRh3UxyDbYeleYBWJ4 GFapHBbFqHTF9DWUM1LYkyg9B9MmLzjr8lZoX X-Google-Smtp-Source: AGHT+IH8SmjSEXOSyI1LK1xSB5a+42f9VjYQMxJbHcL7+k2zMTyV6ZhyWGSnSIcxRw+nD46Z5BgGJbXgnWutgcyIF0s= X-Received: by 2002:a05:6870:b28f:b0:27b:9f8b:277c with SMTP id 586e51a60fabf-287c2007b9dmr321825fac.11.1728015614440; Thu, 03 Oct 2024 21:20:14 -0700 (PDT) MIME-Version: 1.0 References: <20240802061318.2140081-1-aruna.ramakrishna@oracle.com> <20240802061318.2140081-4-aruna.ramakrishna@oracle.com> In-Reply-To: From: Jeff Xu Date: Thu, 3 Oct 2024 21:20:01 -0700 Message-ID: Subject: Re: [PATCH v8 3/5] x86/pkeys: Update PKRU to enable all pkeys before XSAVE To: Aruna Ramakrishna Cc: "linux-kernel@vger.kernel.org" , "x86@kernel.org" , "dave.hansen@linux.intel.com" , Thomas Gleixner , Ingo Molnar , "linux-mm@kvack.org" , Keith Lucas , "rick.p.edgecombe@intel.com" , Jorge Lucangeli Obes , Kees Cook , =?UTF-8?Q?Stephen_R=C3=B6ttger?= , Jann Horn Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Stat-Signature: uqpwiesjg77iyzxn69embzib13c57yai X-Rspamd-Queue-Id: 8FE94100006 X-Rspamd-Server: rspam02 X-HE-Tag: 1728015615-520963 X-HE-Meta: U2FsdGVkX1/nCD+DyTxEWaSgZfogtwjq0O629sqzF9dKUDDZ+ktn+WsjYla1fS4VsQAqGlCEMEr1oCxKEIxMEvTPgUIQlSu/LIHbjEf4tlBn7W+co+HlTWvJtHFPJvuhLGHdRU0njI7zpDKAJsNvGt4vZz3mYS1MYlcWIPWXfR0aZQ8XpMKqAKMqadwx5MwwEdKpJPb8qHvsMjN7sIPBmS/tRN278r1Gass5IQoZWv5g+DLoTmW5ykaWRo1gz0thBF2Y0NMfVDZOHAaSX9lJIFAgY5N6VvsbWs5VzzO4pXTqMwx0udrzUlLWROJBmiTHJivk4sF/M4GggqNEMv+/rLKdDWeznBZ2L2YRt2bymKRwUTl9O2xoQ0+XtzLHBeBzwqStRY7AjdiQchl8NF698ucezOMorDXqSaN4hLt6KgAfzt2fTy4ZgWbCQd+53uOJg5x++fJmBS5d7JYWNSHbUIDT5n+ApWY3DsOfFQ+nFGRfn2tVtJ2RVMBh6OTdmWLF+Q2zURkzIJsA22fbHxnwwLnnUC1jjyrlC0oABtfiouOrk54IC3iOOqjej/tVRvnl1UA0bEyzbwvWRGo3uBb1IyU9M8j4aKJspnMUoK7tZ2/4ErXvPHR1tyg6DTre19v7qonARc6xwqIRb/8Y7XaryWiOd64Y5XTygS2E14kbrSkLoQe7mjJG3Gi+uCWsiCXCZLBR6lQ0SCc80yq7QGcqftQAZRNrKhMnqfDKLJX8ug5Qe1CLssP0tiyNqgHfLo9AdOZZOO1Zfo3WYLB79hp9K6xyK0piuK9VouXYgmK8AbyOR3iYe9Fiv6dDtHbyd8ztK1EX6rXQHPqWJ5fko8blvrBQV2pNMJwM/UzLxIWR7NVKQQ9ScysKyKw5U2zHyDE1iqEi2/PRrK1por7EOBJuGjLntXutcXCKtEciUORBAxHpTeuzBhhCTc07eodyE/CTdEIhIfz+g+8v1xJe1mB gVHDS1hK KP/93wHgzYayr+QrOFcopF/JtwTGFaKzV7gp5wMPBJ1x/wrGigJSdn3I0CSm8vqqwOXnfjEFpe7z+upb2MnFKUADN0VHIpdcYmqDUmHIC/QWpqBFja44UkAiNqX4moW5TRdGJ/ol3Deyw5+Fa6WWWjOBlwkgdb9xCzwvYtJNAA2Zsoeh+STM6TrorOjTol2pBMmKETjvDvpShLvuHSnOHV7VXzuLOhbddCJe0mCR7+WOH/iDIPJR0zXsQ/WBw/g1IFqvtDBjgOjQ8N49eelWAl+fosBgR+pj7Q2k9YzzqQDq8CNpMZjsaAN9lDQ== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: Hi Aruna On Thu, Oct 3, 2024 at 4:29=E2=80=AFPM Aruna Ramakrishna wrote: > > > > > On Aug 26, 2024, at 5:53=E2=80=AFPM, Jeff Xu wrot= e: > > > > Hi Aruna > > > > On Fri, Aug 9, 2024 at 10:30=E2=80=AFAM Jeff Xu w= rote: > >> > >> On Thu, Aug 1, 2024 at 11:13=E2=80=AFPM Aruna Ramakrishna > >> wrote: > >>> > >>> If the alternate signal stack is protected by a different pkey than t= he > >>> current execution stack, copying xsave data to the sigaltstack will f= ail > >>> if its pkey is not enabled in the PKRU register. > >>> > >>> We do not know which pkey was used by the application for the altstac= k, > >>> so enable all pkeys before xsave. > >>> > >>> But this updated PKRU value is also pushed onto the sigframe, which > >>> means the register value restored from sigcontext will be different f= rom > >>> the user-defined one, which is unexpected. Fix that by overwriting th= e > >>> PKRU value on the sigframe with the original, user-defined PKRU. > >>> > >>> Signed-off-by: Aruna Ramakrishna > >>> --- > >>> arch/x86/kernel/fpu/signal.c | 11 +++++++++-- > >>> arch/x86/kernel/signal.c | 12 ++++++++++-- > >>> 2 files changed, 19 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signa= l.c > >>> index 931c5469d7f3..1065ab995305 100644 > >>> --- a/arch/x86/kernel/fpu/signal.c > >>> +++ b/arch/x86/kernel/fpu/signal.c > >>> @@ -168,8 +168,15 @@ static inline bool save_xstate_epilog(void __use= r *buf, int ia32_frame, > >>> > >>> static inline int copy_fpregs_to_sigframe(struct xregs_state __user *= buf, u32 pkru) > >>> { > >>> - if (use_xsave()) > >>> - return xsave_to_user_sigframe(buf); > >>> + int err =3D 0; > >>> + > >>> + if (use_xsave()) { > >>> + err =3D xsave_to_user_sigframe(buf); > >>> + if (!err) > >>> + err =3D update_pkru_in_sigframe(buf, pkru); > >>> + return err; > >>> + } > >>> + > >>> if (use_fxsr()) > >>> return fxsave_to_user_sigframe((struct fxregs_state __= user *) buf); > >>> else > >>> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c > >>> index 9dc77ad03a0e..5f441039b572 100644 > >>> --- a/arch/x86/kernel/signal.c > >>> +++ b/arch/x86/kernel/signal.c > >>> @@ -102,7 +102,7 @@ get_sigframe(struct ksignal *ksig, struct pt_regs= *regs, size_t frame_size, > >>> unsigned long math_size =3D 0; > >>> unsigned long sp =3D regs->sp; > >>> unsigned long buf_fx =3D 0; > >>> - u32 pkru =3D read_pkru(); > >>> + u32 pkru; > >>> > >>> /* redzone */ > >>> if (!ia32_frame) > >>> @@ -157,9 +157,17 @@ get_sigframe(struct ksignal *ksig, struct pt_reg= s *regs, size_t frame_size, > >>> return (void __user *)-1L; > >>> } > >>> > >>> + /* Update PKRU to enable access to the alternate signal stack= . */ > >>> + pkru =3D sig_prepare_pkru(); > >> I think, the better place to call sig_prepare_pkru() at begging of the > >> get_sigframe: > >> > >> get_sigframe() > >> { > >> /* ... */ > >> if (ka->sa.sa_flags & SA_ONSTACK) { > >> if (sas_ss_flags(sp) =3D=3D 0) { > >> // set pkru =3D 0 <--- set pkru =3D 0 here. > >> entering_altstack =3D true; > >> } > >> } > >> For two reasons: > >> - We probably don't want all signaling handling going through "PKRU=3D= 0" > >> , this includes the regular stack and nested signaling handling. The > >> best place is when "entering the altstack". IIUC, this feature only > >> enabled when sigaltstack() is used. > >> - The thread might not have read-access to the altstack, so we will > >> want to make sure that pkru=3D0 is set before any read to the altstack= . > >> And IIRC, fpu__alloc_mathframe needs read-access to the altstack. > >> To help with testing, I will send a test case to demo the usage. > > Apologies, I remembered it wrong, the fpu__alloc_mathframe doesn't > > need read-access to altstack. > > > > But I think the best place to set pkru=3D0 is still when the stack > > entering altstack, as shown above. the reason is: The signal stack can > > be nested, i.e. trigger another signaling inside signal handler and we > > don't need to set pkru=3D0 multiple times (only the first time enter > > sigaltstack) > > > >> (please give me sometime to organize the test code, I'm hoping to send > >> out before the end of next week) > >> > > the test code is placed at [1] > > [1] https://github.com/peaktocreek/pkeytest > > > >> Also, could you please consider adding a new flag SS_PKEYALTSTACK (see > >> SS_AUTODISARM for example). Most existing apps that use sigaltstack() > >> don't use PKEY and don't care about setting PKRU=3D0, and don't need = to > >> overwrite the sig frame after XSAVE. The flag will limit the impact > >> of this patch. > >> > > Thanks > > -Jeff > > > > Hi Jeff, > > I apologize for the delay in my response. I haven=E2=80=99t had a chance = to optimize > this patchset or try new test cases. > Not a problem, I understand. > I agree with your first suggestion that we can set pkru to 0 only when > entering_altstack =3D true, as that is the intention of this flow anyway. > > I=E2=80=99m not so sure if SS_PKEYALTSTACK is really necessary - theoreti= cally it seems > logical to not do this for applications that do not use pkeys but use alt= stack, but > it adds extra code/checks for possibly insignificant performance gain. I = have not > tested this yet. > One of the reasons that I'm thinking about is that signaling handling has real-time requirements for some real-time systems, and applying PKRU=3D0 without checking will increase cost to those systems and might be viewed as a regression. Thanks -Jeff > I=E2=80=99ll try to retest and send out a new patchset on top of the prev= ious ones that have > been merged to 6.12. > > Thank you for your feedback, and your attention to detail - I appreciate = it. > > Thanks, > Aruna > >