Sean Christopherson <[email protected]> writes: > On Mon, Mar 09, 2026, Ackerley Tng wrote: >> Set release always on guest_memfd mappings to enable the use of >> .invalidate_folio, which performs inode accounting for guest_memfd. >> >> Signed-off-by: Ackerley Tng <[email protected]> >> --- >> virt/kvm/guest_memfd.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c >> index 77219551056a7..8246b9fbcf832 100644 >> --- a/virt/kvm/guest_memfd.c >> +++ b/virt/kvm/guest_memfd.c >> @@ -607,6 +607,7 @@ static int __kvm_gmem_create(struct kvm *kvm, loff_t >> size, u64 flags) >> mapping_set_inaccessible(inode->i_mapping); >> /* Unmovable mappings are supposed to be marked unevictable as well. */ >> WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping)); >> + mapping_set_release_always(inode->i_mapping); > > *sigh* > > So... an internal AI review bot flagged setting AS_RELEASE_ALWAYS as being > potentially problematic, and I started poking around, mostly because I was > curious. I'm pretty sure the exact scenario painted by the bot isn't > possible, > but I do think a similar issue exists in at least truncate_error_folio(). Or > at > least, *should* exist, but doesn't because of a different bug. > > On memory error, kvm_gmem_error_folio() will get invoked via this code. Note > the "err != 0" check. kvm_gmem_error_folio() returns MF_DELAYED, which has an > arbitrary value of '2', and so KVM is always signalling "failure". > > int err = mapping->a_ops->error_remove_folio(mapping, folio); > > if (err != 0) > pr_info("%#lx: Failed to punch page: %d\n", pfn, err); > else if (!filemap_release_folio(folio, GFP_NOIO)) > pr_info("%#lx: failed to release buffers\n", pfn); > > I _think_ that's bad? On x86, if I'm following the breadcrubs correctly, > we'll > end up in this code in kill_me_maybe() > > pr_err("Memory error not recovered"); > kill_me_now(cb); > > and send what I assume is a relatively useless SIGBUS and likely kill the VM. > > struct task_struct *p = container_of(ch, struct task_struct, > mce_kill_me); > > p->mce_count = 0; > force_sig(SIGBUS); >
Glad you agree that's bad :). We reported this earlier and Lisa has been working on a fix. RFC v1 is here [1]. RFC v2 (Lisa is about to post it) will address David's comments by aligning shmem to also return MF_DELAYED like guest_memfd, then handling MF_DELAYED returned from .error_remove_folio() accordingly so that it's not interpreted as a memory failure handling failure. [1] https://lore.kernel.org/all/[email protected]/#r > But even if that's somehow the "right" behavior, we're doing it purely by > accident. > > As for this patch, if we fix that bug by returning 0, then > filemap_release_folio() > is definitely reachable by at least one flow, so I think guest_memfd also > needs > to implement release_folio()? > Is posix_fadvise() the one flow you're talking about? It indeed calls filemap_release_folio() through mapping_try_invalidate() -> mapping_evict_folio() -> filemap_release_folio(). >From Documentation/filesystems/locking.rst: ->release_folio() is called when the MM wants to make a change to the folio that would invalidate the filesystem's private data. For example, it may be about to be removed from the address_space or split. The folio is locked and not under writeback. It may be dirty. The gfp parameter is not usually used for allocation, but rather to indicate what the filesystem may do to attempt to free the private data. The filesystem may return false to indicate that the folio's private data cannot be freed. If it returns true, it should have already removed the private data from the folio. If a filesystem does not provide a ->release_folio method, the pagecache will assume that private data is buffer_heads and call try_to_free_buffers(). I could implement .release_folio(). Returning false seems like the easier solution, and is kind of in line with the documentation above. A guest_memfd folio does not have private data, so without private data, the private data cannot be freed. (Took me a while to notice that having private data is not the same as having something in folio->private, so this doesn't change even after the direct map removal series lands.) Returning false is going to break shrink_folio_list(), but that probably won't affect guest_memfd for now. Returning false also breaks page_cache_pipe_buf_try_steal(). Does anyone more familiar with splicing know if that could affect guest_memfd? Returning true could also work, to indicate that the folio's private data has been "removed". I'd also have to do inode_sub_bytes() in .release_folio() then, since in mapping_evict_folio(), remove_mapping() doesn't call .invalidate_folio(). Then we will have to separately ensure that in truncate_error_folio(), guest_memfd doesn't double-deduct the folio's size from the inode. This should be semantically correct though, since IIUC .invalidate_folio() is when a folio is removed (clean or dirty), but .release_folio() is only for clean folios. If .error_remove_folio() returns MF_DELAYED, the truncation didn't happen and so there should be no call to .release_folio(). > > > Full AI bot text: > -- > Setting the AS_RELEASE_ALWAYS flag causes folio_needs_release() to return > true. This correctly triggers .invalidate_folio during truncation, but does > it also unintentionally expose guest_memfd folios to eviction via > posix_fadvise(POSIX_FADV_DONTNEED)? > > If userspace calls posix_fadvise() on a guest_memfd file, the core mm > calls mapping_evict_folio(). Because folio_needs_release() is true, it > calls filemap_release_folio(). > > Since guest_memfd does not implement a .release_folio address space > operation, filemap_release_folio() falls back to calling > try_to_free_buffers(). Could this fallback cause a warning? > > fs/buffer.c:try_to_free_buffers() { > ... > /* Misconfigured folio check */ > if (WARN_ON_ONCE(!folio_buffers(folio))) > return true; > ... > } > > Because the guest_memfd folio has no private data, folio_buffers() > is NULL, which will trigger this WARN_ON_ONCE. > > Furthermore, try_to_free_buffers() returns true, allowing the folio to be > removed from the page cache. Because this eviction path bypasses > truncate_cleanup_folio(), it never calls .invalidate_folio. > > Does this mean inode_sub_bytes() is skipped, leaking the inode block > accounting? > > Userspace could potentially trigger the warning and infinitely inflate the > inode's block count with: > struct kvm_create_guest_memfd args = { .size = 4096 }; > int fd = ioctl(kvm_vm_fd, KVM_CREATE_GUEST_MEMFD, &args); > fallocate(fd, 0, 0, 4096); > posix_fadvise(fd, 0, 4096, POSIX_FADV_DONTNEED); > Should guest_memfd implement a .release_folio callback that simply > returns false to prevent these folios from being evicted? > --
