"David Hildenbrand (Arm)" <[email protected]> writes:
> On 8/7/26 23:52, Ackerley Tng via B4 Relay wrote:
>> From: Ackerley Tng <[email protected]>
>>
>> When converting memory to private in guest_memfd, it is necessary to ensure
>> that the pages are not currently being accessed by any other part of the
>> kernel or userspace to avoid any current user writing to guest private
>> memory.
>>
>> guest_memfd checks for unexpected refcounts to determine whether a page is
>> still in use. The only expected refcounts after unmapping the range
>> requested for conversion are those that are held by guest_memfd itself.
>>
>> Update the kvm_memory_attributes2 structure to include an error_offset
>> field. This allows KVM to report the exact offset where a conversion
>> failed to userspace. If the safety check fails, return -EAGAIN and copy
>> the error_offset back to userspace so that it can potentially retry the
>> operation or handle the failure gracefully.
>>
>> Update documentation to document the error_offset field and the possible
>> -EAGAIN error.
>>
>> Suggested-by: David Hildenbrand <[email protected]>
>> Co-developed-by: Vishal Annapurve <[email protected]>
>> Signed-off-by: Vishal Annapurve <[email protected]>
>> Reviewed-by: Fuad Tabba <[email protected]>
>> Tested-by: Shivank Garg <[email protected]>
>> Signed-off-by: Ackerley Tng <[email protected]>
>> ---
>
> [...]
>
>> #define KVM_MEMORY_ATTRIBUTE_PRIVATE (1ULL << 3)
>> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
>> index 3783e63476569..13c3989136f67 100644
>> --- a/virt/kvm/guest_memfd.c
>> +++ b/virt/kvm/guest_memfd.c
>> @@ -524,8 +524,42 @@ static int kvm_gmem_mas_preallocate(struct ma_state
>> *mas, u64 attributes,
>> return mas_preallocate(mas, xa_mk_value(attributes), GFP_KERNEL);
>> }
>>
>> +static bool kvm_gmem_is_safe_for_conversion(struct inode *inode, pgoff_t
>> start,
>> + size_t nr_pages, pgoff_t *err_index)
>
> I would focus on the "to_private" aspect or abstract it to
> "kvm_gmem_mem_has_unexpected_refs" or sth like that.
>
Do you mean something like kvm_gmem_is_safe_for_to_private_conversion,
as in that you want to emphasise that "safe" here refers to a to_private
and not a to_shared conversion?
Perhaps a little ahead of its time, but later with restructuring for
huge pages, we also need no additional refcounts other than gmem's own
so that restructuring is safe, hence this function name was meant to
extend there as well.
In this case "unexpected" (especially since the next patch adds checks
for maybe dma pinned and unmapping), begs the question "unexpected in
what way"?
>> +{
>> + struct address_space *mapping = inode->i_mapping;
>> + const int filemap_get_folios_refcount = 1;
>> + pgoff_t last = start + nr_pages - 1;
>> + struct folio_batch fbatch;
>> + bool safe = true;
>> + pgoff_t next;
>> + int i;
>> +
>> + folio_batch_init(&fbatch);
>> +
>> + next = start;
>> + while (safe && filemap_get_folios(mapping, &next, last, &fbatch)) {
>> + for (i = 0; i < folio_batch_count(&fbatch); ++i) {
>> + struct folio *folio = fbatch.folios[i];
>> +
>> + if (folio_ref_count(folio) !=
>> + folio_nr_pages(folio) +
>> filemap_get_folios_refcount) {
>
> I'd rather add a comment than have this filemap_get_folios_refcount.
>
> /*
> * We expect one reference per folio-page in the pagecache and one
> * reference from filemap_get_folios().
> */
> if (folio_ref_count(folio) != folio_nr_pages(folio) + 1)
>
This comment explains what's "unexpected". I can do this and switch it
to kvm_gmem_mem_has_unexpected_refs() unless people have other
suggestions.
I wish there was a folio_pagecache_refs(folio) that
folio_expected_ref_count() can share with this, and also
folio_swapcache_refs(), to solidify the definition of refcounts taken by
the pagecache.
>> + safe = false;
>> + *err_index = max(start, folio->index);
>> + break;
>> + }
>> + }
>> +
>> + folio_batch_release(&fbatch);
>> + cond_resched();
>> + }
>> +
>> + return safe;
>> +}
>> +
>> static int __kvm_gmem_set_attributes(struct inode *inode, pgoff_t start,
>> - size_t nr_pages, uint64_t attrs)
>> + size_t nr_pages, uint64_t attrs,
>> + pgoff_t *err_index)
>> {
>> bool to_private = attrs & KVM_MEMORY_ATTRIBUTE_PRIVATE;
>> struct address_space *mapping = inode->i_mapping;
>> @@ -542,8 +576,21 @@ static int __kvm_gmem_set_attributes(struct inode
>> *inode, pgoff_t start,
>>
>> mas_init(&mas, mt, start);
>> r = kvm_gmem_mas_preallocate(&mas, attrs, start, nr_pages);
>> - if (r)
>> + if (r) {
>> + *err_index = start;
>> goto out;
>> + }
>> +
>> + if (to_private) {
>
> I'd add a comment here for the "why are we unmapping".
>
Does this sound right:
Unmap here to ensure that userspace page tables have no mappings, which
also ensures refcounts from those mappings are dropped.
> --
> Cheers,
>
> David
