permit write-sealed memfd read-only shared mappings
Subsystems (11)
v3 cover permit write-sealed memfd read-only shared mappings
The man page for fcntl() describing memfd file seals states the following about F_SEAL_WRITE:-
Furthermore, trying to create new shared, writable memory-mappings via mmap(2) will also fail with EPERM.
With emphasis on 'writable'. In turns out in fact that currently the kernel simply disallows all new shared memory mappings for a memfd with F_SEAL_WRITE applied, rendering this documentation inaccurate.
This matters because users are therefore unable to obtain a shared mapping to a memfd after write sealing altogether, which limits their usefulness. This was reported in the discussion thread [1] originating from a bug report [2].
This is a product of both using the struct address_space->i_mmap_writable atomic counter to determine whether writing may be permitted, and the kernel adjusting this counter when any VM_SHARED mapping is performed and more generally implicitly assuming VM_SHARED implies writable.
It seems sensible that we should only update this mapping if VM_MAYWRITE is specified, i.e. whether it is possible that this mapping could at any point be written to.
If we do so then all we need to do to permit write seals to function as documented is to clear VM_MAYWRITE when mapping read-only. It turns out this functionality already exists for F_SEAL_FUTURE_WRITE - we can therefore simply adapt this logic to do the same for F_SEAL_WRITE.
We then hit a chicken and egg situation in mmap_region() where the check for VM_MAYWRITE occurs before we are able to clear this flag. To work around this, separate the check and its enforcement across call_mmap() - allowing for this function to clear VM_MAYWRITE.
Thanks to Andy Lutomirski for the suggestion!
[1]:https://lore.kernel.org/all/[email protected]/ [2]:https://bugzilla.kernel.org/show_bug.cgi?id=217238
v3:
- Don't defer the writable check until after call_mmap() in case this breaks f_ops->mmap() callbacks which assume this has been done first. Instead, separate the check and enforcement of it across the call, allowing for it to change vma->vm_flags in the meanwhile.
- Improve/correct commit messages and comments throughout.
v2:
- Removed RFC tag.
- Correct incorrect goto pointed out by Jan.
- Reworded cover letter as suggested by Jan.
https://lore.kernel.org/all/[email protected]/
v1:
https://lore.kernel.org/all/[email protected]/
Lorenzo Stoakes (3):
mm: drop the assumption that VM_SHARED always implies writable
mm: update memfd seal write check to include F_SEAL_WRITE
mm: perform the mapping_map_writable() check after call_mmap()
fs/hugetlbfs/inode.c | 2 +- include/linux/fs.h | 4 ++-- include/linux/mm.h | 26 +++++++++++++++++++------- kernel/fork.c | 2 +- mm/filemap.c | 2 +- mm/madvise.c | 2 +- mm/mmap.c | 28 ++++++++++++++++++---------- mm/shmem.c | 2 +- 8 files changed, 44 insertions(+), 24 deletions(-)
Signature · 1 line
2.42.0
PATCH v3 1/3 mm: drop the assumption that VM_SHARED always implies writable
There is a general assumption that VMAs with the VM_SHARED flag set are writable. If the VM_MAYWRITE flag is not set, then this is simply not the case.
Update those checks which affect the struct address_space->i_mmap_writable field to explicitly test for this by introducing [vma_]is_shared_maywrite() helper functions.
This remains entirely conservative, as the lack of VM_MAYWRITE guarantees that the VMA cannot be written to.
--- include/linux/fs.h | 4 ++-- include/linux/mm.h | 11 +++++++++++ kernel/fork.c | 2 +- mm/filemap.c | 2 +- mm/madvise.c | 2 +- mm/mmap.c | 12 ++++++------ 6 files changed, 22 insertions(+), 11 deletions(-)
include/linux/fs.h +2 -2
@@ -454,7 +454,7 @@ extern const struct address_space_operations empty_aops;* It is also used to block modification of page cache contents through * memory mappings. * @gfp_mask: Memory allocation flags to use for allocating pages. -* @i_mmap_writable: Number of VM_SHARED mappings. +* @i_mmap_writable: Number of VM_SHARED, VM_MAYWRITE mappings. * @nr_thps: Number of THPs in the pagecache (non-shmem only). * @i_mmap: Tree of private and shared mappings. * @i_mmap_rwsem: Protects @i_mmap and @i_mmap_writable.
@@ -557,7 +557,7 @@ static inline int mapping_mapped(struct address_space *mapping)/* * Might pages of this file have been modified in userspace? -* Note that i_mmap_writable counts all VM_SHARED vmas: do_mmap +* Note that i_mmap_writable counts all VM_SHARED, VM_MAYWRITE vmas: do_mmap * marks vma as VM_SHARED if it is shared, and the file was opened for * writing i.e. vma may be mprotected writable even if now readonly. *
include/linux/mm.h +11 -0
@@ -937,6 +937,17 @@ static inline bool vma_is_accessible(struct vm_area_struct *vma)return vma->vm_flags &VM_ACCESS_FLAGS ; } +static inline bool is_shared_maywrite (vm_flags_t vm_flags) +{ +return (vm_flags & (VM_SHARED |VM_MAYWRITE )) == + (VM_SHARED |VM_MAYWRITE ); +} + +static inline bool vma_is_shared_maywrite (struct vm_area_struct *vma) +{ +return is_shared_maywrite (vma->vm_flags); +} +static inline struct vm_area_struct *vma_find (struct vma_iterator *vmi,unsigned long max) {
kernel/fork.c +1 -1
@@ -733,7 +733,7 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,get_file (file);i_mmap_lock_write (mapping); -if (tmp->vm_flags &VM_SHARED ) +if (vma_is_shared_maywrite (tmp))mapping_allow_writable (mapping);flush_dcache_mmap_lock (mapping);/* insert tmp into the share list, just after mpnt */
mm/filemap.c +1 -1
@@ -3637,7 +3637,7 @@ int generic_file_mmap(struct file *file, struct vm_area_struct *vma)*/ int generic_file_readonly_mmap (struct file *file,struct vm_area_struct *vma) { -if ((vma->vm_flags &VM_SHARED ) && (vma->vm_flags &VM_MAYWRITE )) +if (vma_is_shared_maywrite (vma))return -EINVAL ;return generic_file_mmap (file, vma); }
mm/madvise.c +1 -1
@@ -999,7 +999,7 @@ static long madvise_remove(struct vm_area_struct *vma,return -EINVAL ; } -if ((vma->vm_flags & (VM_SHARED |VM_WRITE )) != (VM_SHARED |VM_WRITE )) +if (!vma_is_shared_maywrite (vma))return -EACCES ; offset = (loff_t)(start - vma->vm_start)
mm/mmap.c +6 -6
@@ -107,7 +107,7 @@ void vma_set_page_prot(struct vm_area_struct *vma)static void __remove_shared_vm_struct (struct vm_area_struct *vma,struct file *file,struct address_space *mapping) { -if (vma->vm_flags &VM_SHARED ) +if (vma_is_shared_maywrite (vma))mapping_unmap_writable (mapping);flush_dcache_mmap_lock (mapping);
@@ -384,7 +384,7 @@ static unsigned long count_vma_pages_range(struct mm_struct *mm,static void __vma_link_file (struct vm_area_struct *vma,struct address_space *mapping) { -if (vma->vm_flags &VM_SHARED ) +if (vma_is_shared_maywrite (vma))mapping_allow_writable (mapping);flush_dcache_mmap_lock (mapping);
@@ -2767,7 +2767,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr, vma->vm_pgoff = pgoff;if (file) { -if (vm_flags &VM_SHARED ) { +if (is_shared_maywrite (vm_flags)) { error =mapping_map_writable (file->f_mapping);if (error) goto free_vma;
@@ -2842,7 +2842,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr, mm->map_count++;if (vma->vm_file) {i_mmap_lock_write (vma->vm_file->f_mapping); -if (vma->vm_flags &VM_SHARED ) +if (vma_is_shared_maywrite (vma))mapping_allow_writable (vma->vm_file->f_mapping);flush_dcache_mmap_lock (vma->vm_file->f_mapping);
@@ -2859,7 +2859,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,/* Once vma denies write, undo our temporary denial count */ unmap_writable: -if (file && vm_flags &VM_SHARED ) +if (file &&is_shared_maywrite (vm_flags))mapping_unmap_writable (file->f_mapping); file = vma->vm_file;ksm_add_vma (vma);
@@ -2907,7 +2907,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,unmap_region (mm, &vmi.mas, vma, prev, next, vma->vm_start, vma->vm_end, vma->vm_end, true); } -if (file && (vm_flags &VM_SHARED )) +if (file &&is_shared_maywrite (vm_flags))mapping_unmap_writable (file->f_mapping); free_vma:vm_area_free (vma);
Signature · 1 line
2.42.0
PATCH v3 2/3 mm: update memfd seal write check to include F_SEAL_WRITE
In reply to Lorenzo StoakesThe seal_check_future_write() function is called by shmem_mmap() or hugetlbfs_file_mmap() to disallow any future writable mappings of an memfd sealed this way.
The F_SEAL_WRITE flag is not checked here, as that is handled via the mapping->i_mmap_writable mechanism and so any attempt at a mapping would fail before this could be run.
However we intend to change this, meaning this check can be performed for F_SEAL_WRITE mappings also.
The logic here is equally applicable to both flags, so update this function to accommodate both and rename it accordingly.
--- fs/hugetlbfs/inode.c | 2 +- include/linux/mm.h | 15 ++++++++------- mm/shmem.c | 2 +- 3 files changed, 10 insertions(+), 9 deletions(-)
fs/hugetlbfs/inode.c +1 -1
@@ -112,7 +112,7 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)vm_flags_set (vma,VM_HUGETLB |VM_DONTEXPAND ); vma->vm_ops = &hugetlb_vm_ops; - ret =seal_check_future_write (info->seals, vma); + ret =seal_check_write (info->seals, vma);if (ret)return ret;
include/linux/mm.h +8 -7
@@ -4027,25 +4027,26 @@ static inline void mem_dump_obj(void *object) {}#endif /** -* seal_check_future_write - Check for F_SEAL_FUTURE_WRITE flag and handle it +* seal_check_write - Check for F_SEAL_WRITE or F_SEAL_FUTURE_WRITE flags and +* handle them. * @seals: the seals to check * @vma: the vma to operate on * -* Check whether F_SEAL_FUTURE_WRITE is set; if so, do proper check/handling on -* the vma flags. Return 0 if check pass, or <0 for errors. +* Check whether F_SEAL_WRITE or F_SEAL_FUTURE_WRITE are set; if so, do proper +* check/handling on the vma flags. Return 0 if check pass, or <0 for errors. */ -static inline int seal_check_future_write (int seals,struct vm_area_struct *vma) +static inline int seal_check_write (int seals,struct vm_area_struct *vma) { -if (seals &F_SEAL_FUTURE_WRITE ) { +if (seals & (F_SEAL_WRITE |F_SEAL_FUTURE_WRITE )) {/* * New PROT_WRITE and MAP_SHARED mmaps are not allowed when -* "future write" seal active. +* write seals are active. */ if ((vma->vm_flags &VM_SHARED ) && (vma->vm_flags &VM_WRITE ))return -EPERM ;/* -* Since an F_SEAL_FUTURE_WRITE sealed memfd can be mapped as +* Since an F_SEAL_[FUTURE_]WRITE sealed memfd can be mapped as * MAP_SHARED and read-only, take care to not allow mprotect to * revert protections on such mappings. Do this only for shared * mappings. For private mappings, don't need to mask
mm/shmem.c +1 -1
@@ -2405,7 +2405,7 @@ static int shmem_mmap(struct file *file, struct vm_area_struct *vma)struct shmem_inode_info *info =SHMEM_I (inode);int ret; - ret =seal_check_future_write (info->seals, vma); + ret =seal_check_write (info->seals, vma);if (ret)return ret;
Signature · 1 line
2.42.0
PATCH v3 3/3 mm: enforce the mapping_map_writable() check after call_mmap()
In reply to Lorenzo StoakesIn 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.
--- mm/mmap.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)
mm/mmap.c +13 -5
@@ -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; + } +/* * Expansion is handled above, merging is handled below. * Drivers should not alter the address of the VMA.
Signature · 1 line
2.42.0
Jan Kara
maintainerRe: [PATCH v3 3/3] mm: enforce the mapping_map_writable() check after call_mmap()
Quoted 21 lines · Lorenzo Stoakes
In order for an
F_SEAL_WRITEsealed memfd mapping to have an opportunity to clearVM_MAYWRITEinseal_check_write()we must be able to invoke either theshmem_mmap()orhugetlbfs_file_mmap()f_ops->mmap()handler to do so.We would otherwise fail the
mapping_map_writable()check before we had the opportunity to clearVM_MAYWRITE.However, the existing logic in
mmap_region()performs this check BEFORE callingcall_mmap()(which invokesfile->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 viaF_SEAL_WRITEto succeed, whereas previously they were not permitted.
...
Quoted 21 lines
mm/mmap.c +4
-5@@ -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)
mm/mmap.c unsigned long mmap_region(struct file *file, unsigned long addr, · line 2787
Quoted 10 lines
mm/mmap.c +8
-0goto 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
Quoted 7 lines
mm/mmap.c +1
-0+/* * Expansion is handled above, merging is handled below. * Drivers should not alter the address of the VMA. Signature · 1 line
2.42.0
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH v3 3/3] mm: enforce the mapping_map_writable() check after call_mmap()
Quoted 48 lines · Jan Kara
Quoted 21 lines · Lorenzo Stoakes
In order for an
F_SEAL_WRITEsealed memfd mapping to have an opportunity to clearVM_MAYWRITEinseal_check_write()we must be able to invoke either theshmem_mmap()orhugetlbfs_file_mmap()f_ops->mmap()handler to do so.We would otherwise fail the
mapping_map_writable()check before we had the opportunity to clearVM_MAYWRITE.However, the existing logic in
mmap_region()performs this check BEFORE callingcall_mmap()(which invokesfile->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 viaF_SEAL_WRITEto succeed, whereas previously they were not permitted....
Quoted 23 lines
mm/mmap.c +4
-5@@ -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;
Quoted 12 lines
mm/mmap.c unsigned long mmap_region(struct file *file, unsigned long addr, · line 2787
Quoted 8 lines
mm/mmap.c +8
-0+/* +* 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 theVM_MAYWRITEflag, we seem to forget to dropi_mmap_writeablecounter here?
This wouldn't be applicable in the F_SEAL_WRITE case, as the i_mmap_writable counter would already have been decremented, and thus an error would arise causing no further decrement, and everything would work fine.
It'd be very odd for something to be writable here but the driver to make it not writable. But we do need to account for this.
Quoted 5 lines
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 tounmap_and_free_vmawhich callsmapping_unmap_writable()but we didn't callmapping_map_writable()yet so the counter became imbalanced.
yeah that must be the cause, I thought perhaps somehow __remove_shared_vm_struct() got invoked by i915_gem_mmap() but I didn't trace it through to see if it was possible.
Looking at it again, i don't think that is possible, as we hold a mmap/vma write lock, and the only operations that can cause __remove_shared_vm_struct() to run are things that would not be able to do so with this lock held.
Quoted 3 lines
So I'd be for returning to v2 version, just fix up the error handling paths...
So in conclusion, I agree, this is the better approach. Will respin in v4.
Quoted 14 lines
Honza
Quoted 7 lines
mm/mmap.c +1
-1+/* * Expansion is handled above, merging is handled below. * Drivers should not alter the address of the VMA. -- 2.42.0
--
Jan Kara <[email protected]>
SUSE Labs, CR
Jan Kara
maintainerRe: [PATCH v3 3/3] mm: enforce the mapping_map_writable() check after call_mmap()
Quoted 62 lines · Lorenzo Stoakes
Quoted 48 lines · Jan Kara
Quoted 21 lines · Lorenzo Stoakes
In order for an
F_SEAL_WRITEsealed memfd mapping to have an opportunity to clearVM_MAYWRITEinseal_check_write()we must be able to invoke either theshmem_mmap()orhugetlbfs_file_mmap()f_ops->mmap()handler to do so.We would otherwise fail the
mapping_map_writable()check before we had the opportunity to clearVM_MAYWRITE.However, the existing logic in
mmap_region()performs this check BEFORE callingcall_mmap()(which invokesfile->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 viaF_SEAL_WRITEto succeed, whereas previously they were not permitted....
Quoted 23 lines
mm/mmap.c +4
-5@@ -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;Quoted 12 lines
mm/mmap.c unsigned long mmap_region(struct file *file, unsigned long addr, · line 2787
Quoted 8 lines
mm/mmap.c +8
-0+/* +* 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 theVM_MAYWRITEflag, we seem to forget to dropi_mmap_writeablecounter here?
Quoted 7 lines
This wouldn't be applicable in the
F_SEAL_WRITEcase, as thei_mmap_writablecounter would already have been decremented, and thus an error would arise causing no further decrement, and everything would work fine.It'd be very odd for something to be writable here but the driver to make it not writable. But we do need to account for this.
Yeah, it may be odd but this is indeed what i915 driver appears to be doing in i915_gem_object_mmap():
if (i915_gem_object_is_readonly(obj)) {
if (vma->vm_flags & VM_WRITE) {
i915_gem_object_put(obj);
return -EINVAL;
}
vm_flags_clear(vma, VM_MAYWRITE);
}Quoted 14 lines
Quoted 4 lines
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 tounmap_and_free_vmawhich callsmapping_unmap_writable()but we didn't callmapping_map_writable()yet so the counter became imbalanced.yeah that must be the cause, I thought perhaps somehow
__remove_shared_vm_struct()got invoked byi915_gem_mmap()but I didn't trace it through to see if it was possible.Looking at it again, i don't think that is possible, as we hold a mmap/vma write lock, and the only operations that can cause
__remove_shared_vm_struct()to run are things that would not be able to do so with this lock held.
Quoted 4 lines
Quoted 2 lines
So I'd be for returning to v2 version, just fix up the error handling paths...
So in conclusion, I agree, this is the better approach. Will respin in v4.
Thanks!
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR