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 2E927C001B0 for ; Fri, 14 Jul 2023 22:36:02 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229604AbjGNWgA (ORCPT ); Fri, 14 Jul 2023 18:36:00 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33302 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229437AbjGNWf6 (ORCPT ); Fri, 14 Jul 2023 18:35:58 -0400 Received: from mail-pl1-x649.google.com (mail-pl1-x649.google.com [IPv6:2607:f8b0:4864:20::649]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 91D432D73 for ; Fri, 14 Jul 2023 15:35:53 -0700 (PDT) Received: by mail-pl1-x649.google.com with SMTP id d9443c01a7336-1b896096287so17458785ad.0 for ; Fri, 14 Jul 2023 15:35:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1689374153; x=1691966153; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=AqPQXu/AIr+STPlGjrumZEqk7MLinV6fP6qlYwF+Hxk=; b=Tt0V9A0qkpbl5zS0LESlEdwvp+4lS8swsnaM5ygU4p8wmWduWJsTsWiMBUrW9Vh2KL aNiclCU+3JOXB1yiqIKXN6QgR9QdqoTo2RC11ivA+QpKq3HQpnI8PiDAYzg63A260iOT /X8Zs4ITCCFalugl0jsN26LMWffPpuzQBP22vn8lpRSG+H4mTHf3+orEXOoeYZYdfyr2 abFsO9FDujBoCuI48/+aMdXB2jBUEMCB+wK+csXPETYbqUkpzTNRqr4u7awtDvgcPp5C u2l7VKka+4qaqK2LwY7H9dgsRxhvzJ+xsqjkh1lzLWjpI9iGg9n1Apbc27ImlTO1X+KO zawA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689374153; x=1691966153; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=AqPQXu/AIr+STPlGjrumZEqk7MLinV6fP6qlYwF+Hxk=; b=NZeb9URwOmCYQqrxeEzdFPwpToZMpB10u+aDbJaJWq37thvw8ydwgXzvO6rBah87gK oy1LXfmU3B3Z4tE7CKCZY47lao29cwANMHjG9t23f/B+sQOa8wiNvv+0J8GmuqHlLows khyNij0MocIb9TxpjLgl4nJ8QFmyM/257lPA8jqYVYvuoS/32CSo2dYxA0YKeZVAYIzH WScZmvv1Prhc5Oz4/simhwhG2GQH2QppwQ5gcLOGVZHsnV5e4zp5a/voz4NqFU39kvVZ pRoyzpdgmOCNK5pgmfiLa3h5d4MaC47Fccisp3fEJVxhK0qFOPwvO31VjNWn2w0uJj8d V9fQ== X-Gm-Message-State: ABy/qLZ0wnm8F6BPqoETa0dPTSE9R4VfEVT573KZN+r1no5P8v1ZCYOC QcJNX4ROfplGudrtMwy7W9Vg5oK9VcU= X-Google-Smtp-Source: APBJJlF9Rovb1aladgpgZPHMCXoqG55aen/Zed897i+LVWocHvfN1RJ6IbmpkTUFw7J9N1WkMXgLw6ub6IY= X-Received: from zagreus.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:5c37]) (user=seanjc job=sendgmr) by 2002:a17:902:b492:b0:1b9:e338:a90d with SMTP id y18-20020a170902b49200b001b9e338a90dmr17230plr.3.1689374153026; Fri, 14 Jul 2023 15:35:53 -0700 (PDT) Date: Fri, 14 Jul 2023 22:35:51 +0000 In-Reply-To: <20230714115715.000026fe.zhi.wang.linux@gmail.com> Mime-Version: 1.0 References: <358fb191b3690a5cbc2c985d3ffc67224df11cf3.1687991811.git.isaku.yamahata@intel.com> <20230714115715.000026fe.zhi.wang.linux@gmail.com> Message-ID: Subject: Re: [RFC PATCH v3 08/11] KVM: Fix set_mem_attr ioctl when error case From: Sean Christopherson To: Zhi Wang Cc: isaku.yamahata@intel.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, isaku.yamahata@gmail.com, Paolo Bonzini , erdemaktas@google.com, Sagi Shahar , David Matlack , Kai Huang , chen.bo@intel.com, linux-coco@lists.linux.dev, Chao Peng , Ackerley Tng , Vishal Annapurve , Michael Roth , Yuan Yao Content-Type: text/plain; charset="us-ascii" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jul 14, 2023, Zhi Wang wrote: > On Thu, 13 Jul 2023 15:03:54 -0700 > Sean Christopherson wrote: > > + /* > > + * Reserve memory ahead of time to avoid having to deal with failures > > + * partway through setting the new attributes. > > + */ > > + for (i = start; i < end; i++) { > > + r = xa_reserve(&kvm->mem_attr_array, i, GFP_KERNEL_ACCOUNT); > > + if (r) > > + goto out_unlock; > > + } > > + > > KVM_MMU_LOCK(kvm); > > kvm_mmu_invalidate_begin(kvm); > > kvm_mmu_invalidate_range_add(kvm, start, end); > > KVM_MMU_UNLOCK(kvm); > > > > for (i = start; i < end; i++) { > > - if (xa_err(xa_store(&kvm->mem_attr_array, i, entry, > > - GFP_KERNEL_ACCOUNT))) > > + r = xa_err(xa_store(&kvm->mem_attr_array, i, entry, > > + GFP_KERNEL_ACCOUNT)); > > + if (KVM_BUG_ON(r, kvm)) > > break; > > } > > > > IIUC, If an error happenes here, we should bail out and call xa_release()? > Or the code below (which is not shown here) still changes the memory attrs > partially. I'm pretty sure we want to continue on. The VM is dead (killed by KVM_BUG_ON()), so the attributes as seen by userspace and/or the VM don't matter. What does matter is that KVM's internal state is consistent, e.g. that KVM doesn't have shared SPTEs while the attributes say a GFN is private. That might not matter for teardown, but I can't think of any reason not to tidy up. And there can also be other ioctls() in flight. KVM_REQ_VM_DEAD ensures vCPU can't enter the guest, and vm->vm_dead ensures no new ioctls() cant start, but neither of those guarantees there aren't other tasks doing KVM things. Regardless, we definitely don't need to do xa_release(). The VM is dead and all its memory will be reclaimed soon enough. And there's no guarantee xa_release() will actually be able to free anything, e.g. already processed entries won't be freed, nor will any entries that existed _before_ the ioctl() was invoked. Not to mention that the xarray probably isn't consuming much memory, relatively speaking. I.e. in the majority of scenarios, it's likely preferable to get out and destroy the VM asap.