Re: [PATCH 08/46] mm/filemap: finish fault cleanup before dropping the fault lock

[Thadeu Lima de Souza Cascardo]

On Tue, Oct 27, 2026 at 12:17:28AM +0100, David Hildenbrand (Arm) wrote:
> On a retried fault, a fatal signal interrupting the folio lock wait
> makes lock_folio_maybe_drop_mmap() drop the fault lock without pinning
> the file. However, filemap_fault() may still need to release
> mapping->invalidate_lock. A concurrent munmap() can release the last
> file reference and free the mapping before that unlock.
> 
> Defer dropping the fault lock to filemap_fault()'s retry cleanup, after
> releasing invalidate_lock. If the fault lock was already dropped for
> I/O, release the existing file reference instead. Keep the fault lock
> held for FAULT_FLAG_RETRY_NOWAIT as before.
> 
> Fixes: 730633f0b7f9 ("mm: Protect operations adding pages to page cache with invalidate_lock")
> Cc: stable@xxxxxxxxxxxxxxx
> Reported-by: Chris Mason <mason@xxxxxxxxxx>
> Signed-off-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
> ---
>  mm/filemap.c | 37 +++++++++++++------------------------
>  1 file changed, 13 insertions(+), 24 deletions(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 6b5e1d866a9e0..b04be64a5069f 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -3308,11 +3308,11 @@ loff_t mapping_seek_hole_data(struct address_space *mapping, loff_t start,
>   * @folio - the folio to lock.
>   * @fpin - the pointer to the file we may pin (or is already pinned).
>   *
> - * This works similar to lock_folio_or_retry in that it can drop the
> - * mmap_lock.  It differs in that it actually returns the folio locked
> - * if it returns 1 and 0 if it couldn't lock the folio.  If we did have
> - * to drop the mmap_lock then fpin will point to the pinned file and
> - * needs to be fput()'ed at a later point.
> + * Returns 1 with the folio locked, or 0 if locking failed.
> + *
> + * On both entry and return, a non-NULL *fpin means the fault lock has
> + * already been dropped and the caller must eventually fput() the file.
> + * Otherwise the fault lock is still held, including on failure.
>   */
>  static int lock_folio_maybe_drop_mmap(struct vm_fault *vmf, struct folio *folio,
>  				     struct file **fpin)
> @@ -3329,21 +3329,9 @@ static int lock_folio_maybe_drop_mmap(struct vm_fault *vmf, struct folio *folio,
>  		return 0;
>  
>  	*fpin = maybe_unlock_mmap_for_io(vmf, *fpin);
> -	if (vmf->flags & FAULT_FLAG_KILLABLE) {
> -		if (__folio_lock_killable(folio)) {
> -			/*
> -			 * We didn't have the right flags to drop the
> -			 * fault lock, but all fault_handlers only check
> -			 * for fatal signals if we return VM_FAULT_RETRY,
> -			 * so we need to drop the fault lock here and
> -			 * return 0 if we don't have a fpin.
> -			 */
> -			if (*fpin == NULL)
> -				release_fault_lock(vmf);
> -			return 0;
> -		}
> -	} else
> -		__folio_lock(folio);
> +	if (vmf->flags & FAULT_FLAG_KILLABLE)
> +		return !__folio_lock_killable(folio);
> +	__folio_lock(folio);
>  
>  	return 1;
>  }
> @@ -3572,7 +3560,7 @@ static vm_fault_t filemap_fault_recheck_pte_none(struct vm_fault *vmf)
>   * vma->vm_mm->mmap_lock must be held on entry.
>   *
>   * If our return value has VM_FAULT_RETRY set, it's because the mmap_lock
> - * may be dropped before doing I/O or by lock_folio_maybe_drop_mmap().
> + * may be dropped before doing I/O or when aborting an interrupted wait.
>   *
>   * If our return value does not have VM_FAULT_RETRY set, the mmap_lock
>   * has not been released.
> @@ -3728,9 +3716,8 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
>  
>  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.
> +	 * With fpin set, the fault lock was already dropped. Otherwise it is
> +	 * still held. Release invalidate_lock before dropping either protection.
>  	 */
>  	if (!IS_ERR(folio))
>  		folio_put(folio);
> @@ -3738,6 +3725,8 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
>  		filemap_invalidate_unlock_shared(mapping);
>  	if (fpin)
>  		fput(fpin);
> +	else if (!(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))
> +		release_fault_lock(vmf);
>  	return ret | VM_FAULT_RETRY;
>  }
>  EXPORT_SYMBOL(filemap_fault);
> 
> -- 
> 2.43.0
> 
> 

If I understood this correctly, the invariant is that if fpin is set, then
the fault lock has been dropped.

At first, I was confused that maybe_unlock_mmap_for_io drops the fault
lock, but in that case, it also gets a reference to the file, so fpin is
set and the invariant is kept.

With that:

Reviewed-by: Thadeu Lima de Souza Cascardo <cascardo@xxxxxxxxxx>