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

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

[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 <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(-)

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) {

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

[Matthew Wilcox]

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.

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

[David Howells]

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

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

[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 <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
---
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(-)

diff --git a/fs/netfs/buffered_write.c b/fs/netfs/buffered_write.c
index 07bc1fd43530..270f8ebf8328 100644
--- a/fs/netfs/buffered_write.c
+++ b/fs/netfs/buffered_write.c
@@ -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) {

Re: [PATCH v2] netfs: Fix netfs_page_mkwrite() to check folio->mapping is valid

[Jeff Layton]

On Tue, 2024-06-25 at 13:29 +0100, David Howells wrote:
>     
> 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
> ---
> 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(-)
> 
> diff --git a/fs/netfs/buffered_write.c b/fs/netfs/buffered_write.c
> index 07bc1fd43530..270f8ebf8328 100644
> --- a/fs/netfs/buffered_write.c
> +++ b/fs/netfs/buffered_write.c
> @@ -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) {
> 
> 

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2] netfs: Fix netfs_page_mkwrite() to check folio->mapping is valid

[Christian Brauner]

On Tue, 25 Jun 2024 13:29:06 +0100, David Howells wrote:
> 
> 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