linux-kernel - Re: [PATCH v3 06/16] x86/virt/tdx: Improve PAMT refcounters allocation for sparse memory

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: <894408f8987034fcbe945f7c46b68a840d333527.camel@intel.com>
Date: Wed, 1 Oct 2025 10:40:55 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>,
	"binbin.wu@...ux.intel.com" <binbin.wu@...ux.intel.com>
CC: "kvm@...r.kernel.org" <kvm@...r.kernel.org>, "linux-coco@...ts.linux.dev"
	<linux-coco@...ts.linux.dev>, "Zhao, Yan Y" <yan.y.zhao@...el.com>,
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>, "kas@...nel.org"
	<kas@...nel.org>, "seanjc@...gle.com" <seanjc@...gle.com>, "mingo@...hat.com"
	<mingo@...hat.com>, "kirill.shutemov@...ux.intel.com"
	<kirill.shutemov@...ux.intel.com>, "tglx@...utronix.de" <tglx@...utronix.de>,
	"Yamahata, Isaku" <isaku.yamahata@...el.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "pbonzini@...hat.com" <pbonzini@...hat.com>,
	"Annapurve, Vishal" <vannapurve@...gle.com>, "Gao, Chao"
	<chao.gao@...el.com>, "bp@...en8.de" <bp@...en8.de>, "x86@...nel.org"
	<x86@...nel.org>
Subject: Re: [PATCH v3 06/16] x86/virt/tdx: Improve PAMT refcounters
 allocation for sparse memory

On Wed, 2025-10-01 at 00:32 +0000, Edgecombe, Rick P wrote:
> On Wed, 2025-09-24 at 16:57 +0800, Binbin Wu wrote:
> > > This will results in @start pointing to the first refcount and @end
> > > pointing to the second, IIUC.
> > > 
> > > So it seems we need:
> > > 
> > >       start = (unsigned long)tdx_find_pamt_refcount(PFN_PHYS(start_pfn));
> > >       end   = (unsigned long)tdx_find_pamt_refcount(PFN_PHYS(end_pfn) - 1));
> > >       start = round_down(start, PAGE_SIZE);
> > >       end   = round_up(end, PAGE_SIZE);
> > 
> > Checked again, this seems to be the right version.
> 
> Thanks both for the analysis. I lazily created a test program to check some edge
> cases and found the original and this version were both buggy. Clearly this code
> needs to be clearer (as actually Dave pointed out in v2 and I failed to
> address). Example (synthetic failure):
> 
> start_pfn = 0x80000
> end_pfn =   0x80001
> 
> Original code: start = 0xff76ba4f9e034000
>                end   = 0xff76ba4f9e034000
> 
> Above fix:     start = 0xff76ba4f9e034000
>                end   = 0xff76ba4f9e034000

Oh I think the problem of the "Above fix" is it fails when @start and @end
are the same and both are already page aligned.

> 
> Part of the problem is that tdx_find_pamt_refcount() expects the hpa passed in
> to be PMD aligned. The other callers of tdx_find_pamt_refcount() also make sure
> that the PA passed in is 2MB aligned before calling, and compute this starting
> with a PFN. So to try to make it easier to read and be correct what do you think
> about the below:
> 
> static atomic_t *tdx_find_pamt_refcount(unsigned long pfn) {
>     unsigned long hpa = ALIGN_DOWN(pfn, PMD_SIZE);

Shouldn't it be:

	hpa = ALIGN_DOWN(PFN_PHYS(pfn), PMD_SIZE));
?
> 
>     return &pamt_refcounts[hpa / PMD_SIZE];
> }
> 
> /*
>  * 'start_pfn' is inclusive and 'end_pfn' is exclusive. 
> 

I think 'end_pfn' is exclusive is a little bit confusing?  It sounds like
the physical range from PFN_PHYS(end_pfn - 1) to PFN_PHYS(end_pfn) is also
exclusive, but it is actually not?  To me it's more like only the physical
address PFN_PHYS(end_pfn) is exclusive.

> Compute the 
>  * page range to be inclusive of the start and end refcount
>  * addresses and at least a page in size. The teardown logic needs
>  * to handle potentially overlapping refcounts mappings resulting
>  * from this.
>  */
> start = (unsigned long)tdx_find_pamt_refcount(start_pfn);
> end   = (unsigned long)tdx_find_pamt_refcount(end_pfn - 1);
> start = ALIGN_DOWN(start, PAGE_SIZE);
> end   = ALIGN_DOWN(end, PAGE_SIZE) + PAGE_SIZE;

This looks fine to me.  I mean the result should be correct, but the
'end_pfn - 1' (due to 'end_pfn' is exclusive) is a bit confusing to me as
said above, but maybe it's only me, so feel free to ignore.

Or, as said above, I think the problem of the "Above fix" is when
calculating the @end we didn't consider the case where it equals to @start
and is already page aligned.  Does below work (assuming
tdx_find_pamt_refcount() still takes 'hpa')?

    start = (unsigned long)tdx_find_pamt_refcount(PFN_PHYS(start_pfn));
    end   = (unsigned long)tdx_find_pamt_refcount(PFN_PHYS(end_pfn) - 1));
    start = PAGE_ALIGN_DOWN(start);
    end   = PAGE_ALIGN_DOWN(end) + PAGE_SIZE;

Anyway, don't have strong opinion here, so up to you.