Re: [PATCH v3 3/3] mm: enforce the mapping_map_writable() check after call_mmap()

[Jan Kara <jack@suse.cz>]

On Sat 07-10-23 21:51:01, Lorenzo Stoakes wrote:
> In order for an F_SEAL_WRITE sealed memfd mapping to have an opportunity to
> clear VM_MAYWRITE in seal_check_write() we must be able to invoke either
> the shmem_mmap() or hugetlbfs_file_mmap() f_ops->mmap() handler to do so.
> 
> We would otherwise fail the mapping_map_writable() check before we had
> the opportunity to clear VM_MAYWRITE.
> 
> However, the existing logic in mmap_region() performs this check BEFORE
> calling call_mmap() (which invokes file->f_ops->mmap()). We must enforce
> this check AFTER the function call.
> 
> In order to avoid any risk of breaking call_mmap() handlers which assume
> this will have been done first, we continue to mark the file writable
> first, simply deferring enforcement of it failing until afterwards.
> 
> This enables mmap(..., PROT_READ, MAP_SHARED, fd, 0) mappings for memfd's
> sealed via F_SEAL_WRITE to succeed, whereas previously they were not
> permitted.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217238
> Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>

...

> diff --git a/mm/mmap.c b/mm/mmap.c
> index 6f6856b3267a..9fbee92aaaee 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2767,17 +2767,25 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>  	vma->vm_pgoff = pgoff;
>  
>  	if (file) {
> -		if (is_shared_maywrite(vm_flags)) {
> -			error = mapping_map_writable(file->f_mapping);
> -			if (error)
> -				goto free_vma;
> -		}
> +		int writable_error = 0;
> +
> +		if (vma_is_shared_maywrite(vma))
> +			writable_error = mapping_map_writable(file->f_mapping);
>  
>  		vma->vm_file = get_file(file);
>  		error = call_mmap(file, vma);
>  		if (error)
>  			goto unmap_and_free_vma;
>  
> +		/*
> +		 * call_mmap() may have changed VMA flags, so retry this check
> +		 * if it failed before.
> +		 */
> +		if (writable_error && vma_is_shared_maywrite(vma)) {
> +			error = writable_error;
> +			goto close_and_free_vma;
> +		}

Hum, this doesn't quite give me a peace of mind ;). One bug I can see is
that if call_mmap() drops the VM_MAYWRITE flag, we seem to forget to drop
i_mmap_writeable counter here?

I've checked why your v2 version broke i915 and I think the reason maybe
has nothing to do with i915. Just in case call_mmap() failed, it ended up
jumping to unmap_and_free_vma which calls mapping_unmap_writable() but we
didn't call mapping_map_writable() yet so the counter became imbalanced.

So I'd be for returning to v2 version, just fix up the error handling
paths...

								Honza


> +
>  		/*
>  		 * Expansion is handled above, merging is handled below.
>  		 * Drivers should not alter the address of the VMA.
> -- 
> 2.42.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR