| Message ID | 20180724121139.62570-2-kirill.shutemov@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix crash due to vma_is_anonymous() false-positives | expand |
On Tue, 24 Jul 2018 15:11:37 +0300 "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote: > Not all VMAs allocated with vm_area_alloc(). Some of them allocated on > stack or in data segment. > > The new helper can be use to initialize VMA properly regardless where > it was allocated. > > ... > > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -452,6 +452,12 @@ struct vm_operations_struct { > unsigned long addr); > }; > > +static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm) > +{ > + vma->vm_mm = mm; > + INIT_LIST_HEAD(&vma->anon_vma_chain); > +} > + > struct mmu_gather; > struct inode; > > diff --git a/kernel/fork.c b/kernel/fork.c > index a191c05e757d..1b27babc4c78 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -312,10 +312,8 @@ struct vm_area_struct *vm_area_alloc(struct mm_struct *mm) > { > struct vm_area_struct *vma = kmem_cache_zalloc(vm_area_cachep, GFP_KERNEL); I'd sleep better if this became a kmem_cache_alloc() and the memset was moved into vma_init(). A bunch of the vma_init() callers are already doing the memset and the others risk leaving uninitialized stack fields in the vma. > > - if (vma) { > - vma->vm_mm = mm; > - INIT_LIST_HEAD(&vma->anon_vma_chain); > - } > + if (vma) > + vma_init(vma, mm); > return vma; > }
On Tue, Jul 24, 2018 at 1:03 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > I'd sleep better if this became a kmem_cache_alloc() and the memset > was moved into vma_init(). Yeah, with the vma_init(), I guess the advantage of using kmem_cache_zalloc() is pretty dubious. Make it so. Linus
On Tue, 24 Jul 2018 13:16:33 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Tue, Jul 24, 2018 at 1:03 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > > > I'd sleep better if this became a kmem_cache_alloc() and the memset > > was moved into vma_init(). > > Yeah, with the vma_init(), I guess the advantage of using > kmem_cache_zalloc() is pretty dubious. > > Make it so. > Did I get everything? From: Andrew Morton <akpm@linux-foundation.org> Subject: mm: zero out the vma in vma_init() Rather than in vm_area_alloc(). To ensure that the various oddball stack-based vmas are in a good state. Some of the callers were zeroing them out, others were not. Cc: Russell King <rmk+kernel@arm.linux.org.uk> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> Cc: Dmitry Vyukov <dvyukov@google.com> Cc: Oleg Nesterov <oleg@redhat.com> Cc: Andrea Arcangeli <aarcange@redhat.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> --- arch/arm/kernel/process.c | 9 ++++----- fs/hugetlbfs/inode.c | 2 -- include/linux/mm.h | 1 + kernel/fork.c | 3 ++- mm/mempolicy.c | 1 - mm/shmem.c | 1 - 6 files changed, 7 insertions(+), 10 deletions(-) diff -puN arch/arm/kernel/process.c~mm-zero-out-the-vma-in-vma_init arch/arm/kernel/process.c --- a/arch/arm/kernel/process.c~mm-zero-out-the-vma-in-vma_init +++ a/arch/arm/kernel/process.c @@ -330,16 +330,15 @@ unsigned long arch_randomize_brk(struct * atomic helpers. Insert it into the gate_vma so that it is visible * through ptrace and /proc/<pid>/mem. */ -static struct vm_area_struct gate_vma = { - .vm_start = 0xffff0000, - .vm_end = 0xffff0000 + PAGE_SIZE, - .vm_flags = VM_READ | VM_EXEC | VM_MAYREAD | VM_MAYEXEC, -}; +static struct vm_area_struct gate_vma; static int __init gate_vma_init(void) { vma_init(&gate_vma, NULL); gate_vma.vm_page_prot = PAGE_READONLY_EXEC; + gate_vma.vm_start = 0xffff0000; + gate_vma.vm_end = 0xffff0000 + PAGE_SIZE; + gate_vma.vm_flags = VM_READ | VM_EXEC | VM_MAYREAD | VM_MAYEXEC; return 0; } arch_initcall(gate_vma_init); diff -puN fs/hugetlbfs/inode.c~mm-zero-out-the-vma-in-vma_init fs/hugetlbfs/inode.c --- a/fs/hugetlbfs/inode.c~mm-zero-out-the-vma-in-vma_init +++ a/fs/hugetlbfs/inode.c @@ -410,7 +410,6 @@ static void remove_inode_hugepages(struc int i, freed = 0; bool truncate_op = (lend == LLONG_MAX); - memset(&pseudo_vma, 0, sizeof(struct vm_area_struct)); vma_init(&pseudo_vma, current->mm); pseudo_vma.vm_flags = (VM_HUGETLB | VM_MAYSHARE | VM_SHARED); pagevec_init(&pvec); @@ -595,7 +594,6 @@ static long hugetlbfs_fallocate(struct f * allocation routines. If NUMA is configured, use page index * as input to create an allocation policy. */ - memset(&pseudo_vma, 0, sizeof(struct vm_area_struct)); vma_init(&pseudo_vma, mm); pseudo_vma.vm_flags = (VM_HUGETLB | VM_MAYSHARE | VM_SHARED); pseudo_vma.vm_file = file; diff -puN include/linux/mm.h~mm-zero-out-the-vma-in-vma_init include/linux/mm.h --- a/include/linux/mm.h~mm-zero-out-the-vma-in-vma_init +++ a/include/linux/mm.h @@ -456,6 +456,7 @@ static inline void vma_init(struct vm_ar { static const struct vm_operations_struct dummy_vm_ops = {}; + memset(vma, 0, sizeof(*vma)); vma->vm_mm = mm; vma->vm_ops = &dummy_vm_ops; INIT_LIST_HEAD(&vma->anon_vma_chain); diff -puN kernel/fork.c~mm-zero-out-the-vma-in-vma_init kernel/fork.c --- a/kernel/fork.c~mm-zero-out-the-vma-in-vma_init +++ a/kernel/fork.c @@ -310,8 +310,9 @@ static struct kmem_cache *mm_cachep; struct vm_area_struct *vm_area_alloc(struct mm_struct *mm) { - struct vm_area_struct *vma = kmem_cache_zalloc(vm_area_cachep, GFP_KERNEL); + struct vm_area_struct *vma; + vma = kmem_cache_alloc(vm_area_cachep, GFP_KERNEL); if (vma) vma_init(vma, mm); return vma; diff -puN mm/mempolicy.c~mm-zero-out-the-vma-in-vma_init mm/mempolicy.c --- a/mm/mempolicy.c~mm-zero-out-the-vma-in-vma_init +++ a/mm/mempolicy.c @@ -2504,7 +2504,6 @@ void mpol_shared_policy_init(struct shar goto put_new; /* Create pseudo-vma that contains just the policy */ - memset(&pvma, 0, sizeof(struct vm_area_struct)); vma_init(&pvma, NULL); pvma.vm_end = TASK_SIZE; /* policy covers entire file */ mpol_set_shared_policy(sp, &pvma, new); /* adds ref */ diff -puN mm/shmem.c~mm-zero-out-the-vma-in-vma_init mm/shmem.c --- a/mm/shmem.c~mm-zero-out-the-vma-in-vma_init +++ a/mm/shmem.c @@ -1421,7 +1421,6 @@ static void shmem_pseudo_vma_init(struct struct shmem_inode_info *info, pgoff_t index) { /* Create a pseudo vma that just contains the policy */ - memset(vma, 0, sizeof(*vma)); vma_init(vma, NULL); /* Bias interleave by inode number to distribute better across nodes */ vma->vm_pgoff = index + info->vfs_inode.i_ino;
On Tue, Jul 24, 2018 at 01:41:58PM -0700, Andrew Morton wrote: > On Tue, 24 Jul 2018 13:16:33 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote: > > > On Tue, Jul 24, 2018 at 1:03 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > > > > > > I'd sleep better if this became a kmem_cache_alloc() and the memset > > > was moved into vma_init(). > > > > Yeah, with the vma_init(), I guess the advantage of using > > kmem_cache_zalloc() is pretty dubious. > > > > Make it so. > > > > Did I get everything? There are few more: arch/arm64/include/asm/tlb.h: struct vm_area_struct vma = { .vm_mm = tlb->mm, }; arch/arm64/mm/hugetlbpage.c: struct vm_area_struct vma = { .vm_mm = mm }; arch/arm64/mm/hugetlbpage.c: struct vm_area_struct vma = { .vm_mm = mm };
On Wed, Jul 25, 2018 at 5:39 AM Kirill A. Shutemov <kirill@shutemov.name> wrote: > > There are few more: > > arch/arm64/include/asm/tlb.h: struct vm_area_struct vma = { .vm_mm = tlb->mm, }; > arch/arm64/mm/hugetlbpage.c: struct vm_area_struct vma = { .vm_mm = mm }; > arch/arm64/mm/hugetlbpage.c: struct vm_area_struct vma = { .vm_mm = mm }; We probably do not care. These are not "real" vma's and are never used as such. They are literally just fake vmas for the "flush_tlb()" machinery, which won't ever really cause any VM activity and will just call back to the architecture TLB flushing routines. They initialize vm_mm exactly because that's how the mm is passed down to the tlb flushing (we pass the whole vma because some architectures than have special flags in vm_flags too that can affect how the TLB gets flushed - ie "only flush ITLB if it's an execute-only vma" etc). Using "vma_init()" on them is only confusing, I think. Linus
On Wed, 25 Jul 2018 15:39:24 +0300 "Kirill A. Shutemov" <kirill@shutemov.name> wrote: > There are few more: > > arch/arm64/include/asm/tlb.h: struct vm_area_struct vma = { .vm_mm = tlb->mm, }; > arch/arm64/mm/hugetlbpage.c: struct vm_area_struct vma = { .vm_mm = mm }; > arch/arm64/mm/hugetlbpage.c: struct vm_area_struct vma = { .vm_mm = mm }; I'n not understanding. Your "mm: use vma_init() to initialize VMAs on stack and data segments" addressed all those? --- a/arch/arm64/include/asm/tlb.h~mm-use-vma_init-to-initialize-vmas-on-stack-and-data-segments +++ a/arch/arm64/include/asm/tlb.h @@ -37,7 +37,9 @@ static inline void __tlb_remove_table(vo static inline void tlb_flush(struct mmu_gather *tlb) { - struct vm_area_struct vma = { .vm_mm = tlb->mm, }; + struct vm_area_struct vma; + + vma_init(&vma, tlb->mm); /* * The ASID allocator will either invalidate the ASID or mark --- a/arch/arm64/mm/hugetlbpage.c~mm-use-vma_init-to-initialize-vmas-on-stack-and-data-segments +++ a/arch/arm64/mm/hugetlbpage.c @@ -108,11 +108,13 @@ static pte_t get_clear_flush(struct mm_s unsigned long pgsize, unsigned long ncontig) { - struct vm_area_struct vma = { .vm_mm = mm }; + struct vm_area_struct vma; pte_t orig_pte = huge_ptep_get(ptep); bool valid = pte_valid(orig_pte); unsigned long i, saddr = addr; + vma_init(&vma, mm); + for (i = 0; i < ncontig; i++, addr += pgsize, ptep++) { pte_t pte = ptep_get_and_clear(mm, addr, ptep); @@ -145,9 +147,10 @@ static void clear_flush(struct mm_struct unsigned long pgsize, unsigned long ncontig) { - struct vm_area_struct vma = { .vm_mm = mm }; + struct vm_area_struct vma; unsigned long i, saddr = addr; + vma_init(&vma, mm); for (i = 0; i < ncontig; i++, addr += pgsize, ptep++) pte_clear(mm, addr, ptep);
On Wed, Jul 25, 2018 at 07:42:01PM +0000, Andrew Morton wrote: > On Wed, 25 Jul 2018 15:39:24 +0300 "Kirill A. Shutemov" <kirill@shutemov.name> wrote: > > > There are few more: > > > > arch/arm64/include/asm/tlb.h: struct vm_area_struct vma = { .vm_mm = tlb->mm, }; > > arch/arm64/mm/hugetlbpage.c: struct vm_area_struct vma = { .vm_mm = mm }; > > arch/arm64/mm/hugetlbpage.c: struct vm_area_struct vma = { .vm_mm = mm }; > > I'n not understanding. Your "mm: use vma_init() to initialize VMAs on > stack and data segments" addressed all those? Yeah, sorry. Looked at wrong tree.
diff --git a/include/linux/mm.h b/include/linux/mm.h index d3a3842316b8..31540f166987 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -452,6 +452,12 @@ struct vm_operations_struct { unsigned long addr); }; +static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm) +{ + vma->vm_mm = mm; + INIT_LIST_HEAD(&vma->anon_vma_chain); +} + struct mmu_gather; struct inode; diff --git a/kernel/fork.c b/kernel/fork.c index a191c05e757d..1b27babc4c78 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -312,10 +312,8 @@ struct vm_area_struct *vm_area_alloc(struct mm_struct *mm) { struct vm_area_struct *vma = kmem_cache_zalloc(vm_area_cachep, GFP_KERNEL); - if (vma) { - vma->vm_mm = mm; - INIT_LIST_HEAD(&vma->anon_vma_chain); - } + if (vma) + vma_init(vma, mm); return vma; }
Not all VMAs allocated with vm_area_alloc(). Some of them allocated on stack or in data segment. The new helper can be use to initialize VMA properly regardless where it was allocated. Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> --- include/linux/mm.h | 6 ++++++ kernel/fork.c | 6 ++---- 2 files changed, 8 insertions(+), 4 deletions(-)