From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sean Christopherson Date: Tue, 9 Nov 2021 01:17:39 +0000 Subject: [PATCH v5.5 01/30] KVM: Ensure local memslot copies operate on up-to-date arch-specific data In-Reply-To: <6407c2d3-854b-edf6-9990-b54a5baedd0a@oracle.com> References: <20211104002531.1176691-1-seanjc@google.com> <20211104002531.1176691-2-seanjc@google.com> <6407c2d3-854b-edf6-9990-b54a5baedd0a@oracle.com> Message-ID: List-Id: To: kvm-riscv@lists.infradead.org MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit On Tue, Nov 09, 2021, Maciej S. Szmigiero wrote: > On 04.11.2021 01:25, Sean Christopherson wrote: > > @@ -1597,6 +1596,26 @@ static int kvm_set_memslot(struct kvm *kvm, > > kvm_copy_memslots(slots, __kvm_memslots(kvm, as_id)); > > } > > + /* > > + * Make a full copy of the old memslot, the pointer will become stale > > + * when the memslots are re-sorted by update_memslots(), and the old > > + * memslot needs to be referenced after calling update_memslots(), e.g. > > + * to free its resources and for arch specific behavior. This needs to > > + * happen *after* (re)acquiring slots_arch_lock. > > + */ > > + slot = id_to_memslot(slots, new->id); > > + if (slot) { > > + old = *slot; > > + } else { > > + WARN_ON_ONCE(change != KVM_MR_CREATE); > > + memset(&old, 0, sizeof(old)); > > + old.id = new->id; > > + old.as_id = as_id; > > + } > > + > > + /* Copy the arch-specific data, again after (re)acquiring slots_arch_lock. */ > > + memcpy(&new->arch, &old.arch, sizeof(old.arch)); > > Had "new" been zero-initialized completely in __kvm_set_memory_region() > for safety (so it does not contain stack garbage - I don't mean just the > new.arch field in the "if (!old.npages)" branch in that function but the > whole struct) this line would be needed only in the "if (slot)" branch > above (as Ben said). > > Also, when patch 7 from this series removes this memcpy(), > kvm_arch_prepare_memory_region() does indeed receive this field > uninitialized - I know only x86 and ppcHV care > and kvm_alloc_memslot_metadata() or kvmppc_core_prepare_memory_region_hv() > then overwrites it unconditionally but it feels a bit wrong. > > I am almost certain that compiler would figure out to only actually > zero the fields that wouldn't be overwritten immediately anyway. > > But on the other hand, this patch is only a fix for code that's going > to be replaced anyway so perfection here probably isn't that important. Yeah, that about sums up my feelings about the existing code. That said, an individual memslot isn't _that_ big, and memslot updates without the scalable implementation are dreadfully slow anyways, so I'm leaning strongly toward your suggestion of zeroing all of new as part of this fix. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sean Christopherson Date: Tue, 09 Nov 2021 01:17:39 +0000 Subject: Re: [PATCH v5.5 01/30] KVM: Ensure local memslot copies operate on up-to-date arch-specific data Message-Id: List-Id: References: <20211104002531.1176691-1-seanjc@google.com> <20211104002531.1176691-2-seanjc@google.com> <6407c2d3-854b-edf6-9990-b54a5baedd0a@oracle.com> In-Reply-To: <6407c2d3-854b-edf6-9990-b54a5baedd0a@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: "Maciej S. Szmigiero" Cc: Anup Patel , Wanpeng Li , kvm@vger.kernel.org, David Hildenbrand , linux-kernel@vger.kernel.org, Paul Mackerras , Atish Patra , Ben Gardon , linux-riscv@lists.infradead.org, Claudio Imbrenda , kvmarm@lists.cs.columbia.edu, Janosch Frank , Marc Zyngier , Joerg Roedel , Huacai Chen , Christian Borntraeger , Aleksandar Markovic , Palmer Dabbelt , Albert Ou , kvm-ppc@vger.kernel.org, Paul Walmsley , linux-arm-kernel@lists.infradead.org, Jim Mattson , Cornelia Huck , linux-mips@vger.kernel.org, kvm-riscv@lists.infradead.org, Paolo Bonzini , Vitaly Kuznetsov On Tue, Nov 09, 2021, Maciej S. Szmigiero wrote: > On 04.11.2021 01:25, Sean Christopherson wrote: > > @@ -1597,6 +1596,26 @@ static int kvm_set_memslot(struct kvm *kvm, > > kvm_copy_memslots(slots, __kvm_memslots(kvm, as_id)); > > } > > + /* > > + * Make a full copy of the old memslot, the pointer will become stale > > + * when the memslots are re-sorted by update_memslots(), and the old > > + * memslot needs to be referenced after calling update_memslots(), e.g. > > + * to free its resources and for arch specific behavior. This needs to > > + * happen *after* (re)acquiring slots_arch_lock. > > + */ > > + slot = id_to_memslot(slots, new->id); > > + if (slot) { > > + old = *slot; > > + } else { > > + WARN_ON_ONCE(change != KVM_MR_CREATE); > > + memset(&old, 0, sizeof(old)); > > + old.id = new->id; > > + old.as_id = as_id; > > + } > > + > > + /* Copy the arch-specific data, again after (re)acquiring slots_arch_lock. */ > > + memcpy(&new->arch, &old.arch, sizeof(old.arch)); > > Had "new" been zero-initialized completely in __kvm_set_memory_region() > for safety (so it does not contain stack garbage - I don't mean just the > new.arch field in the "if (!old.npages)" branch in that function but the > whole struct) this line would be needed only in the "if (slot)" branch > above (as Ben said). > > Also, when patch 7 from this series removes this memcpy(), > kvm_arch_prepare_memory_region() does indeed receive this field > uninitialized - I know only x86 and ppcHV care > and kvm_alloc_memslot_metadata() or kvmppc_core_prepare_memory_region_hv() > then overwrites it unconditionally but it feels a bit wrong. > > I am almost certain that compiler would figure out to only actually > zero the fields that wouldn't be overwritten immediately anyway. > > But on the other hand, this patch is only a fix for code that's going > to be replaced anyway so perfection here probably isn't that important. Yeah, that about sums up my feelings about the existing code. That said, an individual memslot isn't _that_ big, and memslot updates without the scalable implementation are dreadfully slow anyways, so I'm leaning strongly toward your suggestion of zeroing all of new as part of this fix. 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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 05FB3C433FE for ; Tue, 9 Nov 2021 01:17:51 +0000 (UTC) Received: from mm01.cs.columbia.edu (mm01.cs.columbia.edu [128.59.11.253]) by mail.kernel.org (Postfix) with ESMTP id 689B16134F for ; Tue, 9 Nov 2021 01:17:50 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 689B16134F Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.cs.columbia.edu Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id D895B4B1A4; Mon, 8 Nov 2021 20:17:49 -0500 (EST) X-Virus-Scanned: at lists.cs.columbia.edu Authentication-Results: mm01.cs.columbia.edu (amavisd-new); dkim=softfail (fail, message has been altered) header.i=@google.com Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id D28OWZ6XWrMM; Mon, 8 Nov 2021 20:17:48 -0500 (EST) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 5F2124B178; Mon, 8 Nov 2021 20:17:48 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 0B62E4B12F for ; Mon, 8 Nov 2021 20:17:47 -0500 (EST) X-Virus-Scanned: at lists.cs.columbia.edu Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id KdVeYXLUQA+V for ; Mon, 8 Nov 2021 20:17:45 -0500 (EST) Received: from mail-pl1-f180.google.com (mail-pl1-f180.google.com [209.85.214.180]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id 14BA44B11B for ; Mon, 8 Nov 2021 20:17:45 -0500 (EST) Received: by mail-pl1-f180.google.com with SMTP id q17so5779772plr.11 for ; Mon, 08 Nov 2021 17:17:45 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=gT3MUMtIZ0PS90yl0lE9QBzqwoRSMBq2QOt4vNKzTys=; b=b13sxWM9K5iisnYPadl4iOffaoys9RijOi3mTsZncT79yh0EYcQ5zP36xW3B53gbfI hf38uXQxIRmk/GO8JXWSnV+gRSRIGZYtVYUdIsHAt6HO9GbiW9mBH/HQFPbdGVhmJzAE d+5xCaoOiMJhRE25WFHguzeVSIifxnExemfwDQtGBdPF4S9WTr8CvKXghy1m1SqKVnwm EERujOqfJ1YA7bmWxiB6FDxmsEbHoK9/PEVJSGjKx5CzfV6eZDPa/bWo0671IUu3X0m3 XIR3TxcculNhN3pd4DHjS7KL7tM8G3YWRmURA4LG43nJsj/84/En3IvJRGamYai7QoJh 2EpQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=gT3MUMtIZ0PS90yl0lE9QBzqwoRSMBq2QOt4vNKzTys=; b=RHfGN4yn70nzsefbNxbdBH47cf4qYDZqTbj0vvJrJuWHtAa32J+2gIPnBSB3Di9Ycl dOfBZxg/w5bm8L1hDXOG+DB9qLTz3AlML9imiG2xR/KllRO9T+dhB4DH/r0RVACTrSjz 5PHormYObY8/kTUyP326IBC0+e3gtYLlEazFtEbn26uyLJnrHJA1Oq+Iya5Kp3tXYDoe d9EOjX464wzoYBFRlOxhfydQIo0MABdlnQGH3R7bGPYI1fvlNhyWnAIkz5jDfDTtaNpG vC4yOnobQ8i1iHipa89P3HZ+VyC/hQeBcN31BVQqoJKG0QOOk0e2BOIxnbllKxpz949Z Fjqg== X-Gm-Message-State: AOAM5338IVJfvbn/8wy2C1/HK6lkN0FnLm14hkJikIFhvOw+avYh+560 IIxzZ8DdrNAkyXRZT4KxGjOHVQ== X-Google-Smtp-Source: ABdhPJxdGShKmFpj2IYQOxd+oGtoXe5J3Pj9Ip1XNoCcHBZfb0/9N7iKTadK2twJpq0wRG+MfXG5fA== X-Received: by 2002:a17:902:6acb:b0:142:76c3:d35f with SMTP id i11-20020a1709026acb00b0014276c3d35fmr3494468plt.89.1636420663931; Mon, 08 Nov 2021 17:17:43 -0800 (PST) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id q13sm18256433pfj.26.2021.11.08.17.17.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 08 Nov 2021 17:17:43 -0800 (PST) Date: Tue, 9 Nov 2021 01:17:39 +0000 From: Sean Christopherson To: "Maciej S. Szmigiero" Subject: Re: [PATCH v5.5 01/30] KVM: Ensure local memslot copies operate on up-to-date arch-specific data Message-ID: References: <20211104002531.1176691-1-seanjc@google.com> <20211104002531.1176691-2-seanjc@google.com> <6407c2d3-854b-edf6-9990-b54a5baedd0a@oracle.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <6407c2d3-854b-edf6-9990-b54a5baedd0a@oracle.com> Cc: Anup Patel , Wanpeng Li , kvm@vger.kernel.org, David Hildenbrand , linux-kernel@vger.kernel.org, Paul Mackerras , Atish Patra , Ben Gardon , linux-riscv@lists.infradead.org, Claudio Imbrenda , kvmarm@lists.cs.columbia.edu, Janosch Frank , Marc Zyngier , Joerg Roedel , Huacai Chen , Christian Borntraeger , Aleksandar Markovic , Palmer Dabbelt , Albert Ou , kvm-ppc@vger.kernel.org, Paul Walmsley , linux-arm-kernel@lists.infradead.org, Jim Mattson , Cornelia Huck , linux-mips@vger.kernel.org, kvm-riscv@lists.infradead.org, Paolo Bonzini , Vitaly Kuznetsov X-BeenThere: kvmarm@lists.cs.columbia.edu X-Mailman-Version: 2.1.14 Precedence: list List-Id: Where KVM/ARM decisions are made List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu On Tue, Nov 09, 2021, Maciej S. Szmigiero wrote: > On 04.11.2021 01:25, Sean Christopherson wrote: > > @@ -1597,6 +1596,26 @@ static int kvm_set_memslot(struct kvm *kvm, > > kvm_copy_memslots(slots, __kvm_memslots(kvm, as_id)); > > } > > + /* > > + * Make a full copy of the old memslot, the pointer will become stale > > + * when the memslots are re-sorted by update_memslots(), and the old > > + * memslot needs to be referenced after calling update_memslots(), e.g. > > + * to free its resources and for arch specific behavior. This needs to > > + * happen *after* (re)acquiring slots_arch_lock. > > + */ > > + slot = id_to_memslot(slots, new->id); > > + if (slot) { > > + old = *slot; > > + } else { > > + WARN_ON_ONCE(change != KVM_MR_CREATE); > > + memset(&old, 0, sizeof(old)); > > + old.id = new->id; > > + old.as_id = as_id; > > + } > > + > > + /* Copy the arch-specific data, again after (re)acquiring slots_arch_lock. */ > > + memcpy(&new->arch, &old.arch, sizeof(old.arch)); > > Had "new" been zero-initialized completely in __kvm_set_memory_region() > for safety (so it does not contain stack garbage - I don't mean just the > new.arch field in the "if (!old.npages)" branch in that function but the > whole struct) this line would be needed only in the "if (slot)" branch > above (as Ben said). > > Also, when patch 7 from this series removes this memcpy(), > kvm_arch_prepare_memory_region() does indeed receive this field > uninitialized - I know only x86 and ppcHV care > and kvm_alloc_memslot_metadata() or kvmppc_core_prepare_memory_region_hv() > then overwrites it unconditionally but it feels a bit wrong. > > I am almost certain that compiler would figure out to only actually > zero the fields that wouldn't be overwritten immediately anyway. > > But on the other hand, this patch is only a fix for code that's going > to be replaced anyway so perfection here probably isn't that important. Yeah, that about sums up my feelings about the existing code. That said, an individual memslot isn't _that_ big, and memslot updates without the scalable implementation are dreadfully slow anyways, so I'm leaning strongly toward your suggestion of zeroing all of new as part of this fix. _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm 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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3C069C433EF for ; Tue, 9 Nov 2021 01:38:39 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 206DB61104 for ; Tue, 9 Nov 2021 01:38:39 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239142AbhKIBlU (ORCPT ); Mon, 8 Nov 2021 20:41:20 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50616 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237858AbhKIBlO (ORCPT ); Mon, 8 Nov 2021 20:41:14 -0500 Received: from mail-pl1-x635.google.com (mail-pl1-x635.google.com [IPv6:2607:f8b0:4864:20::635]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A1D6AC04CAC2 for ; Mon, 8 Nov 2021 17:17:44 -0800 (PST) Received: by mail-pl1-x635.google.com with SMTP id p18so17796917plf.13 for ; Mon, 08 Nov 2021 17:17:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=gT3MUMtIZ0PS90yl0lE9QBzqwoRSMBq2QOt4vNKzTys=; b=b13sxWM9K5iisnYPadl4iOffaoys9RijOi3mTsZncT79yh0EYcQ5zP36xW3B53gbfI hf38uXQxIRmk/GO8JXWSnV+gRSRIGZYtVYUdIsHAt6HO9GbiW9mBH/HQFPbdGVhmJzAE d+5xCaoOiMJhRE25WFHguzeVSIifxnExemfwDQtGBdPF4S9WTr8CvKXghy1m1SqKVnwm EERujOqfJ1YA7bmWxiB6FDxmsEbHoK9/PEVJSGjKx5CzfV6eZDPa/bWo0671IUu3X0m3 XIR3TxcculNhN3pd4DHjS7KL7tM8G3YWRmURA4LG43nJsj/84/En3IvJRGamYai7QoJh 2EpQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=gT3MUMtIZ0PS90yl0lE9QBzqwoRSMBq2QOt4vNKzTys=; b=FR2VYe/kicqXO5Qb0OdVxSNjy42NbhWfZNyDC2MdXbY6WvbbHlLUuRLUmWBfu2RqSG JAVphWePvRC9eja6tKE2Llan+H+v5sn4+Hh+7vWXi0/h6WnUTWcavC0esEmZl/C2TQhx tPuDVacisPy/vqcrttWlVHm+CI7d10zU9M1W/zO/5pZwNVuBtu3nBlg2wJVvjAxxNNcM vJ3016kDUsuf6rs8sx2jksbfD1SGNqqr1oqaw/nI1C1q73ZDyyi+QO6xXXLHiLP/+Vz7 AhUfcclPWpxN7cAt/dZzdggtqa62atf5UavECKT+40Fy2S0OiVUDsPwDRafTHl7xvKDJ jciQ== X-Gm-Message-State: AOAM5325d6XYp97M90yT8wzGH4eZi8q9+UrR+LJb8LBJ4ckNws7z8HVK SHzybC7IdDNddc+19sx0oUEpHA== X-Google-Smtp-Source: ABdhPJxdGShKmFpj2IYQOxd+oGtoXe5J3Pj9Ip1XNoCcHBZfb0/9N7iKTadK2twJpq0wRG+MfXG5fA== X-Received: by 2002:a17:902:6acb:b0:142:76c3:d35f with SMTP id i11-20020a1709026acb00b0014276c3d35fmr3494468plt.89.1636420663931; Mon, 08 Nov 2021 17:17:43 -0800 (PST) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id q13sm18256433pfj.26.2021.11.08.17.17.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 08 Nov 2021 17:17:43 -0800 (PST) Date: Tue, 9 Nov 2021 01:17:39 +0000 From: Sean Christopherson To: "Maciej S. Szmigiero" Cc: James Morse , Alexandru Elisei , Suzuki K Poulose , Atish Patra , David Hildenbrand , Cornelia Huck , Claudio Imbrenda , Vitaly Kuznetsov , Wanpeng Li , Jim Mattson , Joerg Roedel , linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, linux-mips@vger.kernel.org, kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, kvm-riscv@lists.infradead.org, linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, Ben Gardon , Marc Zyngier , Huacai Chen , Aleksandar Markovic , Paul Mackerras , Anup Patel , Palmer Dabbelt , Paul Walmsley , Albert Ou , Christian Borntraeger , Janosch Frank , Paolo Bonzini Subject: Re: [PATCH v5.5 01/30] KVM: Ensure local memslot copies operate on up-to-date arch-specific data Message-ID: References: <20211104002531.1176691-1-seanjc@google.com> <20211104002531.1176691-2-seanjc@google.com> <6407c2d3-854b-edf6-9990-b54a5baedd0a@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6407c2d3-854b-edf6-9990-b54a5baedd0a@oracle.com> Precedence: bulk List-ID: X-Mailing-List: linux-mips@vger.kernel.org On Tue, Nov 09, 2021, Maciej S. Szmigiero wrote: > On 04.11.2021 01:25, Sean Christopherson wrote: > > @@ -1597,6 +1596,26 @@ static int kvm_set_memslot(struct kvm *kvm, > > kvm_copy_memslots(slots, __kvm_memslots(kvm, as_id)); > > } > > + /* > > + * Make a full copy of the old memslot, the pointer will become stale > > + * when the memslots are re-sorted by update_memslots(), and the old > > + * memslot needs to be referenced after calling update_memslots(), e.g. > > + * to free its resources and for arch specific behavior. This needs to > > + * happen *after* (re)acquiring slots_arch_lock. > > + */ > > + slot = id_to_memslot(slots, new->id); > > + if (slot) { > > + old = *slot; > > + } else { > > + WARN_ON_ONCE(change != KVM_MR_CREATE); > > + memset(&old, 0, sizeof(old)); > > + old.id = new->id; > > + old.as_id = as_id; > > + } > > + > > + /* Copy the arch-specific data, again after (re)acquiring slots_arch_lock. */ > > + memcpy(&new->arch, &old.arch, sizeof(old.arch)); > > Had "new" been zero-initialized completely in __kvm_set_memory_region() > for safety (so it does not contain stack garbage - I don't mean just the > new.arch field in the "if (!old.npages)" branch in that function but the > whole struct) this line would be needed only in the "if (slot)" branch > above (as Ben said). > > Also, when patch 7 from this series removes this memcpy(), > kvm_arch_prepare_memory_region() does indeed receive this field > uninitialized - I know only x86 and ppcHV care > and kvm_alloc_memslot_metadata() or kvmppc_core_prepare_memory_region_hv() > then overwrites it unconditionally but it feels a bit wrong. > > I am almost certain that compiler would figure out to only actually > zero the fields that wouldn't be overwritten immediately anyway. > > But on the other hand, this patch is only a fix for code that's going > to be replaced anyway so perfection here probably isn't that important. Yeah, that about sums up my feelings about the existing code. That said, an individual memslot isn't _that_ big, and memslot updates without the scalable implementation are dreadfully slow anyways, so I'm leaning strongly toward your suggestion of zeroing all of new as part of this fix. 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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5D9C7C433F5 for ; Tue, 9 Nov 2021 01:42:45 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 2C32A610F7 for ; Tue, 9 Nov 2021 01:42:45 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 2C32A610F7 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=GTH4s9tiphSJi47KhfbXSr1TsKeW8czrO+GSaGRUNaA=; b=TT0U4HCSSAjeD/ wuUxOe+HKYVZ06piEwDXjFlf4eSoPwvxEl7mTilrM8cNS4Ee/CQzfB/TVSHzi5hTexnIViGl5RYBK PE6eMWxVIsSPJANuUQdTiUE6NR/0Z2ye117Rkf2vEpbQC2ATEvlUsiFL+rwK1XHa28niRqeVggOXx zMYH+WUy1+1p9iZMRtnYJVzFfQFQY/1lIY42JWBMLbzhZIuIH3OoTIfhpUveX96NhmFwMi2aTy127 BS5d7YLnPLBPWD/u3c7p4v9LjoUsLlMnUehs66IvMkabSnY8JN82WCUrLiFd6OafWz2zqqryjNz6F GfPjgDW7YvKPa5HhiKhQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mkG9k-000R3w-7Z; Tue, 09 Nov 2021 01:42:36 +0000 Received: from mail-pl1-x62c.google.com ([2607:f8b0:4864:20::62c]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mkFlh-000HHW-EE for linux-riscv@lists.infradead.org; Tue, 09 Nov 2021 01:17:48 +0000 Received: by mail-pl1-x62c.google.com with SMTP id u17so17838665plg.9 for ; Mon, 08 Nov 2021 17:17:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=gT3MUMtIZ0PS90yl0lE9QBzqwoRSMBq2QOt4vNKzTys=; b=b13sxWM9K5iisnYPadl4iOffaoys9RijOi3mTsZncT79yh0EYcQ5zP36xW3B53gbfI hf38uXQxIRmk/GO8JXWSnV+gRSRIGZYtVYUdIsHAt6HO9GbiW9mBH/HQFPbdGVhmJzAE d+5xCaoOiMJhRE25WFHguzeVSIifxnExemfwDQtGBdPF4S9WTr8CvKXghy1m1SqKVnwm EERujOqfJ1YA7bmWxiB6FDxmsEbHoK9/PEVJSGjKx5CzfV6eZDPa/bWo0671IUu3X0m3 XIR3TxcculNhN3pd4DHjS7KL7tM8G3YWRmURA4LG43nJsj/84/En3IvJRGamYai7QoJh 2EpQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=gT3MUMtIZ0PS90yl0lE9QBzqwoRSMBq2QOt4vNKzTys=; b=PSU+fsFDcixkMWERz+CfDH1VViebzbnqT2urGfmf/4CxEKnrjcc8dsY+u7nqDhHQNH HVJMXPbitinD+j7KkPChiirCZNDnZdmzDVf3hhprwuQJhGPdWJkxQFiWlieQtZTb2zM/ l6aiHIDuXg1TEEHvQiKOS7206LvOJVUZ99no2+Xbbuv1X2VKu62/4Nu6/uvyfJagbG6p dZOGTe4UfeUzcuw+v+kzjFVWJlynAlQc+0fL8fR/XnnzegbWOAAi4LIXMGcKbfh5epRQ 8eMkQxV3Ez68lrO6KqWBxziNKg78HHObtvdTYaiumBWcBTlTvyUW/ui/XqWQN6CYBdJp oJBA== X-Gm-Message-State: AOAM532pmWT7iD/ZA13Rr5StbcQPUxK/Dk12fhv/UaFyeXXXmi0n0/TT NnV7EFckW5arccmxwQADU42i7A== X-Google-Smtp-Source: ABdhPJxdGShKmFpj2IYQOxd+oGtoXe5J3Pj9Ip1XNoCcHBZfb0/9N7iKTadK2twJpq0wRG+MfXG5fA== X-Received: by 2002:a17:902:6acb:b0:142:76c3:d35f with SMTP id i11-20020a1709026acb00b0014276c3d35fmr3494468plt.89.1636420663931; Mon, 08 Nov 2021 17:17:43 -0800 (PST) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id q13sm18256433pfj.26.2021.11.08.17.17.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 08 Nov 2021 17:17:43 -0800 (PST) Date: Tue, 9 Nov 2021 01:17:39 +0000 From: Sean Christopherson To: "Maciej S. Szmigiero" Cc: James Morse , Alexandru Elisei , Suzuki K Poulose , Atish Patra , David Hildenbrand , Cornelia Huck , Claudio Imbrenda , Vitaly Kuznetsov , Wanpeng Li , Jim Mattson , Joerg Roedel , linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, linux-mips@vger.kernel.org, kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, kvm-riscv@lists.infradead.org, linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, Ben Gardon , Marc Zyngier , Huacai Chen , Aleksandar Markovic , Paul Mackerras , Anup Patel , Palmer Dabbelt , Paul Walmsley , Albert Ou , Christian Borntraeger , Janosch Frank , Paolo Bonzini Subject: Re: [PATCH v5.5 01/30] KVM: Ensure local memslot copies operate on up-to-date arch-specific data Message-ID: References: <20211104002531.1176691-1-seanjc@google.com> <20211104002531.1176691-2-seanjc@google.com> <6407c2d3-854b-edf6-9990-b54a5baedd0a@oracle.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <6407c2d3-854b-edf6-9990-b54a5baedd0a@oracle.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211108_171745_595265_22A9D42B X-CRM114-Status: GOOD ( 23.30 ) X-BeenThere: linux-riscv@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-riscv" Errors-To: linux-riscv-bounces+linux-riscv=archiver.kernel.org@lists.infradead.org On Tue, Nov 09, 2021, Maciej S. Szmigiero wrote: > On 04.11.2021 01:25, Sean Christopherson wrote: > > @@ -1597,6 +1596,26 @@ static int kvm_set_memslot(struct kvm *kvm, > > kvm_copy_memslots(slots, __kvm_memslots(kvm, as_id)); > > } > > + /* > > + * Make a full copy of the old memslot, the pointer will become stale > > + * when the memslots are re-sorted by update_memslots(), and the old > > + * memslot needs to be referenced after calling update_memslots(), e.g. > > + * to free its resources and for arch specific behavior. This needs to > > + * happen *after* (re)acquiring slots_arch_lock. > > + */ > > + slot = id_to_memslot(slots, new->id); > > + if (slot) { > > + old = *slot; > > + } else { > > + WARN_ON_ONCE(change != KVM_MR_CREATE); > > + memset(&old, 0, sizeof(old)); > > + old.id = new->id; > > + old.as_id = as_id; > > + } > > + > > + /* Copy the arch-specific data, again after (re)acquiring slots_arch_lock. */ > > + memcpy(&new->arch, &old.arch, sizeof(old.arch)); > > Had "new" been zero-initialized completely in __kvm_set_memory_region() > for safety (so it does not contain stack garbage - I don't mean just the > new.arch field in the "if (!old.npages)" branch in that function but the > whole struct) this line would be needed only in the "if (slot)" branch > above (as Ben said). > > Also, when patch 7 from this series removes this memcpy(), > kvm_arch_prepare_memory_region() does indeed receive this field > uninitialized - I know only x86 and ppcHV care > and kvm_alloc_memslot_metadata() or kvmppc_core_prepare_memory_region_hv() > then overwrites it unconditionally but it feels a bit wrong. > > I am almost certain that compiler would figure out to only actually > zero the fields that wouldn't be overwritten immediately anyway. > > But on the other hand, this patch is only a fix for code that's going > to be replaced anyway so perfection here probably isn't that important. Yeah, that about sums up my feelings about the existing code. That said, an individual memslot isn't _that_ big, and memslot updates without the scalable implementation are dreadfully slow anyways, so I'm leaning strongly toward your suggestion of zeroing all of new as part of this fix. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv 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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id EE0BCC433F5 for ; Tue, 9 Nov 2021 01:43:16 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id B2FDF610F8 for ; Tue, 9 Nov 2021 01:43:16 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org B2FDF610F8 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=TwALPYBNM8/tZTZousizrJACkYXBb0UxTrFforqtrtI=; b=pg7V1nBkZYaIAz mb/WNm1LaVbr5q73bGQUEEKYLZFBWLH8Od3JuFXGw6MxKKu238Ma3PKD4RwbFj3Y3U6NBIcMlImXT Mfs2fy3zQcOZuiulD0rcF69HktHT1KcVt+0+lGJVqw8bFX1Vm4KgmgGXlxyWqAjxtJF5g95neHfig J0tkSpj2da5bW6/QmZc2FquvgFU6r0a8rNMssGQLO3Op6IHpT9izYfRsQuFsPnZcuYPZXes239SPZ tkIDIuARbOmoymi6qtREdt02JfcnTxD63OnfzkKv4F/TcaViEMTNYAib6tbsNb10n5mqK8hfAzWNN OAROSSXALimVL762cUuw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mkG8S-000QWL-KO; Tue, 09 Nov 2021 01:41:18 +0000 Received: from mail-pl1-x632.google.com ([2607:f8b0:4864:20::632]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mkFlh-000HHU-ED for linux-arm-kernel@lists.infradead.org; Tue, 09 Nov 2021 01:17:47 +0000 Received: by mail-pl1-x632.google.com with SMTP id n8so17870063plf.4 for ; Mon, 08 Nov 2021 17:17:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=gT3MUMtIZ0PS90yl0lE9QBzqwoRSMBq2QOt4vNKzTys=; b=b13sxWM9K5iisnYPadl4iOffaoys9RijOi3mTsZncT79yh0EYcQ5zP36xW3B53gbfI hf38uXQxIRmk/GO8JXWSnV+gRSRIGZYtVYUdIsHAt6HO9GbiW9mBH/HQFPbdGVhmJzAE d+5xCaoOiMJhRE25WFHguzeVSIifxnExemfwDQtGBdPF4S9WTr8CvKXghy1m1SqKVnwm EERujOqfJ1YA7bmWxiB6FDxmsEbHoK9/PEVJSGjKx5CzfV6eZDPa/bWo0671IUu3X0m3 XIR3TxcculNhN3pd4DHjS7KL7tM8G3YWRmURA4LG43nJsj/84/En3IvJRGamYai7QoJh 2EpQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=gT3MUMtIZ0PS90yl0lE9QBzqwoRSMBq2QOt4vNKzTys=; b=XhhL+l/UF6eYDFLp5EtWhC1lHYrHwLQmOLwf22FnO66x4wLpIug1+TmVLaFb04ep0o CZehLu56yLEW+SjkX9czERhsrC4A6230OQPEsk3h0bCim+FcTXAUyzlniHHixJYKCDrR jgA1GkzrzuNOE6RM7W87RMGjseD2q+lGzw18uYgpwdCDfdbC7i8FaMQPGBqaM3nvS4UA iSK4HzfizmQXhtE7ePJGUx+b13bEtoQRvEMbOrwcsy02M8yIMEIsUCq3Dy2GPLfZIApj 89ZA3lcAFh2ZWDCIGMmHFAbJXubla64Am80+eH1QnmJ7Ab/ngfkoaZPtapd7ozkJMXhz KpbQ== X-Gm-Message-State: AOAM531JfZ2SKij5ubxN7tcsdkj6IRYBa/v6weXZYKMJLsOCL1wbnh0i HWrXANumbgVyWIjcRVxsrZNrWg== X-Google-Smtp-Source: ABdhPJxdGShKmFpj2IYQOxd+oGtoXe5J3Pj9Ip1XNoCcHBZfb0/9N7iKTadK2twJpq0wRG+MfXG5fA== X-Received: by 2002:a17:902:6acb:b0:142:76c3:d35f with SMTP id i11-20020a1709026acb00b0014276c3d35fmr3494468plt.89.1636420663931; Mon, 08 Nov 2021 17:17:43 -0800 (PST) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id q13sm18256433pfj.26.2021.11.08.17.17.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 08 Nov 2021 17:17:43 -0800 (PST) Date: Tue, 9 Nov 2021 01:17:39 +0000 From: Sean Christopherson To: "Maciej S. Szmigiero" Cc: James Morse , Alexandru Elisei , Suzuki K Poulose , Atish Patra , David Hildenbrand , Cornelia Huck , Claudio Imbrenda , Vitaly Kuznetsov , Wanpeng Li , Jim Mattson , Joerg Roedel , linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, linux-mips@vger.kernel.org, kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, kvm-riscv@lists.infradead.org, linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, Ben Gardon , Marc Zyngier , Huacai Chen , Aleksandar Markovic , Paul Mackerras , Anup Patel , Palmer Dabbelt , Paul Walmsley , Albert Ou , Christian Borntraeger , Janosch Frank , Paolo Bonzini Subject: Re: [PATCH v5.5 01/30] KVM: Ensure local memslot copies operate on up-to-date arch-specific data Message-ID: References: <20211104002531.1176691-1-seanjc@google.com> <20211104002531.1176691-2-seanjc@google.com> <6407c2d3-854b-edf6-9990-b54a5baedd0a@oracle.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <6407c2d3-854b-edf6-9990-b54a5baedd0a@oracle.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211108_171745_571729_FB756C67 X-CRM114-Status: GOOD ( 24.82 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Nov 09, 2021, Maciej S. Szmigiero wrote: > On 04.11.2021 01:25, Sean Christopherson wrote: > > @@ -1597,6 +1596,26 @@ static int kvm_set_memslot(struct kvm *kvm, > > kvm_copy_memslots(slots, __kvm_memslots(kvm, as_id)); > > } > > + /* > > + * Make a full copy of the old memslot, the pointer will become stale > > + * when the memslots are re-sorted by update_memslots(), and the old > > + * memslot needs to be referenced after calling update_memslots(), e.g. > > + * to free its resources and for arch specific behavior. This needs to > > + * happen *after* (re)acquiring slots_arch_lock. > > + */ > > + slot = id_to_memslot(slots, new->id); > > + if (slot) { > > + old = *slot; > > + } else { > > + WARN_ON_ONCE(change != KVM_MR_CREATE); > > + memset(&old, 0, sizeof(old)); > > + old.id = new->id; > > + old.as_id = as_id; > > + } > > + > > + /* Copy the arch-specific data, again after (re)acquiring slots_arch_lock. */ > > + memcpy(&new->arch, &old.arch, sizeof(old.arch)); > > Had "new" been zero-initialized completely in __kvm_set_memory_region() > for safety (so it does not contain stack garbage - I don't mean just the > new.arch field in the "if (!old.npages)" branch in that function but the > whole struct) this line would be needed only in the "if (slot)" branch > above (as Ben said). > > Also, when patch 7 from this series removes this memcpy(), > kvm_arch_prepare_memory_region() does indeed receive this field > uninitialized - I know only x86 and ppcHV care > and kvm_alloc_memslot_metadata() or kvmppc_core_prepare_memory_region_hv() > then overwrites it unconditionally but it feels a bit wrong. > > I am almost certain that compiler would figure out to only actually > zero the fields that wouldn't be overwritten immediately anyway. > > But on the other hand, this patch is only a fix for code that's going > to be replaced anyway so perfection here probably isn't that important. Yeah, that about sums up my feelings about the existing code. That said, an individual memslot isn't _that_ big, and memslot updates without the scalable implementation are dreadfully slow anyways, so I'm leaning strongly toward your suggestion of zeroing all of new as part of this fix. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel