[v10,16/50] x86/sev: Introduce snp leaked pages list

  From: Ashish Kalra <ashish.kalra@amd.com>

Pages are unsafe to be released back to the page-allocator, if they
have been transitioned to firmware/guest state and can't be reclaimed
or transitioned back to hypervisor/shared state. In this case add
them to an internal leaked pages list to ensure that they are not freed
or touched/accessed to cause fatal page faults.

Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
[mdr: relocate to arch/x86/coco/sev/host.c]
Signed-off-by: Michael Roth <michael.roth@amd.com>
---
 arch/x86/include/asm/sev-host.h |  3 +++
 arch/x86/virt/svm/sev.c         | 28 ++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)
  

On Mon, Oct 16, 2023 at 08:27:45AM -0500, Michael Roth wrote:
> +	spin_lock(&snp_leaked_pages_list_lock);
> +	while (npages--) {
> +		/*
> +		 * Reuse the page's buddy list for chaining into the leaked
> +		 * pages list. This page should not be on a free list currently
> +		 * and is also unsafe to be added to a free list.
> +		 */
> +		list_add_tail(&page->buddy_list, &snp_leaked_pages_list);
> +		sev_dump_rmpentry(pfn);
> +		pfn++;
> +	}
> +	spin_unlock(&snp_leaked_pages_list_lock);
> +	atomic_long_inc(&snp_nr_leaked_pages);

How is this supposed to count?

You're leaking @npages as the function's parameter but are incrementing
snp_nr_leaked_pages only once?

Just make it a bog-normal unsigned long and increment it inside the
locked section.

Or do at the beginning of the function:

	atomic_long_add(npages, &snp_nr_leaked_pages);
  

On 10/16/23 15:27, Michael Roth wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
> 
> Pages are unsafe to be released back to the page-allocator, if they
> have been transitioned to firmware/guest state and can't be reclaimed
> or transitioned back to hypervisor/shared state. In this case add
> them to an internal leaked pages list to ensure that they are not freed

Note the adding to the list doesn't ensure anything like that. Not dropping
the refcount to zero does. But tracking them might indeed not be bad for
e.g. crashdump investigations so no objection there.

> or touched/accessed to cause fatal page faults.
> 
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> [mdr: relocate to arch/x86/coco/sev/host.c]
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> ---
>  arch/x86/include/asm/sev-host.h |  3 +++
>  arch/x86/virt/svm/sev.c         | 28 ++++++++++++++++++++++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/arch/x86/include/asm/sev-host.h b/arch/x86/include/asm/sev-host.h
> index 1df989411334..7490a665e78f 100644
> --- a/arch/x86/include/asm/sev-host.h
> +++ b/arch/x86/include/asm/sev-host.h
> @@ -19,6 +19,8 @@ void sev_dump_hva_rmpentry(unsigned long address);
>  int psmash(u64 pfn);
>  int rmp_make_private(u64 pfn, u64 gpa, enum pg_level level, int asid, bool immutable);
>  int rmp_make_shared(u64 pfn, enum pg_level level);
> +void snp_leak_pages(u64 pfn, unsigned int npages);
> +
>  #else
>  static inline int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level) { return -ENXIO; }
>  static inline void sev_dump_hva_rmpentry(unsigned long address) {}
> @@ -29,6 +31,7 @@ static inline int rmp_make_private(u64 pfn, u64 gpa, enum pg_level level, int as
>  	return -ENXIO;
>  }
>  static inline int rmp_make_shared(u64 pfn, enum pg_level level) { return -ENXIO; }
> +static inline void snp_leak_pages(u64 pfn, unsigned int npages) {}
>  #endif
>  
>  #endif
> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index bf9b97046e05..29a69f4b8cfb 100644
> --- a/arch/x86/virt/svm/sev.c
> +++ b/arch/x86/virt/svm/sev.c
> @@ -59,6 +59,12 @@ struct rmpentry {
>  static struct rmpentry *rmptable_start __ro_after_init;
>  static u64 rmptable_max_pfn __ro_after_init;
>  
> +/* list of pages which are leaked and cannot be reclaimed */
> +static LIST_HEAD(snp_leaked_pages_list);
> +static DEFINE_SPINLOCK(snp_leaked_pages_list_lock);
> +
> +static atomic_long_t snp_nr_leaked_pages = ATOMIC_LONG_INIT(0);
> +
>  #undef pr_fmt
>  #define pr_fmt(fmt)	"SEV-SNP: " fmt
>  
> @@ -518,3 +524,25 @@ int rmp_make_shared(u64 pfn, enum pg_level level)
>  	return rmpupdate(pfn, &val);
>  }
>  EXPORT_SYMBOL_GPL(rmp_make_shared);
> +
> +void snp_leak_pages(u64 pfn, unsigned int npages)
> +{
> +	struct page *page = pfn_to_page(pfn);
> +
> +	pr_debug("%s: leaking PFN range 0x%llx-0x%llx\n", __func__, pfn, pfn + npages);
> +
> +	spin_lock(&snp_leaked_pages_list_lock);
> +	while (npages--) {
> +		/*
> +		 * Reuse the page's buddy list for chaining into the leaked
> +		 * pages list. This page should not be on a free list currently
> +		 * and is also unsafe to be added to a free list.
> +		 */
> +		list_add_tail(&page->buddy_list, &snp_leaked_pages_list);
> +		sev_dump_rmpentry(pfn);
> +		pfn++;

You increment pfn, but not page, which is always pointing to the page of the
initial pfn, so need to do page++ too.
But that assumes it's all order-0 pages (hard to tell for me whether that's
true as we start with a pfn), if there can be compound pages, it would be
best to only add the head page and skip the tail pages - it's not expected
to use page->buddy_list of tail pages.

> +	}
> +	spin_unlock(&snp_leaked_pages_list_lock);
> +	atomic_long_inc(&snp_nr_leaked_pages);
> +}
> +EXPORT_SYMBOL_GPL(snp_leak_pages);
  

On 12/6/2023 2:42 PM, Borislav Petkov wrote:
> On Mon, Oct 16, 2023 at 08:27:45AM -0500, Michael Roth wrote:
>> +	spin_lock(&snp_leaked_pages_list_lock);
>> +	while (npages--) {
>> +		/*
>> +		 * Reuse the page's buddy list for chaining into the leaked
>> +		 * pages list. This page should not be on a free list currently
>> +		 * and is also unsafe to be added to a free list.
>> +		 */
>> +		list_add_tail(&page->buddy_list, &snp_leaked_pages_list);
>> +		sev_dump_rmpentry(pfn);
>> +		pfn++;
>> +	}
>> +	spin_unlock(&snp_leaked_pages_list_lock);
>> +	atomic_long_inc(&snp_nr_leaked_pages);
> 
> How is this supposed to count?
> 
> You're leaking @npages as the function's parameter but are incrementing
> snp_nr_leaked_pages only once?
> 
> Just make it a bog-normal unsigned long and increment it inside the
> locked section.
> 
> Or do at the beginning of the function:
> 
> 	atomic_long_add(npages, &snp_nr_leaked_pages);
> 

Yes will fix accordingly by incrementing it inside the locked section.

Thanks,
Ashish
  

Hello Vlastimil,

On 12/7/2023 10:20 AM, Vlastimil Babka wrote:

>> +
>> +void snp_leak_pages(u64 pfn, unsigned int npages)
>> +{
>> +	struct page *page = pfn_to_page(pfn);
>> +
>> +	pr_debug("%s: leaking PFN range 0x%llx-0x%llx\n", __func__, pfn, pfn + npages);
>> +
>> +	spin_lock(&snp_leaked_pages_list_lock);
>> +	while (npages--) {
>> +		/*
>> +		 * Reuse the page's buddy list for chaining into the leaked
>> +		 * pages list. This page should not be on a free list currently
>> +		 * and is also unsafe to be added to a free list.
>> +		 */
>> +		list_add_tail(&page->buddy_list, &snp_leaked_pages_list);
>> +		sev_dump_rmpentry(pfn);
>> +		pfn++;
> 
> You increment pfn, but not page, which is always pointing to the page of the
> initial pfn, so need to do page++ too.

Yes, that is a bug and needs to be fixed.

> But that assumes it's all order-0 pages (hard to tell for me whether that's
> true as we start with a pfn), if there can be compound pages, it would be
> best to only add the head page and skip the tail pages - it's not expected
> to use page->buddy_list of tail pages.

Can't we use PageCompound() to check if the page is a compound page and 
then use page->compound_head to get and add the head page to leaked 
pages list. I understand the tail pages for compound pages are really 
limited for usage.

Thanks,
Ashish
  

On 12/8/23 23:10, Kalra, Ashish wrote:
> Hello Vlastimil,
> 
> On 12/7/2023 10:20 AM, Vlastimil Babka wrote:
> 
>>> +
>>> +void snp_leak_pages(u64 pfn, unsigned int npages)
>>> +{
>>> +    struct page *page = pfn_to_page(pfn);
>>> +
>>> +    pr_debug("%s: leaking PFN range 0x%llx-0x%llx\n", __func__, pfn,
>>> pfn + npages);
>>> +
>>> +    spin_lock(&snp_leaked_pages_list_lock);
>>> +    while (npages--) {
>>> +        /*
>>> +         * Reuse the page's buddy list for chaining into the leaked
>>> +         * pages list. This page should not be on a free list currently
>>> +         * and is also unsafe to be added to a free list.
>>> +         */
>>> +        list_add_tail(&page->buddy_list, &snp_leaked_pages_list);
>>> +        sev_dump_rmpentry(pfn);
>>> +        pfn++;
>>
>> You increment pfn, but not page, which is always pointing to the page
>> of the
>> initial pfn, so need to do page++ too.
> 
> Yes, that is a bug and needs to be fixed.
> 
>> But that assumes it's all order-0 pages (hard to tell for me whether
>> that's
>> true as we start with a pfn), if there can be compound pages, it would be
>> best to only add the head page and skip the tail pages - it's not
>> expected
>> to use page->buddy_list of tail pages.
> 
> Can't we use PageCompound() to check if the page is a compound page and
> then use page->compound_head to get and add the head page to leaked
> pages list. I understand the tail pages for compound pages are really
> limited for usage.

Yeah that should work. Need to be careful though, should probably only
process head pages and check if the whole compound_order() is within the
range we are to leak, and then leak the head page and advance the loop
by compound_order(). And if we encounter a tail page, it should probably
be just skipped. I'm looking at snp_reclaim_pages() which seems to
process a number of pages with SEV_CMD_SNP_PAGE_RECLAIM and once any
fails, call snp_leak_pages() on the rest. Could that invoke
snp_leak_pages with the first pfn being a tail page?

> Thanks,
> Ashish
  

Hello Vlastimil,

On 12/11/2023 7:08 AM, Vlastimil Babka wrote:
> 
> 
> On 12/8/23 23:10, Kalra, Ashish wrote:
>> Hello Vlastimil,
>>
>> On 12/7/2023 10:20 AM, Vlastimil Babka wrote:
>>
>>>> +
>>>> +void snp_leak_pages(u64 pfn, unsigned int npages)
>>>> +{
>>>> +    struct page *page = pfn_to_page(pfn);
>>>> +
>>>> +    pr_debug("%s: leaking PFN range 0x%llx-0x%llx\n", __func__, pfn,
>>>> pfn + npages);
>>>> +
>>>> +    spin_lock(&snp_leaked_pages_list_lock);
>>>> +    while (npages--) {
>>>> +        /*
>>>> +         * Reuse the page's buddy list for chaining into the leaked
>>>> +         * pages list. This page should not be on a free list currently
>>>> +         * and is also unsafe to be added to a free list.
>>>> +         */
>>>> +        list_add_tail(&page->buddy_list, &snp_leaked_pages_list);
>>>> +        sev_dump_rmpentry(pfn);
>>>> +        pfn++;
>>>
>>> You increment pfn, but not page, which is always pointing to the page
>>> of the
>>> initial pfn, so need to do page++ too.
>>
>> Yes, that is a bug and needs to be fixed.
>>
>>> But that assumes it's all order-0 pages (hard to tell for me whether
>>> that's
>>> true as we start with a pfn), if there can be compound pages, it would be
>>> best to only add the head page and skip the tail pages - it's not
>>> expected
>>> to use page->buddy_list of tail pages.
>>
>> Can't we use PageCompound() to check if the page is a compound page and
>> then use page->compound_head to get and add the head page to leaked
>> pages list. I understand the tail pages for compound pages are really
>> limited for usage.
> 
> Yeah that should work. Need to be careful though, should probably only
> process head pages and check if the whole compound_order() is within the
> range we are to leak, and then leak the head page and advance the loop
> by compound_order(). And if we encounter a tail page, it should probably
> be just skipped. I'm looking at snp_reclaim_pages() which seems to
> process a number of pages with SEV_CMD_SNP_PAGE_RECLAIM and once any
> fails, call snp_leak_pages() on the rest. Could that invoke
> snp_leak_pages with the first pfn being a tail page?

Yes i don't think we can assume that the first pfn will not be a tail 
page. But then this becomes complex as we might have already reclaimed 
the head page and one or more tail pages successfully or probably never 
transitioned head page to FW state as alloc_page()/alloc_pages() would 
have returned subpage(s) of a largepage.

But then we really can't use the buddy_list of a tail page to insert it 
in the snp leaked pages list, right ?

These non-reclaimed pages are not usable anymore anyway, any access to 
them will cause fatal RMP #PF, so don't know if i can use the buddy_list 
to insert tail pages as that will corrupt the page metadata ?

We initially used to invoke memory_failure() here to try to gracefully 
handle failure of these non-reclaimed pages and that used to handle 
hugepages, etc., but as pointed in previous review feedback that is not 
a logical approach for this as that's meant more for the RAS stuff.

Maybe it is a simpler approach to have our own container object on top 
and have this page pointer and list_head in it and use that list_head to 
insert into the snp leaked list instead of re-using the buddy_list for 
chaining into the leaked pages list ?

Thanks,
Ashish