The map->file_doesnt_need_get flag is confusing and the existing implementation has holes.
Drivers are permitted to change the owning file of a mapping. If they do so, they are required to take a reference on that file. The mmap() operation which ultimately invokes __mmap_region() is guaranteed to drop the refcount for the original file the mapping was made under, but this is not true for the replaced file. This has been addressed so far by tracking map->file_doesnt_need_get, which is rather poorly named and unfortunately fails to correctly track whether or not an additional put were needed in a number of cases. Make life easier by removing this flag, and instead drop the reference for both mmap_prepare and the deprecated mmap callback in a new function put_map(). Track whether this needs to be done by aligning mmap_state with vm_area_desc and store the original file in the map->file field, keeping the updated file in map->vm_file. In order to have the same behaviour for both types of hooks, only drop the reference __mmap_new_file_vma() itself took in its error path, deferring the replaced file's reference to put_map(). To make this work correctly, map->vm_file has to be updated before any error handling, so update __mmap_new_file_vma() and call_mmap_prepare() to set this field first. Also when mmap_prepare() changes the file and is then merged, the reference count also must be decremented, so update the logic to call put_map() in this case too. Also update __compat_vma_mmap() to manually perform this step for stacked file systems using the compatibility layer, and update compat_set_vma_from_desc() to replace vma_set_file() with a correct refcount/file update. No in-tree driver is impacted by the incorrect implementation of this currently (no driver that does this is mergeable for one), so this does not need to be a fix. Signed-off-by: Lorenzo Stoakes (ARM) <[email protected]> --- mm/internal.h | 1 + mm/util.c | 5 +++- mm/vma.c | 95 ++++++++++++++++++++++++++++++++++------------------------- mm/vma.h | 6 ++-- 4 files changed, 64 insertions(+), 43 deletions(-) diff --git a/mm/internal.h b/mm/internal.h index 0dca33db068f..fe576d468af4 100644 --- a/mm/internal.h +++ b/mm/internal.h @@ -7,6 +7,7 @@ #ifndef __MM_INTERNAL_H #define __MM_INTERNAL_H +#include <linux/file.h> #include <linux/fs.h> #include <linux/khugepaged.h> #include <linux/mm.h> diff --git a/mm/util.c b/mm/util.c index bf0513d1d3d0..016932780925 100644 --- a/mm/util.c +++ b/mm/util.c @@ -1228,8 +1228,11 @@ int __compat_vma_mmap(struct vm_area_desc *desc, /* Perform any preparatory tasks for mmap action. */ err = mmap_action_prepare(desc); - if (err) + if (err) { + if (desc->vm_file != vma->vm_file) + fput(desc->vm_file); return err; + } /* Update the VMA from the descriptor. */ compat_set_vma_from_desc(vma, desc); /* Complete any specified mmap actions. */ diff --git a/mm/vma.c b/mm/vma.c index 55917d097933..a319a9fc2f29 100644 --- a/mm/vma.c +++ b/mm/vma.c @@ -24,7 +24,8 @@ struct mmap_state { vm_flags_t vm_flags; vma_flags_t vma_flags; }; - struct file *file; + struct file *file; /* mmap()-specified file. */ + struct file *vm_file; /* May be updated by mmap_prepare. */ pgprot_t page_prot; /* User-defined fields, perhaps updated by .mmap_prepare(). */ @@ -43,8 +44,6 @@ struct mmap_state { /* Determine if we can check KSM flags early in mmap() logic. */ bool check_ksm_early :1; - /* If .mmap_prepare changed the file, we don't need to pin. */ - bool file_doesnt_need_get :1; }; #define MMAP_STATE(name, mm_, vmi_, addr_, len_, pgoff_, anon_pgoff_, vma_flags_, file_) \ @@ -58,6 +57,7 @@ struct mmap_state { .pglen = PHYS_PFN(len_), \ .vma_flags = vma_flags_, \ .file = file_, \ + .vm_file = file_, \ .page_prot = vma_flags_to_page_prot(vma_flags_), \ } @@ -70,7 +70,7 @@ struct mmap_state { .vma_flags = (map_)->vma_flags, \ .pgoff = (map_)->pgoff, \ .anon_pgoff = (map_)->anon_pgoff, \ - .file = (map_)->file, \ + .file = (map_)->vm_file, \ .prev = (map_)->prev, \ .middle = vma_, \ .next = (vma_) ? NULL : (map_)->next, \ @@ -2447,7 +2447,7 @@ void mm_drop_all_locks(struct mm_struct *mm) */ static bool accountable_mapping(struct mmap_state *map) { - const struct file *file = map->file; + const struct file *file = map->vm_file; /* * hugetlb has its own accounting separate from the core VM @@ -2496,7 +2496,7 @@ static void vms_abort_munmap_vmas(struct vma_munmap_struct *vms, static void update_ksm_flags(struct mmap_state *map) { - map->vma_flags = ksm_vma_flags(map->mm, map->file, map->vma_flags); + map->vma_flags = ksm_vma_flags(map->mm, map->vm_file, map->vma_flags); } static void set_desc_from_map(struct vm_area_desc *desc, @@ -2506,7 +2506,7 @@ static void set_desc_from_map(struct vm_area_desc *desc, desc->end = map->end; desc->pgoff = map->pgoff; - desc->vm_file = map->file; + desc->vm_file = map->vm_file; desc->vma_flags = map->vma_flags; desc->page_prot = map->page_prot; } @@ -2586,6 +2586,10 @@ static int __mmap_setup(struct mmap_state *map, struct vm_area_desc *desc, return 0; } +static bool map_same_file(struct mmap_state *map) +{ + return map->vm_file == map->file; +} static int __mmap_new_file_vma(struct mmap_state *map, struct vm_area_struct *vma) @@ -2593,20 +2597,23 @@ static int __mmap_new_file_vma(struct mmap_state *map, struct vma_iterator *vmi = map->vmi; int error; - vma->vm_file = map->file; - if (!map->file_doesnt_need_get) - get_file(map->file); + vma->vm_file = map->vm_file; + if (map_same_file(map)) + get_file(map->vm_file); - if (!map->file->f_op->mmap) + if (!map->vm_file->f_op->mmap) return 0; error = mmap_file(vma->vm_file, vma); + map->vm_file = vma->vm_file; + if (error) { UNMAP_STATE(unmap, vmi, vma, vma->vm_start, vma->vm_end, map->prev, map->next); - fput(vma->vm_file); - vma->vm_file = NULL; + if (map_same_file(map)) + fput(map->vm_file); + vma->vm_file = NULL; vma_iter_set(vmi, vma->vm_end); /* Undo any partial mapping done by a device driver. */ unmap_region(&unmap); @@ -2623,7 +2630,6 @@ static int __mmap_new_file_vma(struct mmap_state *map, !vma_flags_test(&map->vma_flags, VMA_MAYWRITE_BIT) && vma_test(vma, VMA_MAYWRITE_BIT)); - map->file = vma->vm_file; map->vma_flags = vma->flags; return 0; @@ -2631,7 +2637,7 @@ static int __mmap_new_file_vma(struct mmap_state *map, static void map_set_anon(struct mmap_state *map) { - map->file = NULL; + map->vm_file = NULL; map->vm_ops = NULL; map->pgoff = map->addr >> PAGE_SHIFT; } @@ -2643,7 +2649,7 @@ static bool map_is_private(const struct mmap_state *map) static bool map_is_anon(const struct mmap_state *map) { - return map_is_private(map) && !map->file; + return map_is_private(map) && !map->vm_file; } /* @@ -2688,7 +2694,7 @@ static int __mmap_new_vma(struct mmap_state *map, struct vm_area_struct **vmap, } /* Invoke callbacks. */ - if (map->file) + if (map->vm_file) error = __mmap_new_file_vma(map, vma); else if (!is_anon) error = shmem_zero_setup(vma); @@ -2797,36 +2803,34 @@ static int call_mmap_prepare(struct mmap_state *map, int err; /* Invoke the hook. */ - err = vfs_mmap_prepare(map->file, desc); - if (err) - return err; - - /* It's invalid for mmap_preprare hooks to clear vm_ops. */ - if (!desc->vm_ops) - return -EINVAL; - - err = call_action_prepare(map, desc); + err = vfs_mmap_prepare(map->vm_file, desc); if (err) return err; /* Update fields permitted to be changed. */ map->pgoff = desc->pgoff; - if (desc->vm_file != map->file) { - map->file_doesnt_need_get = true; - map->file = desc->vm_file; - } + if (desc->vm_file != map->vm_file) + map->vm_file = desc->vm_file; map->vma_flags = desc->vma_flags; map->page_prot = desc->page_prot; /* User-defined fields. */ map->vm_ops = desc->vm_ops; map->vm_private_data = desc->private_data; + /* It's invalid for mmap_prepare hooks to clear vm_ops. */ + if (!desc->vm_ops) + return -EINVAL; + + err = call_action_prepare(map, desc); + if (err) + return err; + /* * MAP_PRIVATE-/dev/zero mappings are an ancient way of getting * anonymous mappings. Rather than allowing these mappings to be odd * outliers, simply make them truly anonymous. */ - if (map_is_private(map) && file_is_dev_zero(map->file)) + if (map_is_private(map) && file_is_dev_zero(map->vm_file)) map_set_anon(map); return 0; @@ -2845,7 +2849,7 @@ static void set_vma_user_defined_fields(struct vm_area_struct *vma, */ static bool can_set_ksm_flags_early(struct mmap_state *map) { - struct file *file = map->file; + struct file *file = map->vm_file; /* Anonymous mappings have no driver which can change them. */ if (!file) @@ -2868,6 +2872,20 @@ static bool can_set_ksm_flags_early(struct mmap_state *map) return false; } +static void put_map(struct mmap_state *map) +{ + /* + * An error occurred or the VMA was merged. + * + * If the file was changed by the driver (which is required to increment + * the replacement file's reference count), drop its reference count. + * + * On error, the caller always drops the original file regardless. + */ + if (map->vm_file && !map_same_file(map)) + fput(map->vm_file); +} + static unsigned long __mmap_region(struct file *file, unsigned long addr, unsigned long len, vma_flags_t vma_flags, unsigned long pgoff, struct list_head *uf) @@ -2922,7 +2940,10 @@ static unsigned long __mmap_region(struct file *file, unsigned long addr, __mmap_complete(&map, vma); - if (have_mmap_prepare && allocated_new) { + if (!allocated_new) { + /* Merged, so need to drop refcount. */ + put_map(&map); + } else if (have_mmap_prepare) { error = mmap_action_complete(vma, &desc.action, /*is_compat=*/false); if (error) @@ -2936,13 +2957,7 @@ static unsigned long __mmap_region(struct file *file, unsigned long addr, if (map.charged) vm_unacct_memory(map.charged); abort_munmap: - /* - * This indicates that .mmap_prepare has set a new file, differing from - * desc->vm_file. But since we're aborting the operation, only the - * original file will be cleaned up. Ensure we clean up both. - */ - if (map.file_doesnt_need_get) - fput(map.file); + put_map(&map); vms_abort_munmap_vmas(&map.vms, &map.mas_detach); return error; } diff --git a/mm/vma.h b/mm/vma.h index e97bd2dfa786..f15faa83f3d6 100644 --- a/mm/vma.h +++ b/mm/vma.h @@ -394,8 +394,10 @@ static inline void compat_set_vma_from_desc(struct vm_area_struct *vma, /* Mutable fields. Populated with initial state. */ vma_set_pgoff(vma, desc->pgoff); - if (desc->vm_file != vma->vm_file) - vma_set_file(vma, desc->vm_file); + if (desc->vm_file != vma->vm_file) { + fput(vma->vm_file); + vma->vm_file = desc->vm_file; + } vma->flags = desc->vma_flags; vma->vm_page_prot = desc->page_prot; -- 2.55.0
