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@xxxxxxxxx> ... > 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@xxxxxxxx> SUSE Labs, CR
