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 78B7AC00140 for ; Thu, 18 Aug 2022 21:27:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1345944AbiHRV1S (ORCPT ); Thu, 18 Aug 2022 17:27:18 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40258 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1347084AbiHRV1A (ORCPT ); Thu, 18 Aug 2022 17:27:00 -0400 Received: from mail-pj1-x1031.google.com (mail-pj1-x1031.google.com [IPv6:2607:f8b0:4864:20::1031]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5E31DEA8AB for ; Thu, 18 Aug 2022 14:19:15 -0700 (PDT) Received: by mail-pj1-x1031.google.com with SMTP id a8so2848013pjg.5 for ; Thu, 18 Aug 2022 14:19:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc; bh=tTJTjqw4MggpWtxGKCzfbFeqFxFbwJEwUmHLuAFNlC4=; b=KLY0b+bh0hfzUpqKcJhe/wX0zYM3DwWqLn+1EFPX/z/mvu9FX8S6481IG9EwIwKlr0 sksbXHJQ4Kh7CSdeJ54/AGd442l6xfwUSwbgjuIY7cPkTmb/P8QcpBZsFw1/AEgW7BxO Z7vFc1cM6wHyTLrXmXs4BpPTKJqKJm4CDzUsmuDRJO5YqQ2oTdhgmDQfcISIZdvLXi+a iIdIuaNiTdK6EZBOyO8Sqr7BsD7W8hLLdMZHw8f/Q35ywyTfALa1cNGq5gHNhppDaPfZ O+kBBnOCDICeLF6quWO+kUt7vyDNo8jD0dQqib2ZIPNPKJ57Uq0YHy8r7Fi2W5SNMe/A HD3w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc; bh=tTJTjqw4MggpWtxGKCzfbFeqFxFbwJEwUmHLuAFNlC4=; b=Hr+X/P0ABpI+bVO2KXk9w+H14UPaN/qEkzOSgKrZeYflKajUjsJxRgiuJq/h+ofowS bjLrsawwQ0iAEwV7oFqV5RkaC/Mf4DbaEbkLCi2EcicYi4dLLH1CjiAne/s6KiRSaOhR jSp0NTK5Qrg6aJGPuXhppeOo5NX8LY0ijLe3cvpGQ3bIxWq9jzc711E8c/rahQ2EQxAr aPCUxpBLGN2NwE4H3iBJz4enVs6vChvJ6zmMLm2ZepeZz/u9a+hjqiNgo30Lyvvqduif MCeLr2ur3J4DjCwHhyA1VVet8+pvWWq9nfG9OVSEIroNRvfZREkmYmI4MZ7EhGm5imCp 8Zbg== X-Gm-Message-State: ACgBeo1xI7IS0qYEbkNoxBnYsgoAhdIDxOinDOK2XFU1lYlY7qrCwCUc 1e/HvjxE8WlhmHoyxy/kj9wS/Q== X-Google-Smtp-Source: AA6agR6y0AhMETJyQ+vXl0SNQkEBxWGMTcxU9uSEZQLV6JUuiIN57zfeFgll2QLSPF2S39iiIToTNg== X-Received: by 2002:a17:90b:4c4b:b0:1f4:d176:aa5a with SMTP id np11-20020a17090b4c4b00b001f4d176aa5amr4899401pjb.233.1660857553755; Thu, 18 Aug 2022 14:19:13 -0700 (PDT) Received: from google.com (7.104.168.34.bc.googleusercontent.com. [34.168.104.7]) by smtp.gmail.com with ESMTPSA id h8-20020a170902f54800b0016ee328fd61sm1819751plf.198.2022.08.18.14.19.13 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 18 Aug 2022 14:19:13 -0700 (PDT) Date: Thu, 18 Aug 2022 21:19:09 +0000 From: Sean Christopherson To: Kyle Huey Cc: Thomas Gleixner , Dave Hansen , Borislav Petkov , Ingo Molnar , x86@kernel.org, "H. Peter Anvin" , Paolo Bonzini , Andy Lutomirski , Peter Zijlstra , linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org, Robert O'Callahan , David Manouchehri , Borislav Petkov , kvm@vger.kernel.org, stable@vger.kernel.org Subject: Re: [PATCH v5 1/2] x86/fpu: Allow PKRU to be (once again) written by ptrace. Message-ID: References: <20220808141538.102394-1-khuey@kylehuey.com> <87ilmpzunz.ffs@tglx> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kselftest@vger.kernel.org On Thu, Aug 18, 2022, Kyle Huey wrote: > On Thu, Aug 18, 2022 at 3:57 AM Thomas Gleixner wrote: > > On Mon, Aug 08 2022 at 07:15, Kyle Huey wrote: > > > When management of the PKRU register was moved away from XSTATE, emulation > > > of PKRU's existence in XSTATE was added for APIs that read XSTATE, but not > > > for APIs that write XSTATE. This can be seen by running gdb and executing > > > `p $pkru`, `set $pkru = 42`, and `p $pkru`. On affected kernels (5.14+) the > > > write to the PKRU register (which gdb performs through ptrace) is ignored. > > > > > > There are three relevant APIs: PTRACE_SETREGSET with NT_X86_XSTATE, > > > sigreturn, and KVM_SET_XSAVE. KVM_SET_XSAVE has its own special handling to > > > make PKRU writes take effect (in fpu_copy_uabi_to_guest_fpstate). Push that > > > down into copy_uabi_to_xstate and have PTRACE_SETREGSET with NT_X86_XSTATE > > > and sigreturn pass in pointers to the appropriate PKRU value. > > > > > > This also adds code to initialize the PKRU value to the hardware init value > > > (namely 0) if the PKRU bit is not set in the XSTATE header to match XRSTOR. > > > This is a change to the current KVM_SET_XSAVE behavior. > > > > You are stating a fact here, but provide 0 justification why this is > > correct. > > Well, the justification is that this *is* the behavior we want for > ptrace/sigreturn, and it's very likely the existing KVM_SET_XSAVE > behavior in this edge case is an oversight rather than intentional, > and in the absence of confirmation that KVM wants the existing > behavior (the KVM mailing list and maintainer are CCd) one correct > code path is better than one correct code path and one buggy code > path. Sorry, I missed the KVM-relevant flags. Hrm, the current behavior has been KVM ABI for a very long time. It's definitely odd because all other components will be initialized due to their bits being cleared in the header during kvm_load_guest_fpu(), and it probably wouldn't cause problems in practice as most VMMs likely do "all or nothing" loads. But, in theory, userspace could save/restore a subset of guest XSTATE and rely on the kernel not overwriting guest PKRU when its bit is cleared in the header. All that said, I don't see any reason to force KVM to change at this time, it's trivial enough to handle KVM's oddities while providing sane behavior for others. Nullify the pointer in the guest path and then update copy_uabi_to_xstate() to play nice with a NULL pointer, e.g. /* * 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). */ if (!(kstate->regs.xsave.header.xfeatures & XFEATURE_MASK_PKRU)) vpkru = NULL; return copy_uabi_from_kernel_to_xstate(kstate, ustate, vpkru);