linux-kernel - Re: [PATCH RFC v3 1/2] KVM: guest_memfd: Convert .free_folio() to .release

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: <d2147b7c-bb2e-4434-aa10-40cacac43d4f@redhat.com>
Date: Fri, 15 Nov 2024 11:58:59 +0100
From: David Hildenbrand <david@...hat.com>
To: Elliot Berman <quic_eberman@...cinc.com>,
 Paolo Bonzini <pbonzini@...hat.com>,
 Andrew Morton <akpm@...ux-foundation.org>,
 Sean Christopherson <seanjc@...gle.com>, Fuad Tabba <tabba@...gle.com>,
 Ackerley Tng <ackerleytng@...gle.com>, Mike Rapoport <rppt@...nel.org>,
 "H. Peter Anvin" <hpa@...or.com>, Matthew Wilcox <willy@...radead.org>
Cc: James Gowans <jgowans@...zon.com>, linux-fsdevel@...r.kernel.org,
 kvm@...r.kernel.org, linux-coco@...ts.linux.dev,
 linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-mm@...ck.org
Subject: Re: [PATCH RFC v3 1/2] KVM: guest_memfd: Convert .free_folio() to
 .release_folio()

On 15.11.24 11:58, David Hildenbrand wrote:
> On 13.11.24 23:34, Elliot Berman wrote:
>> When guest_memfd becomes a library, a callback will need to be made to
>> the owner (KVM SEV) to transition pages back to hypervisor-owned/shared
>> state. This is currently being done as part of .free_folio() address
>> space op, but this callback shouldn't assume that the mapping still
>> exists. guest_memfd library will need the mapping to still exist to look
>> up its operations table.
> 
> I assume you mean, that the mapping is no longer set for the folio (it
> sure still exists, because we are getting a callback from it :) )?
> 
> Staring at filemap_remove_folio(), this is exactly what happens:
> 
> We remember folio->mapping, call __filemap_remove_folio(), and then call
> filemap_free_folio() where we zap folio->mapping via page_cache_delete().
> 
> Maybe it's easier+cleaner to also forward the mapping to the
> free_folio() callback, just like we do with filemap_free_folio()? Would
> that help?
> 
> CCing Willy if that would be reasonable extension of the free_folio
> callback.
> 

Now really CCing him. :)

> 
>>
>> .release_folio() and .invalidate_folio() address space ops can serve the
>> same purpose here. The key difference between release_folio() and
>> free_folio() is whether the mapping is still valid at time of the
>> callback. This approach was discussed in the link in the footer, but not
>> taken because free_folio() was easier to implement.
>>
>> Link: https://lore.kernel.org/kvm/20231016115028.996656-1-michael.roth@amd.com/
>> Signed-off-by: Elliot Berman <quic_eberman@...cinc.com>
>> ---
>>    virt/kvm/guest_memfd.c | 19 ++++++++++++++++---
>>    1 file changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
>> index 47a9f68f7b247f4cba0c958b4c7cd9458e7c46b4..13f83ad8a4c26ba82aca4f2684f22044abb4bc19 100644
>> --- a/virt/kvm/guest_memfd.c
>> +++ b/virt/kvm/guest_memfd.c
>> @@ -358,22 +358,35 @@ static int kvm_gmem_error_folio(struct address_space *mapping, struct folio *fol
>>    }
>>    
>>    #ifdef CONFIG_HAVE_KVM_ARCH_GMEM_INVALIDATE
>> -static void kvm_gmem_free_folio(struct folio *folio)
>> +static bool kvm_gmem_release_folio(struct folio *folio, gfp_t gfp)
>>    {
>>    	struct page *page = folio_page(folio, 0);
>>    	kvm_pfn_t pfn = page_to_pfn(page);
>>    	int order = folio_order(folio);
>>    
>>    	kvm_arch_gmem_invalidate(pfn, pfn + (1ul << order));
>> +
>> +	return true;
>> +}
>> +
>> +static void kvm_gmem_invalidate_folio(struct folio *folio, size_t offset,
>> +				      size_t len)
>> +{
>> +	WARN_ON_ONCE(offset != 0);
>> +	WARN_ON_ONCE(len != folio_size(folio));
>> +
>> +	if (offset == 0 && len == folio_size(folio))
>> +		filemap_release_folio(folio, 0);
>>    }
>>    #endif
>>    
>>    static const struct address_space_operations kvm_gmem_aops = {
>>    	.dirty_folio = noop_dirty_folio,
>> -	.migrate_folio	= kvm_gmem_migrate_folio,
>> +	.migrate_folio = kvm_gmem_migrate_folio,
>>    	.error_remove_folio = kvm_gmem_error_folio,
>>    #ifdef CONFIG_HAVE_KVM_ARCH_GMEM_INVALIDATE
>> -	.free_folio = kvm_gmem_free_folio,
>> +	.release_folio = kvm_gmem_release_folio,
>> +	.invalidate_folio = kvm_gmem_invalidate_folio,
>>    #endif
>>    };
>>    
>>
> 
> 


-- 
Cheers,

David / dhildenb