[PATCH] fs/proc: smaps should avoid VM_PFNMAP areas

[PATCH] fs/proc: smaps should avoid VM_PFNMAP areas

[Cliff Wickman]

/proc/<pid>/smaps should not be looking at VM_PFNMAP areas.

Certain tests in show_smap() (especially for huge pages) assume that the
mapped PFN's are backed with page structures.  And this is not usually true
for VM_PFNMAP areas.  This can result in panics on kernel page faults when
attempting to address those page structures.

VM_PFNMAP areas are used by
- graphics memory manager   gpu/drm/drm_gem.c
- global reference unit     sgi-gru/grufile.c
- sgi special memory        char/mspec.c
- probably several out-of-tree modules

I'm copying everyone who has changed fs/proc/task_mmu.c recently, in case
of some reason to provide /proc/<pid>/smaps for these areas that I am not
aware of.

Signed-off-by: Cliff Wickman <cpw@sgi.com>
---
 fs/proc/task_mmu.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux/fs/proc/task_mmu.c
===================================================================
--- linux.orig/fs/proc/task_mmu.c
+++ linux/fs/proc/task_mmu.c
@@ -589,6 +589,9 @@ static int show_smap(struct seq_file *m,
 		.private = &mss,
 	};
 
+	if (vma->vm_flags & VM_PFNMAP)
+		return 0;
+
 	memset(&mss, 0, sizeof mss);
 	mss.vma = vma;
 	/* mmap_sem is held in m_start */

Re: [PATCH] fs/proc: smaps should avoid VM_PFNMAP areas

[Naoya Horiguchi]

Hello Cliff,

On Mon, Apr 29, 2013 at 04:25:53PM -0500, Cliff Wickman wrote:
> 
> /proc/<pid>/smaps should not be looking at VM_PFNMAP areas.
> 
> Certain tests in show_smap() (especially for huge pages) assume that the
> mapped PFN's are backed with page structures.  And this is not usually true
> for VM_PFNMAP areas.  This can result in panics on kernel page faults when
> attempting to address those page structures.

I think it's strange that you mention to hugepages, because in my understanding
VM_PFNMAP and hugepage related vma (VM_HUGEPAGE or VM_HUGETLB) should not set
at the same time. In what testcase are these flags both set?

And I guess this race can also happen on reading pagemap or numa_maps because
walk_page_range() is called in those code paths. Are you sure the race doesn't
happen on these paths? If not, please add a few more flag checks for them.

Thanks,
Naoya Horiguchi

> VM_PFNMAP areas are used by
> - graphics memory manager   gpu/drm/drm_gem.c
> - global reference unit     sgi-gru/grufile.c
> - sgi special memory        char/mspec.c
> - probably several out-of-tree modules
> 
> I'm copying everyone who has changed fs/proc/task_mmu.c recently, in case
> of some reason to provide /proc/<pid>/smaps for these areas that I am not
> aware of.
> 
> Signed-off-by: Cliff Wickman <cpw@sgi.com>
> ---
>  fs/proc/task_mmu.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> Index: linux/fs/proc/task_mmu.c
> ===================================================================
> --- linux.orig/fs/proc/task_mmu.c
> +++ linux/fs/proc/task_mmu.c
> @@ -589,6 +589,9 @@ static int show_smap(struct seq_file *m,
>  		.private = &mss,
>  	};
>  
> +	if (vma->vm_flags & VM_PFNMAP)
> +		return 0;
> +
>  	memset(&mss, 0, sizeof mss);
>  	mss.vma = vma;
>  	/* mmap_sem is held in m_start */
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] fs/proc: smaps should avoid VM_PFNMAP areas

[Cliff Wickman]

> On Tue, Apr 30, 2013 at 01:11:45PM -0500, Naoya Horiguchi wrote:
> > 
> > /proc/<pid>/smaps should not be looking at VM_PFNMAP areas.
> > 
> > Certain tests in show_smap() (especially for huge pages) assume that the
> > mapped PFN's are backed with page structures.  And this is not usually true
> > for VM_PFNMAP areas.  This can result in panics on kernel page faults when
> > attempting to address those page structures.
> 
> I think it's strange that you mention to hugepages, because in my understanding
> VM_PFNMAP and hugepage related vma (VM_HUGEPAGE or VM_HUGETLB) should not set
> at the same time. In what testcase are these flags both set?

I don't think VM_PFNMAP and VM_HUGE... set at the same time.
The problem is that a VM_PFNMAP'd area might have 2MB mappings in its
page table, but they may point to pfn's that are not backed by page
structures.

Then a sequence like:
show_smap
  show_map_vma
    walk_page_range
      walk_pud_range
        walk_pmd_range
          split_huge_page_pmd(walk->mm, pmd)
            __split_huge_page_pmd
              page = pmd_page(*pmd)
can address   (vmemmap + (pfn)) and panic

Or a sequence like this:
        walk_pmd_range
          walk->pmd_entry(pmd, addr, next, walk)
          smaps_pte_range
            smaps_pte_entry(*pte, addr, PAGE_SIZE, walk)
              page = vm_normal_page(vma, addr, ptent)
                return pfn_to_page(pfn)
> 
> And I guess this race can also happen on reading pagemap or numa_maps because
> walk_page_range() is called in those code paths. Are you sure the race doesn't
> happen on these paths? If not, please add a few more flag checks for them.

Okay.  I'll check and submit a version 2 of this patch.

-Cliff Wickman
 
> Thanks,
> Naoya Horiguchi
> 
>  
> > VM_PFNMAP areas are used by
> > - graphics memory manager   gpu/drm/drm_gem.c
> > - global reference unit     sgi-gru/grufile.c
> > - sgi special memory        char/mspec.c
> > - probably several out-of-tree modules
> > 
> > I'm copying everyone who has changed fs/proc/task_mmu.c recently, in case
> > of some reason to provide /proc/<pid>/smaps for these areas that I am not
> > aware of.
> > 
> > Signed-off-by: Cliff Wickman <cpw@sgi.com>
> > ---
> >  fs/proc/task_mmu.c |    3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > Index: linux/fs/proc/task_mmu.c
> > ===================================================================
> > --- linux.orig/fs/proc/task_mmu.c
> > +++ linux/fs/proc/task_mmu.c
> > @@ -589,6 +589,9 @@ static int show_smap(struct seq_file *m,
> >  		.private = &mss,
> >  	};
> >  
> > +	if (vma->vm_flags & VM_PFNMAP)
> > +		return 0;
> > +
> >  	memset(&mss, 0, sizeof mss);
> >  	mss.vma = vma;
> >  	/* mmap_sem is held in m_start */

-- 
Cliff Wickman
SGI
cpw@sgi.com
(651) 683-3824