netfs: Fix netfs_page_mkwrite() to check folio->mapping is valid

PATCH Superseded David Howells 4 participants 6 messages Jun 24, 2024 – Jun 26, 2024

Subsystems (6)

Memory management VFS SMB client (CIFS) Kernel SMB3 server (ksmbd) +2 more subsystems

lore

Share & tools

lore

David Howells

author maintainer

2024-06-24 11:23 UTC #0 lore raw

PATCH 1/1 netfs: Fix netfs_page_mkwrite() to check folio->mapping is valid

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 <[email protected]>

cc Matthew Wilcox <[email protected]>

cc Jeff Layton <[email protected]>

cc [email protected]

---
 fs/netfs/buffered_write.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

fs/netfs/buffered_write.c +6 -1

index c36643c97cb5..6a6387b3aaff 100644
@@ -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) {

Matthew Wilcox

2024-06-24 15:30 UTC #1 lore raw

fs/netfs/buffered_write.c vm_fault_t netfs_page_mkwrite(struct vm_fault *vmf, struct netfs_group *netfs_gr · line 515

Quoted 8 lines · David Howells

fs/netfs/buffered_write.c +4 -0

@@ -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.

David Howells

author maintainer

2024-06-24 22:40 UTC #2 lore raw

Quoted 8 lines · Matthew Wilcox

fs/netfs/buffered_write.c vm_fault_t netfs_page_mkwrite(struct vm_fault *vmf, struct netfs_group *netfs_gr · line 515
Quoted 6 lines
fs/netfs/buffered_write.c +4 -0
 	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.

Quoted 1 line

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

David Howells

author maintainer

2024-06-25 12:29 UTC #3 lore raw

PATCH v2 1/1 netfs: Fix netfs_page_mkwrite() to check folio->mapping is valid

In reply to David Howells

    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 <[email protected]>

cc Matthew Wilcox <[email protected]>

cc Jeff Layton <[email protected]>

cc [email protected]

---
Changes
=======
ver #2)
- Actually unlock the folio rather than returning VM_FAULT_LOCKED with VM_FAULT_NOPAGE.

 fs/netfs/buffered_write.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

fs/netfs/buffered_write.c +7 -1

index 07bc1fd43530..270f8ebf8328 100644
@@ -523,6 +523,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;

@@ -534,6 +535,11 @@ 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) {
+		folio_unlock(folio);
+		ret = VM_FAULT_NOPAGE;
+		goto out;
+	}
 
 	if (folio_wait_writeback_killable(folio)) {
 		ret = VM_FAULT_LOCKED;

@@ -549,7 +555,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) {

Jeff Layton

Reviewed-by on v2 1/1

2024-06-25 14:54 UTC #4 lore raw

Quoted 89 lines · David Howells

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 <[email protected]>
cc Matthew Wilcox <[email protected]>
cc Jeff Layton <[email protected]>
cc [email protected]
cc [email protected]
cc [email protected]
cc [email protected]
cc [email protected]
cc [email protected]
---
Changes
=======
ver #2)
- Actually unlock the folio rather than returning VM_FAULT_LOCKED with
VM_FAULT_NOPAGE.
fs/netfs/buffered_write.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
fs/netfs/buffered_write.c +1 -0
index 07bc1fd43530..270f8ebf8328 100644
@@ -523,6 +523,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;
fs/netfs/buffered_write.c +0 -0
@@ -534,6 +535,11 @@ vm_fault_t netfs_page_mkwrite(struct vm_fault
 *vmf, struct netfs_group *netfs_gr
if (folio_lock_killable(folio) < 0)
goto out;
fs/netfs/buffered_write.c +5 -0
+	if (folio->mapping != mapping) {
+		folio_unlock(folio);
+		ret = VM_FAULT_NOPAGE;
+		goto out;
+	}
if (folio_wait_writeback_killable(folio)) { ret = VM_FAULT_LOCKED;
fs/netfs/buffered_write.c +0 -0
@@ -549,7 +555,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);
fs/netfs/buffered_write.c +1 -1
-		err = filemap_fdatawait_range(inode->i_mapping,
+		err = filemap_fdatawait_range(mapping,

Quoted 6 lines

folio_pos(folio), folio_pos(folio) + folio_size(folio));
switch (err) {

Reviewed-by Jeff Layton <[email protected]>

Christian Brauner

maintainer Applied

2024-06-26 12:19 UTC #5 lore raw

In reply to David Howells

Quoted 10 lines · David Howells

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
[...]

Applied to the vfs.fixes branch of the vfs/vfs.git tree. Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/1] netfs: Fix netfs_page_mkwrite() to check folio->mapping is valid

      https://git.kernel.org/vfs/vfs/c/a81c98bfa40c