| Message ID | 20230503154526.1223095-1-hch@lst.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] filemap: fix the conditional folio_put in filemap_fault | expand |
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))
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.
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 >
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.
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)
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(-)