From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-dl1-f53.google.com (mail-dl1-f53.google.com [74.125.82.53]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CB6F4381AF for ; Fri, 26 Jun 2026 00:07:27 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=pass smtp.client-ip=74.125.82.53 ARC-Seal:i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1782432450; cv=pass; b=D/I5NM4wS1h2ggRW/GqfcenLJ30xu7eLvd3YAc4oLEgpEcA+psPkQZ0pbZyx4Z+JyKHPCqAASI3qgxHfhoxLPC7VLxHUu+kmF7oHW8+rsAG9ozK4lcJ8lnW1rbY0xt5NpGPsgzgZzK18yD2r5pJsoH6fCF4dZsYBeqwwqJFAxXM= ARC-Message-Signature:i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1782432450; c=relaxed/simple; bh=rLK+LmNxOJTRUAAZGq1OR3eo1LZknLxQo62bHG3MWVs=; h=From:In-Reply-To:References:MIME-Version:Date:Message-ID:Subject: To:Cc:Content-Type; b=WtO7USSHvcLpQxI++99/5i0pBFNKFyPA3i3oozUI3LcL70HfLLqYX1NWzPWgM/70SNNAsORN1TFTnEVTj1rb1ZeJ8jyTnWV6ei5su2TvkgUxqQsIaeYRuBxv1/NtdEKaDvk6mxgCS+nSl8FhWsRSKhESgXeRiCk4Ri2rGnCUhso= ARC-Authentication-Results:i=2; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=dDwk8Ij3; arc=pass smtp.client-ip=74.125.82.53 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="dDwk8Ij3" Received: by mail-dl1-f53.google.com with SMTP id a92af1059eb24-1384ebe7a10so1017951c88.1 for ; Thu, 25 Jun 2026 17:07:27 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1782432447; cv=none; d=google.com; s=arc-20260327; b=oavmCR6mWIIIcMLEGlziczefqy/EcmfoxrZmn7/kZaJzwPRzlWmCzImJLS2GC9LeIw o1+rmmHAMZKCKmH9P24eFXWvrAooD3u+9QPDGgh6Hx6MhnYFyS+1JrrZ1dQlz8Ci0kM/ Ec9+MPL8eZBblirzaE0PFfmidxQITErrAjJI6CMfdjGWFrEA76v6uvmr4sYLhaidp0Xk Oh0VZQgz0VStj5O6lQdmry/uF6CwlVnIqu5UeUSq+zY3pnaszdZUDebSZst94XUt9QQd ziuJs//m7fUJ0YSd5brWJMMMstitCPq3znddRDlEPGujOsYlG6AMe6waqVTG59Xv+OUM i+3w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20260327; h=cc:to:subject:message-id:date:mime-version:references:in-reply-to :from:dkim-signature; bh=NNd8RQ2oAjOine6lO9TBbPn3RS+xKH28gpFKike5xvo=; fh=PHbFBkrNQYQCWHTAndlUAHtFKQGQpYvolv5jB+LSHEQ=; b=Vm+6v+qPqdsmYN2v+UyMdd3IvikNeqPJ74ufF3pOtWRqFKqBvULFfOi97DtjDuqzNu TYbCJgLs2rYT97InpwC+fTHShgHAUkfCr2KwN0Ax3JjLf7Yfs0yJJ2rAzLHv0x/FcPhL P6BwuGxRn1dHJYTNHiguTj4uEGSvGiYyJHK7eNfAMeYK+v4RzUHH32qQOgmORMceKY2x OwFVht3XKkEav6f3QxrqiW3dbSPqBhjzwVu+6LJH7UcDFDVl6NclvxU2r1QHNEkf2JN/ h4y2t28X6AmBOUTcE38lr9kxMd82oj/n05cI4PWZfl7UEoZu6MwWlCmQ8v2fUtKeKLuE W60A==; darn=vger.kernel.org ARC-Authentication-Results: i=1; mx.google.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20251104; t=1782432447; x=1783037247; darn=vger.kernel.org; h=cc:to:subject:message-id:date:mime-version:references:in-reply-to :from:from:to:cc:subject:date:message-id:reply-to; bh=NNd8RQ2oAjOine6lO9TBbPn3RS+xKH28gpFKike5xvo=; b=dDwk8Ij3rh70kybF3MsWTtSugw4Ns5NZmGQjYBSj9T5hvDPpKx4NukI9dz6t/lwINO CEd0FvHyLyZOskfvDknvfAh3iLfGMmEVyJgsvY7lKa321pLmNDPAcuKrmIcYL//cF5sI BwgWeUZCd2t3cXiFF/KxcymJ5AGjJdP0hXnKvBBeTRwSmlVDia4dHI7dFoZahEuqjIvc b/XcvTK2bug2LOi4XL4EAo/3x2FJkhyra7jmXxxQ0v8SfGSdi2+cHiCgQjD+YvBNcfdM 348JMq+MFlYnQL7i1JWYh6tfAynIXF5zIX5ig3DNClUA14SFyYC/odiFKWE9IK2MEDl5 TEIQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1782432447; x=1783037247; h=cc:to:subject:message-id:date:mime-version:references:in-reply-to :from:x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=NNd8RQ2oAjOine6lO9TBbPn3RS+xKH28gpFKike5xvo=; b=l9xmsB8QZjHvCS1I2mQMBzKxkwXzinIaL8xTAqAKVXvCa+KKTnSx1sxr6XfDCYjqUJ LKEdtqaLtp1QEB+Ac77f7t/y6PfbJeM8MUp4fcQAAYja2wVDkW+ca4eVFC9NvwBurdhc oN4w5ZqRKIzD/ptPiccFQq+gUb1xwdTha67rAEpRmiFfbxrzluKt63F7oI1ejTaNpfrg OcosU3qv05nk0QRtGvdzQTmkzmVwtVtdGoUdgskimdCq02VA32mv7vC0PpOVk8XQBnNy TnLkhJEFW+pxgB1EaIJyCF8drkCQHhCBdN2IsDAwQ+7wthg6VyNX/men2p1V/gvuShTN ezHw== X-Forwarded-Encrypted: i=1; AFNElJ9038ON8kdCSVvO8qjE1NFNBtHfOqStuseDSlDq8w/Nhv6cJaUWR3PKSy3NpPOVMqpT/z6fONswyqVMN8w=@vger.kernel.org X-Gm-Message-State: AOJu0YxdHw0Y5n6rIXxZMpYjI7m87QEBnkc9KXTUF6lEz+eKSQJIZaTo 2z+y5nV8WcEmjYu6HqPQkPNzwHFgS9I2VT+T/LqShrUXQIJV8UT1dGEKLUeHfNhXByRFPzfp7Ex QvLtSEEb83bdOq4cyBbchaFgsgpB6pwNgWQypDB3j X-Gm-Gg: AfdE7ckyZi315xM8Q0OJhskqW5U/QvocqsWq4cy++stiHDYJxZafB/mvICx1thB5OnS fJnkPEUoy3/xB0/tp+zxKWNscaGHvR7tFFiurR/BrMa3JKc8lAl7dyubyZWFGCWh5gbRK3mCPkQ YHEnTh1kernUfZHzZw7S6QI8/WcT5YsMWCjsIsw+UL9T5K1I/ohs3FPL4/Sa5fTjiw9bPGy0obI pyKEEvN27P7rVq+dgPO+kII62cDxnqEueRtEeGFECusO8u65OFGWCrZdTOPB5b7vHYuPJj1xVig EYDNvsBlGaEr3lYeTd/6Ycgp8+eh6qV0Tos4uh6i7XFaSI8X7kGSf1NN4YE= X-Received: by 2002:a05:7022:43a4:b0:137:f49d:3388 with SMTP id a92af1059eb24-139dbb5b144mr4108872c88.21.1782432444996; Thu, 25 Jun 2026 17:07:24 -0700 (PDT) Received: from 176938342045 named unknown by gmailapi.google.com with HTTPREST; Thu, 25 Jun 2026 17:07:23 -0700 Received: from 176938342045 named unknown by gmailapi.google.com with HTTPREST; Thu, 25 Jun 2026 17:07:23 -0700 From: Ackerley Tng In-Reply-To: References: <20260618-gmem-inplace-conversion-v8-0-9d2959357853@google.com> <20260618-gmem-inplace-conversion-v8-23-9d2959357853@google.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Date: Thu, 25 Jun 2026 17:07:23 -0700 X-Gm-Features: AVVi8CeCYnzA1y3pJSgcOnrMDG9x7K-8H5mOt6-X5dtowJ5-kK2ahwrJyY06kZw Message-ID: Subject: Re: [PATCH v8 23/46] KVM: TDX: Make source page optional for KVM_TDX_INIT_MEM_REGION To: Yan Zhao Cc: Sean Christopherson , aik@amd.com, andrew.jones@linux.dev, binbin.wu@linux.intel.com, brauner@kernel.org, chao.p.peng@linux.intel.com, david@kernel.org, jmattson@google.com, jthoughton@google.com, michael.roth@amd.com, oupton@kernel.org, pankaj.gupta@amd.com, qperret@google.com, rick.p.edgecombe@intel.com, rientjes@google.com, shivankg@amd.com, steven.price@arm.com, tabba@google.com, willy@infradead.org, wyihan@google.com, forkloop@google.com, pratyush@kernel.org, suzuki.poulose@arm.com, aneesh.kumar@kernel.org, liam@infradead.org, Paolo Bonzini , Thomas Gleixner , Ingo Molnar , Borislav Petkov , Dave Hansen , x86@kernel.org, "H. Peter Anvin" , Steven Rostedt , Masami Hiramatsu , Mathieu Desnoyers , Jonathan Corbet , Shuah Khan , Shuah Khan , Vishal Annapurve , Andrew Morton , Chris Li , Kairui Song , Kemeng Shi , Nhat Pham , Barry Song , Axel Rasmussen , Yuanchu Xie , Wei Xu , Youngjun Park , Qi Zheng , Shakeel Butt , Kiryl Shutsemau , Baoquan He , Jason Gunthorpe , Vlastimil Babka , kvm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org, linux-doc@vger.kernel.org, linux-kselftest@vger.kernel.org, linux-mm@kvack.org, linux-coco@lists.linux.dev Content-Type: text/plain; charset="UTF-8" Yan Zhao writes: > On Wed, Jun 24, 2026 at 04:00:32PM -0700, Ackerley Tng wrote: >> Sean Christopherson writes: >> >> > On Tue, Jun 23, 2026, Yan Zhao wrote: >> >> On Tue, Jun 23, 2026 at 01:16:14PM +0800, Yan Zhao wrote: >> >> > On Mon, Jun 22, 2026 at 06:22:45PM -0700, Sean Christopherson wrote: >> >> > > On Mon, Jun 22, 2026, Yan Zhao wrote: >> >> > > > On Thu, Jun 18, 2026 at 05:32:00PM -0700, Ackerley Tng via B4 Relay wrote: >> >> > > > > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c >> >> > > > > index ffe9d0db58c59..56d10333c61a7 100644 >> >> > > > > --- a/arch/x86/kvm/vmx/tdx.c >> >> > > > > +++ b/arch/x86/kvm/vmx/tdx.c >> >> > > > > @@ -3198,8 +3198,12 @@ static int tdx_gmem_post_populate(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn, >> >> > > > > if (KVM_BUG_ON(kvm_tdx->page_add_src, kvm)) >> >> > > > > return -EIO; >> >> > > > > >> >> > > > > - if (!src_page) >> >> > > > > - return -EOPNOTSUPP; >> >> > > > > + if (!src_page) { >> >> > > > > + if (!gmem_in_place_conversion) >> >> > > > When userspace turns on gmem_in_place_conversion while creating guest_memfd >> >> > > > without the MMAP flag, the absence of src_page should still be treated as an >> >> > > > error. >> >> > > >> >> > > Why MMAP? >> >> > Hmm, I was showing a scenario that in-place conversion couldn't occur. >> >> > I didn't mean that with the MMAP flag, mmap() and user write must occur. >> >> > >> >> > > Shouldn't this be a general "if (!src_page && !up-to-date)"? Just >> >> > > because userspace _can_ mmap() the memory doesn't mean userspace _has_ mmap()'d >> >> > > and written memory. And when write() lands, MMAP wouldn't be necessary to >> >> > > initialize the memory. >> >> > Do you mean using up-to-date flag as below? >> > >> > Yes? I didn't actually look at the implementation details. >> > >> >> > if (!src_page) { >> >> > src_page = pfn_to_page(pfn); >> >> > if (!folio_test_uptodate(page_folio(src_page))) >> >> > return -EOPNOTSUPP; >> >> > } >> >> Yan is right that with the earlier patch "Zero page while getting pfn", >> folio_test_uptodate() here will always return true. >> >> Actually, this is an alternative fix for the issue Sashiko pointed out >> on v7 where userspace can do a populate() (either TDX or SNP) without >> first allocating the page, with src_address == NULL, and leak >> uninitialized memory into the guest. >> >> Advantage of using the uptodate check in populate: if the host never >> allocates the page, populate doesn't incur zeroing before writing the >> page anyway in populate(). >> >> Disadvantage: Both TDX and SNP will have to implement this uptodate >> check. guest_memfd can't check centrally because for SNP, for a >> PAGE_TYPE_ZERO, !src_page should be allowed with a !uptodate page since >> firmware will zero and there's no leakage of uninitialized host memory? > Another disadvantage: the uptodate flag is per-folio. What if the folio > is only partially initialized by the userspace especially after huge page is > supported? > Good point on huge pages! The uptodate flag on the folio in guest_memfd means "this folio has been written to". As of now (before patch at [1]), this happens when + folio is zeroed on first use by userspace + folio is zeroed on first use of the guest + folio is populated When huge pages are supported, the folio can't partially be initialized? On allocation, if any part is shared, we split the page. The parts are separate folios that have their own uptodate flags. On splitting, if the huge page is uptodate, the split pages will also be uptodate. If the huge page is not uptodate, the split pages won't be uptodate, but that's ok since they will be marked uptodate on first use. On merging, the non-uptodate parts have to be zeroed and then marked uptodate. Any parts that are in use would have been marked uptodate already, so there's no overwriting data that is in use. I'll need to think more about when it's safe to zero. I'm still on the fence between the two options 1. Using uptodate check in populate to reject src_pages that have never been written to or 2. Always zero before populate but whether the uptodate flag is per-folio or not doesn't affect these two options in terms of fixing the leak of uninitialized host memory, right? > >> >> Another concern with this fix is that: >> >> commit "KVM: guest_memfd: Zero page while getting pfn" [1] always marks the >> >> folio uptodate before reaching post_populate(). >> >> >> >> [1] https://lore.kernel.org/all/20260618-gmem-inplace-conversion-v8-21-9d2959357853@google.com/ >> >> >> >> > One concern is that TDX now does not much care about the up-to-date flag since >> >> > TDX doesn't rely on the flag to clear pages on conversions. >> >> > I'm not sure if the flag can be reliably checked in this case. e.g., >> >> > now the whole folio is marked up-to-date even if only part of it is faulted by >> >> > user access. >> >> > Ensuring that the up-to-date flag works correctly with huge page support seems >> >> > to have more effort than introducing a dedicated flag for TDX. >> >> > >> >> > > > Additionally, to properly enable in-place copying for the TDX initial memory >> >> > > > region, userspace must not only specify source_addr to NULL, but also follow >> >> > > > a specific sequence (where steps 1/2/3/7 are required only for in-place copy): >> >> > > > 1. create guest_memfd with MMAP flag >> >> > > > 2. mmap the guest_memfd. >> >> > > > 3. convert the initial memory range to shared. >> >> > > > 4. copy initial content to the source page. >> >> > > > 5. convert the initial memory range to private >> >> > > > 6. invoke ioctl KVM_TDX_INIT_MEM_REGION. >> >> > > > 7. do not unmap the source backend. >> >> > > > >> >> > > > So, would it be reasonable to introduce a dedicated flag that allows userspace >> >> > > > to explicitly opt into the in-place copy functionality? e.g., >> >> > > >> >> > > Why? It's userspace's responsibility to get the above right. If userspace fails >> >> > > to provide a src_page when it doesn't want in-place copy, that's a userspace bug. >> >> Yan, is your concern that userspace forgot to update the code and >> forgets to provide a src_page, and if we keep the "Zero page while > Yes. Previously, it would be rejected after GUP fails. > I see, didn't realize previously it would be rejected because GUP fails. GUP failed because it wasn't faulted into the host? That's kind of orthogonal, I don't think GUP fail leading to rejecting populate was meant to help userspace catch these issues. GUP would also fail if the user did mmap(), write to it, unmap using madvise(MADV_DONTNEED), then forget and pass 0 as src_address. >> getting pfn" patch, ends up with the guest silently having a zero page? >> I think that would be found quite early in userspace VMM testing... > I actually encountered this during testing this patch. > I update most code path to follow this sequence. However, still some corner ones > for TDVF HOB, which are less obvious and harder to update. > The TD just booted up and hang silently. > I think this is just the life of a close-to-hardware software engineer :P no errors, got stuck somewhere, root cause is some unitialized thing. >> >> > I mean if userspace specifies a NULL source_addr by mistake, it's better for >> >> > kernel to detect this mistake, similar to how it validates whether source_addr >> >> > is PAGE_ALIGNED. >> > >> > The alignment case is different. If userspace provides an unaligned value, KVM >> > *can't* do what userspace is asking because hardware and thus KVM only supports >> > converting on page boundaries. >> > >> > For a NULL source, KVM can still do what userspace is asking. Rejecting userspace's >> > request would then be making assumptions about what userspace wants. >> > >> >> Also, +1 on this, what if userspace, knowing that pages are zeroed on >> allocation, actually wants to rely on that to get a zero page in the guest? > What if 0 uaddr is a valid address? :) > >> >> > Since userspace already needs to perform additional steps to enable in-place >> >> > copy, specifying a dedicated flag to indicate that the NULL source_addr is >> >> > intentional seems like a reasonable burden. >> > >> > I don't see how it adds any value. I wouldn't be at all surprised if most VMMs >> > just wen up with code that does: >> > >> > if (in-place) { >> > src = NULL; >> > flags |= KVM_TDX_IN_PLACE_COPY_INITIAL_MEMORY_REGION; >> > } >>