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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 98D19C433FE for ; Fri, 11 Nov 2022 16:38:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233023AbiKKQiL (ORCPT ); Fri, 11 Nov 2022 11:38:11 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60818 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232341AbiKKQiE (ORCPT ); Fri, 11 Nov 2022 11:38:04 -0500 Received: from mail-yb1-xb30.google.com (mail-yb1-xb30.google.com [IPv6:2607:f8b0:4864:20::b30]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9374C836B0 for ; Fri, 11 Nov 2022 08:38:03 -0800 (PST) Received: by mail-yb1-xb30.google.com with SMTP id i131so6256979ybc.9 for ; Fri, 11 Nov 2022 08:38:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kylehuey.com; s=google; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=K6NvtIX+Lbvb+KUOrKTR0ljTcijw4msq9rtYQAvDAUk=; b=ZIV1Enf0EAuq2JCxnGrXKI8jk/7tY3he6qJy8sZcYtjGNNiLD0dUKQN26hcwcVr7g0 ZHSsqZwjYd/amA7hKKNWDI9b5LZ19MVL2uGtfJ2XCvRFB4DlM9Gj7XdDctYmU/Rqnvt8 5kasCPXeKaizIqzuDA7tWJaJx/RmSqeAsezseVn+JObhr7w+AnWOUt0cuM9yl9qdfNRo NOxm7kb/aTOMFj+7vJMjknh6lMSUQl08qkuBuQHNCypelX6FicXzpCsPfN44ihrZ+VP4 bAaZI7hDq6zsG384dY8S0x+JWdVqUt+Z1OxRjg3HZXFn5EboRSLm6+ZTiDvpmh+RGCqg LumA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=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=K6NvtIX+Lbvb+KUOrKTR0ljTcijw4msq9rtYQAvDAUk=; b=vImwZRugkinSd6Mkc7AZRHicPdsYbfhYEkLc+8jLR2DFuAFFbMZP5hS71A3g9JLWYc v6gwoPnHb+rnM8msy+/NERbiu3qyMZAKxqrprzHA9ZNMAxJPz6612SBG+GtSmq1XNozr o0OGkYFhgZXU5pfgrVxFSH62bmaK66vWlQhgKYiFY9Y4PlHy5c98jkVf2bOaGY5OsmcQ bb60FvHXISoyCDi/s54bz56otc5H5aeKSIZIs2mSQfb5XlbMADzQKwu6FCc1xbETnZ9i aiyps0CJHr/oSAS9/W4D89O5TDD0QizyjLHh5Wb0kfBL9hlIcVXz8OngpaMY2n04aWwf 2Mqg== X-Gm-Message-State: ANoB5pm4ik4/Fx30K0TJ1qFXVe44koiBlKts+62wXeOsRjBUGKiTCPWH 8pbuYWClWnmuXfeFQnfiHMoj8sOKDPIJ5QqXu4snqA== X-Google-Smtp-Source: AA0mqf59F1exMtt7BFu3TKN2pQsxyZEPjDy9r/NuB/KlmIuT83XtZxGvU1wIl1kHMhe4l07A5h8qeMI+Z94l5cp/ee4= X-Received: by 2002:a5b:38b:0:b0:6cb:75a6:3bdb with SMTP id k11-20020a5b038b000000b006cb75a63bdbmr2407196ybp.96.1668184682697; Fri, 11 Nov 2022 08:38:02 -0800 (PST) MIME-Version: 1.0 References: <20221107063807.81774-1-khuey@kylehuey.com> <20221107063807.81774-2-khuey@kylehuey.com> <64e62ab9-71f6-6d90-24de-402921c244e7@intel.com> In-Reply-To: From: Kyle Huey Date: Fri, 11 Nov 2022 08:37:49 -0800 Message-ID: Subject: Re: [RESEND PATCH v6 1/2] x86/fpu: Allow PKRU to be (once again) written by ptrace. To: Dave Hansen Cc: Linus Torvalds , Dave Hansen , Thomas Gleixner , Borislav Petkov , Ingo Molnar , x86@kernel.org, "H. Peter Anvin" , Paolo Bonzini , Andy Lutomirski , Peter Zijlstra , Sean Christopherson , linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org, "Robert O'Callahan" , David Manouchehri , Borislav Petkov , stable@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Nov 10, 2022 at 5:38 PM Dave Hansen wrote: > > On 11/10/22 16:03, Kyle Huey wrote: > > On Tue, Nov 8, 2022 at 10:23 AM Dave Hansen wrote: > ... > >> At a high level, this patch does a *LOT*. Generally, it's nice when > >> bugfixes can be encapsulted in one patch, but I think there's too much > >> going on here for one patch. > > > > Ok. How about I break the first part into two pieces, one that changes the > > signatures of copy_uabi_from_kernel_to_xstate() and > > copy_sigframe_from_user_to_xstate(), and one that moves the relevant > > KVM code from fpu_copy_uabi_to_guest_fpstate() to copy_uabi_to_xstate() > > and deals with the edge case behavior of the mask? > > Sounds like a good start. My gut says there's another patch or two that > could be broken out, but that sounds like a reasonable next step. > > >>> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c > >>> index 3b28c5b25e12..c273669e8a00 100644 > >>> --- a/arch/x86/kernel/fpu/core.c > >>> +++ b/arch/x86/kernel/fpu/core.c > >>> @@ -391,8 +391,6 @@ int fpu_copy_uabi_to_guest_fpstate(struct fpu_guest *gfpu, const void *buf, > >>> { > >>> struct fpstate *kstate = gfpu->fpstate; > >>> const union fpregs_state *ustate = buf; > >>> - struct pkru_state *xpkru; > >>> - int ret; > >>> > >>> if (!cpu_feature_enabled(X86_FEATURE_XSAVE)) { > >>> if (ustate->xsave.header.xfeatures & ~XFEATURE_MASK_FPSSE) > >>> @@ -406,16 +404,16 @@ int fpu_copy_uabi_to_guest_fpstate(struct fpu_guest *gfpu, const void *buf, > >>> if (ustate->xsave.header.xfeatures & ~xcr0) > >>> return -EINVAL; > >>> > >>> - ret = copy_uabi_from_kernel_to_xstate(kstate, ustate); > >>> - if (ret) > >>> - return ret; > >>> + /* > >>> + * Nullify @vpkru to preserve its current value if PKRU's bit isn't set > >>> + * in the header. KVM's odd ABI is to leave PKRU untouched in this > >>> + * case (all other components are eventually re-initialized). > >>> + * (Not clear that this is actually necessary for compat). > >>> + */ > >>> + if (!(ustate->xsave.header.xfeatures & XFEATURE_MASK_PKRU)) > >>> + vpkru = NULL; > >> > >> I'm not a big fan of hunks that are part of bugfixes where it is not > >> clear that the hunk is necessary. > > > > This is necessary to avoid changing KVM's behavior at the same time > > that we change > > ptrace, since KVM doesn't want the same behavior as ptrace. > > Your "This is necessary" doesn't really match with "Not clear that this > is actually necessary" from the comment, right? > > Rather than claim whether it is necessary or not, maybe just say why > it's there: it's there to preserve wonky KVM behavior. > > BTW, I'd love to know if KVM *REALLY* depends on this. It'd be nice to > kill if not. qemu didn't appear to (it treats the KVM_GET_XSAVE2/KVM_SET_XSAVE buffers as opaque blobs afaict) but it's of course not the only KVM application out there. > >> Would something like this be more clear? > >> > >> if (hdr.xfeatures & XFEATURE_MASK_PKRU) { > >> struct pkru_state *xpkru; > >> > >> xpkru = __raw_xsave_addr(xsave, XFEATURE_PKRU); > >> *pkru = xpkru->pkru; > >> } else { > >> /* > >> * KVM may pass a NULL 'pkru' to indicate > >> * that it does not need PKRU updated. > >> */ > >> if (pkru) > >> *pkru = 0; > >> } > > > > Yeah, Sean Christopherson suggested this (with the else and if > > collapsed into a single level) when I submitted this previously. > > I generally agree with Sean, but he's also been guilty of an atrocity or > two over the years. :) While I generally like low levels of > indentation I also think my version is much more clear in this case. > - Kyle