On Tue, 10 Jul 2018 16:48:20 +0300 "Kirill A. Shutemov" kirill.shutemov@linux.intel.com wrote:
vma_is_anonymous() relies on ->vm_ops being NULL to detect anonymous VMA. This is unreliable as ->mmap may not set ->vm_ops.
False-positive vma_is_anonymous() may lead to crashes:
...
This can be fixed by assigning anonymous VMAs own vm_ops and not relying on it being NULL.
If ->mmap() failed to set ->vm_ops, mmap_region() will set it to dummy_vm_ops. This way we will have non-NULL ->vm_ops for all VMAs.
Is there a smaller, simpler fix which we can use for backporting purposes and save the larger rework for development kernels?
...
--- a/mm/mmap.c +++ b/mm/mmap.c @@ -71,6 +71,9 @@ int mmap_rnd_compat_bits __read_mostly = CONFIG_ARCH_MMAP_RND_COMPAT_BITS; static bool ignore_rlimit_data; core_param(ignore_rlimit_data, ignore_rlimit_data, bool, 0644); +const struct vm_operations_struct anon_vm_ops = {}; +const struct vm_operations_struct dummy_vm_ops = {};
Some nice comments here would be useful. Especially for dummy_vm_ops. Why does it exist, what is its role, etc.
static void unmap_region(struct mm_struct *mm, struct vm_area_struct *vma, struct vm_area_struct *prev, unsigned long start, unsigned long end); @@ -561,6 +564,8 @@ static unsigned long count_vma_pages_range(struct mm_struct *mm, void __vma_link_rb(struct mm_struct *mm, struct vm_area_struct *vma, struct rb_node **rb_link, struct rb_node *rb_parent) {
- WARN_ONCE(!vma->vm_ops, "missing vma->vm_ops");
- /* Update tracking information for the gap following the new vma. */ if (vma->vm_next) vma_gap_update(vma->vm_next);
@@ -1774,12 +1779,19 @@ unsigned long mmap_region(struct file *file, unsigned long addr, */ WARN_ON_ONCE(addr != vma->vm_start);
/* All mappings must have ->vm_ops set */if (!vma->vm_ops)vma->vm_ops = &dummy_vm_ops;
Can this happen? Can we make it a rule that file_operations.mmap(vma) must initialize vma->vm_ops? Should we have a WARN here to detect when the fs implementation failed to do that?
addr = vma->vm_start; vm_flags = vma->vm_flags;} else if (vm_flags & VM_SHARED) { error = shmem_zero_setup(vma); if (error) goto free_vma;
- } else {
/* vma_is_anonymous() relies on this. */vma->vm_ops = &anon_vm_ops;vma_link(mm, vma, prev, rb_link, rb_parent); ...