linux-kernel - Re: [PATCH] x86/tdx: Support vmalloc() for tdx_enc_status

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240710081501.GAZo5DBW3nvdzp34AI@fat_crate.local>
Date: Wed, 10 Jul 2024 10:15:01 +0200
From: Borislav Petkov <bp@...en8.de>
To: Dexuan Cui <decui@...rosoft.com>
Cc: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
	"H. Peter Anvin" <hpa@...or.com>,
	"open list:X86 TRUST DOMAIN EXTENSIONS (TDX)" <linux-coco@...ts.linux.dev>,
	"open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <linux-kernel@...r.kernel.org>,
	Michael Kelley <mikelley@...rosoft.com>,
	Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>,
	Rick Edgecombe <rick.p.edgecombe@...el.com>,
	Kai Huang <kai.huang@...el.com>,
	"stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH] x86/tdx: Support vmalloc() for tdx_enc_status_changed()

On Wed, Jul 10, 2024 at 07:48:14AM +0000, Dexuan Cui wrote:
> It's ok to me it will be after -rc1. I just thought the patch would get more
> testing if it could be on some branch (e.g., x86/tdx ?) in the tip.git tree, e.g.,
> if the patch is on some tip.git branch, I suppose the linux-next tree would
> merge the patch so the patch will get more testing automatically. 

Yes, it will get more testing automatically but the period is important: if
I rush it now, it goes to Linus next week and then any fallout it causes needs
to be dealt with in mainline.

If I queue it after -rc1, it'll be only in tip and linux-next for an
additional 7 week cycle and I can always whack it if it breaks something. If
it doesn't, I can send it mainline in the 6.12 merge window.

But we won't have to revert it mainline.

See the difference?

> I guess we have different options on whether "the patch has changed
> substantially". My impression is that it hasn't.

If you're calling the difference between what I reverted and what you're
sending now unsubstantial:

--- /tmp/old	2024-07-10 10:03:20.016629439 +0200
+++ /tmp/new	2024-07-10 10:02:23.696872729 +0200
 diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
-index c1cb90369915..abf3cd591afd 100644
+index 078e2bac25531..8f471260924f7 100644
 --- a/arch/x86/coco/tdx/tdx.c
 +++ b/arch/x86/coco/tdx/tdx.c
-@@ -7,6 +7,7 @@
- #include <linux/cpufeature.h>
+@@ -8,6 +8,7 @@
  #include <linux/export.h>
  #include <linux/io.h>
+ #include <linux/kexec.h>
 +#include <linux/mm.h>
  #include <asm/coco.h>
  #include <asm/tdx.h>
  #include <asm/vmx.h>
-@@ -778,6 +779,19 @@ static bool tdx_map_gpa(phys_addr_t start, phys_addr_t end, bool enc)
+@@ -782,6 +783,19 @@ static bool tdx_map_gpa(phys_addr_t start, phys_addr_t end, bool enc)
  	return false;
  }
  
@@ -53,7 +86,7 @@ index c1cb90369915..abf3cd591afd 100644
  /*
   * Inform the VMM of the guest's intent for this physical page: shared with
   * the VMM or private to the guest.  The VMM is expected to change its mapping
-@@ -785,15 +799,22 @@ static bool tdx_map_gpa(phys_addr_t start, phys_addr_t end, bool enc)
+@@ -789,15 +803,30 @@ static bool tdx_map_gpa(phys_addr_t start, phys_addr_t end, bool enc)
   */
  static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
  {
@@ -63,23 +96,34 @@ index c1cb90369915..abf3cd591afd 100644
 +	unsigned long end = start + numpages * PAGE_SIZE;
 +	unsigned long step = end - start;
 +	unsigned long addr;
- 
--	if (!tdx_map_gpa(start, end, enc))
--		return false;
++
 +	/* Step through page-by-page for vmalloc() mappings */
 +	if (is_vmalloc_addr((void *)vaddr))
 +		step = PAGE_SIZE;
++
++	for (addr = start; addr < end; addr += step) {
++		phys_addr_t start_pa;
++		phys_addr_t end_pa;
++
++		/* The check fails on vmalloc() mappings */
++		if (virt_addr_valid(addr))
++			start_pa = __pa(addr);
++		else
++			start_pa = slow_virt_to_phys((void *)addr);
+ 
+-	if (!tdx_map_gpa(start, end, enc))
+-		return false;
++		end_pa = start_pa + step;
  
 -	/* shared->private conversion requires memory to be accepted before use */
 -	if (enc)
 -		return tdx_accept_memory(start, end);
-+	for (addr = start; addr < end; addr += step) {
-+		phys_addr_t start_pa = slow_virt_to_phys((void *)addr);
-+		phys_addr_t end_pa   = start_pa + step;
-+
 +		if (!tdx_enc_status_changed_phys(start_pa, end_pa, enc))
 +			return false;
 +	}
  
  	return true;
  }

especially for a patch which is already known to break things and where we're
especially careful, then yes, we strongly disagree here.

So yes, it will definitely not go in now.

> I started to add people's tags since v4 and my impression is that since then
> it's rebasing and minor changes.

When version N introduces changes like above in what is already non-trivial
code, you drop all tags. And if people want to review it again, then they
should give you those R-by tags.

Also, think about it: your patch broke a use case. How much are those R-by
tags worth if the patch is broken? And why do you want to hold on to them so
badly?

If a patch needs to be reverted because it breaks a use case, all reviewed and
acked tags should simply be removed too. It is that simple.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette