[PATCH 1/2] filemap: fix the conditional folio_put in filemap_fault

[PATCH 1/2] filemap: fix the conditional folio_put in filemap_fault

[Christoph Hellwig]

folio can't be NULL here now that __filemap_get_folio returns an
ERR_PTR.  Remove the conditional folio_put after the out_retry
label and add a new label for the cases where we have a valid folio.

Fixes: 66dabbb65d67 ("mm: return an ERR_PTR from __filemap_get_folio")
Reported-by: syzbot+48011b86c8ea329af1b9@syzkaller.appspotmail.com
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 mm/filemap.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index a34abfe8c65430..ae597f63a9bc54 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -3298,7 +3298,7 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
 	}
 
 	if (!lock_folio_maybe_drop_mmap(vmf, folio, &fpin))
-		goto out_retry;
+		goto out_retry_put_folio;
 
 	/* Did it get truncated? */
 	if (unlikely(folio->mapping != mapping)) {
@@ -3334,7 +3334,7 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
 	 */
 	if (fpin) {
 		folio_unlock(folio);
-		goto out_retry;
+		goto out_retry_put_folio;
 	}
 	if (mapping_locked)
 		filemap_invalidate_unlock_shared(mapping);
@@ -3363,7 +3363,7 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
 	fpin = maybe_unlock_mmap_for_io(vmf, fpin);
 	error = filemap_read_folio(file, mapping->a_ops->read_folio, folio);
 	if (fpin)
-		goto out_retry;
+		goto out_retry_put_folio;
 	folio_put(folio);
 
 	if (!error || error == AOP_TRUNCATED_PAGE)
@@ -3372,14 +3372,14 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
 
 	return VM_FAULT_SIGBUS;
 
+out_retry_put_folio:
+	folio_put(folio);
 out_retry:
 	/*
 	 * We dropped the mmap_lock, we need to return to the fault handler to
 	 * re-find the vma and come back and find our hopefully still populated
 	 * page.
 	 */
-	if (folio)
-		folio_put(folio);
 	if (mapping_locked)
 		filemap_invalidate_unlock_shared(mapping);
 	if (fpin)
-- 
2.39.2

[PATCH 2/2] afs: fix the afs_dir_get_folio return value

[Christoph Hellwig]

Keep returning NULL on failure instead of letting an ERR_PTR escape to
callers that don't expect it.

Fixes: 66dabbb65d67 ("mm: return an ERR_PTR from __filemap_get_folio")
Reported-by: Jan Kara <jack@suse.cz>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/afs/dir_edit.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/afs/dir_edit.c b/fs/afs/dir_edit.c
index f0eddccbdd9541..e2fa577b66fe0a 100644
--- a/fs/afs/dir_edit.c
+++ b/fs/afs/dir_edit.c
@@ -115,11 +115,12 @@ static struct folio *afs_dir_get_folio(struct afs_vnode *vnode, pgoff_t index)
 	folio = __filemap_get_folio(mapping, index,
 				    FGP_LOCK | FGP_ACCESSED | FGP_CREAT,
 				    mapping->gfp_mask);
-	if (IS_ERR(folio))
+	if (IS_ERR(folio)) {
 		clear_bit(AFS_VNODE_DIR_VALID, &vnode->flags);
-	else if (folio && !folio_test_private(folio))
+		return NULL;
+	}
+	if (!folio_test_private(folio))
 		folio_attach_private(folio, (void *)1);
-
 	return folio;
 }
 
-- 
2.39.2

Re: [PATCH 1/2] filemap: fix the conditional folio_put in filemap_fault

[Matthew Wilcox]

On Wed, May 03, 2023 at 05:45:25PM +0200, Christoph Hellwig wrote:
> @@ -3372,14 +3372,14 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
>  
>  	return VM_FAULT_SIGBUS;
>  
> +out_retry_put_folio:
> +	folio_put(folio);
>  out_retry:
>  	/*
>  	 * We dropped the mmap_lock, we need to return to the fault handler to
>  	 * re-find the vma and come back and find our hopefully still populated
>  	 * page.
>  	 */
> -	if (folio)
> -		folio_put(folio);

Why not simply:

-	if (folio)
+	if (!IS_ERR_OR_NULL(folio))

Re: [PATCH 1/2] filemap: fix the conditional folio_put in filemap_fault

[Christoph Hellwig]

On Wed, May 03, 2023 at 04:48:20PM +0100, Matthew Wilcox wrote:
> > -		folio_put(folio);
> 
> Why not simply:
> 
> -	if (folio)
> +	if (!IS_ERR_OR_NULL(folio))

no need for the OR_NULL.  But I find the extra label way easier to
reason about, and it's exactly the same amount of code.

Re: [PATCH 2/2] afs: fix the afs_dir_get_folio return value

[Jan Kara]

On Wed 03-05-23 17:45:26, Christoph Hellwig wrote:
> Keep returning NULL on failure instead of letting an ERR_PTR escape to
> callers that don't expect it.
> 
> Fixes: 66dabbb65d67 ("mm: return an ERR_PTR from __filemap_get_folio")
> Reported-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/afs/dir_edit.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/afs/dir_edit.c b/fs/afs/dir_edit.c
> index f0eddccbdd9541..e2fa577b66fe0a 100644
> --- a/fs/afs/dir_edit.c
> +++ b/fs/afs/dir_edit.c
> @@ -115,11 +115,12 @@ static struct folio *afs_dir_get_folio(struct afs_vnode *vnode, pgoff_t index)
>  	folio = __filemap_get_folio(mapping, index,
>  				    FGP_LOCK | FGP_ACCESSED | FGP_CREAT,
>  				    mapping->gfp_mask);
> -	if (IS_ERR(folio))
> +	if (IS_ERR(folio)) {
>  		clear_bit(AFS_VNODE_DIR_VALID, &vnode->flags);
> -	else if (folio && !folio_test_private(folio))
> +		return NULL;
> +	}
> +	if (!folio_test_private(folio))
>  		folio_attach_private(folio, (void *)1);
> -
>  	return folio;
>  }
>  
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/2] filemap: fix the conditional folio_put in filemap_fault

[Jan Kara]

On Wed 03-05-23 17:45:25, Christoph Hellwig wrote:
> folio can't be NULL here now that __filemap_get_folio returns an
> ERR_PTR.  Remove the conditional folio_put after the out_retry
> label and add a new label for the cases where we have a valid folio.
> 
> Fixes: 66dabbb65d67 ("mm: return an ERR_PTR from __filemap_get_folio")
> Reported-by: syzbot+48011b86c8ea329af1b9@syzkaller.appspotmail.com
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  mm/filemap.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index a34abfe8c65430..ae597f63a9bc54 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -3298,7 +3298,7 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
>  	}
>  
>  	if (!lock_folio_maybe_drop_mmap(vmf, folio, &fpin))
> -		goto out_retry;
> +		goto out_retry_put_folio;
>  
>  	/* Did it get truncated? */
>  	if (unlikely(folio->mapping != mapping)) {
> @@ -3334,7 +3334,7 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
>  	 */
>  	if (fpin) {
>  		folio_unlock(folio);
> -		goto out_retry;
> +		goto out_retry_put_folio;
>  	}
>  	if (mapping_locked)
>  		filemap_invalidate_unlock_shared(mapping);
> @@ -3363,7 +3363,7 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
>  	fpin = maybe_unlock_mmap_for_io(vmf, fpin);
>  	error = filemap_read_folio(file, mapping->a_ops->read_folio, folio);
>  	if (fpin)
> -		goto out_retry;
> +		goto out_retry_put_folio;
>  	folio_put(folio);
>  
>  	if (!error || error == AOP_TRUNCATED_PAGE)
> @@ -3372,14 +3372,14 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
>  
>  	return VM_FAULT_SIGBUS;
>  
> +out_retry_put_folio:
> +	folio_put(folio);
>  out_retry:
>  	/*
>  	 * We dropped the mmap_lock, we need to return to the fault handler to
>  	 * re-find the vma and come back and find our hopefully still populated
>  	 * page.
>  	 */
> -	if (folio)
> -		folio_put(folio);
>  	if (mapping_locked)
>  		filemap_invalidate_unlock_shared(mapping);
>  	if (fpin)
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/2] afs: fix the afs_dir_get_folio return value

[David Howells]

Christoph Hellwig <hch@lst.de> wrote:

> Keep returning NULL on failure instead of letting an ERR_PTR escape to
> callers that don't expect it.
> 
> Fixes: 66dabbb65d67 ("mm: return an ERR_PTR from __filemap_get_folio")
> Reported-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: David Howells <dhowells@redhat.com>
Tested-by: David Howells <dhowells@redhat.com>

Re: [PATCH 1/2] filemap: fix the conditional folio_put in filemap_fault

[Matthew Wilcox]

On Wed, May 03, 2023 at 05:49:36PM +0200, Christoph Hellwig wrote:
> On Wed, May 03, 2023 at 04:48:20PM +0100, Matthew Wilcox wrote:
> > > -		folio_put(folio);
> > 
> > Why not simply:
> > 
> > -	if (folio)
> > +	if (!IS_ERR_OR_NULL(folio))
> 
> no need for the OR_NULL.

Right.  I didn't read the whole function.

> But I find the extra label way easier to
> reason about, and it's exactly the same amount of code.

If it were easy to reason about, it would have a less ugly name.