| Message ID | 614257.1719228181@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | netfs: Fix netfs_page_mkwrite() to check folio->mapping is valid | expand |
On Mon, Jun 24, 2024 at 12:23:01PM +0100, David Howells wrote: > @@ -508,6 +509,10 @@ vm_fault_t netfs_page_mkwrite(struct vm_fault *vmf, struct netfs_group *netfs_gr > > if (folio_lock_killable(folio) < 0) > goto out; > + if (folio->mapping != mapping) { > + ret = VM_FAULT_NOPAGE | VM_FAULT_LOCKED; > + goto out; > + } Have you tested this? I'd expect it to throw some VM assertions. ret = vmf->vma->vm_ops->page_mkwrite(vmf); /* Restore original flags so that caller is not surprised */ vmf->flags = old_flags; if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE))) return ret; ... if (unlikely(!tmp || (tmp & (VM_FAULT_ERROR | VM_FAULT_NOPAGE)))) { folio_put(folio); return tmp; } So you locked the folio, then called folio_put() without unlocking it. Usually the VM complains noisily if you free a locked folio.
Matthew Wilcox <willy@infradead.org> wrote: > > if (folio_lock_killable(folio) < 0) > > goto out; > > + if (folio->mapping != mapping) { > > + ret = VM_FAULT_NOPAGE | VM_FAULT_LOCKED; > > + goto out; > > + } > > Have you tested this? I've tried to. generic/247 can trigger it, but the race happens rarely. > I'd expect it to throw some VM assertions. I didn't see any. I guess VM_FAULT_LOCKED is not universally handled by the caller and that I should unlock the folio myself instead. David
diff --git a/fs/netfs/buffered_write.c b/fs/netfs/buffered_write.c index c36643c97cb5..6a6387b3aaff 100644 --- a/fs/netfs/buffered_write.c +++ b/fs/netfs/buffered_write.c @@ -497,6 +497,7 @@ vm_fault_t netfs_page_mkwrite(struct vm_fault *vmf, struct netfs_group *netfs_gr struct netfs_group *group; struct folio *folio = page_folio(vmf->page); struct file *file = vmf->vma->vm_file; + struct address_space *mapping = file->f_mapping; struct inode *inode = file_inode(file); struct netfs_inode *ictx = netfs_inode(inode); vm_fault_t ret = VM_FAULT_RETRY; @@ -508,6 +509,10 @@ vm_fault_t netfs_page_mkwrite(struct vm_fault *vmf, struct netfs_group *netfs_gr if (folio_lock_killable(folio) < 0) goto out; + if (folio->mapping != mapping) { + ret = VM_FAULT_NOPAGE | VM_FAULT_LOCKED; + goto out; + } if (folio_wait_writeback_killable(folio)) { ret = VM_FAULT_LOCKED; @@ -523,7 +528,7 @@ vm_fault_t netfs_page_mkwrite(struct vm_fault *vmf, struct netfs_group *netfs_gr group = netfs_folio_group(folio); if (group != netfs_group && group != NETFS_FOLIO_COPY_TO_CACHE) { folio_unlock(folio); - err = filemap_fdatawait_range(inode->i_mapping, + err = filemap_fdatawait_range(mapping, folio_pos(folio), folio_pos(folio) + folio_size(folio)); switch (err) {
Fix netfs_page_mkwrite() to check that folio->mapping is valid once it has taken the folio lock (as filemap_page_mkwrite() does). Without this, generic/247 occasionally oopses with something like the following: BUG: kernel NULL pointer dereference, address: 0000000000000000 #PF: supervisor read access in kernel mode #PF: error_code(0x0000) - not-present page RIP: 0010:trace_event_raw_event_netfs_folio+0x61/0xc0 ... Call Trace: <TASK> ? __die_body+0x1a/0x60 ? page_fault_oops+0x6e/0xa0 ? exc_page_fault+0xc2/0xe0 ? asm_exc_page_fault+0x22/0x30 ? trace_event_raw_event_netfs_folio+0x61/0xc0 trace_netfs_folio+0x39/0x40 netfs_page_mkwrite+0x14c/0x1d0 do_page_mkwrite+0x50/0x90 do_pte_missing+0x184/0x200 __handle_mm_fault+0x42d/0x500 handle_mm_fault+0x121/0x1f0 do_user_addr_fault+0x23e/0x3c0 exc_page_fault+0xc2/0xe0 asm_exc_page_fault+0x22/0x30 This is due to the invalidate_inode_pages2_range() issued at the end of the DIO write interfering with the mmap'd writes. Fixes: 102a7e2c598c ("netfs: Allow buffered shared-writeable mmap through netfs_page_mkwrite()") Signed-off-by: David Howells <dhowells@redhat.com> cc: Matthew Wilcox <willy@infradead.org> cc: Jeff Layton <jlayton@kernel.org> cc: netfs@lists.linux.dev cc: v9fs@lists.linux.dev cc: linux-afs@lists.infradead.org cc: linux-cifs@vger.kernel.org cc: linux-mm@kvack.org cc: linux-fsdevel@vger.kernel.org --- fs/netfs/buffered_write.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)