Re: 2.6.29 pat issue

["Pallipadi, Venkatesh" <venkatesh.pallipadi@intel.com>]

On Wed, 2009-03-04 at 01:56 -0800, Thomas Hellstrom wrote:
> Hi!
> 
> Pallipadi, Venkatesh wrote:
> > On Fri, Feb 06, 2009 at 01:43:08AM -0800, Thomas Hellström wrote:
> >   
> >> Eric W. Biederman wrote:
> >>     
> >>> Thomas Hellstrom <thellstrom@vmware.com> writes:
> >>>
> >>>
> >>>
> >>>       
> >>>> Indeed, it's crucial to keep the mappings consistent, but failure to do so is a
> >>>> kernel driver bug, it should never be the result of invalid user data.
> >>>>
> >>>>         
> >>> It easily can be.  Think of an X server mmaping frame buffers. Or other
> >>> device bars.
> >>>
> >>>
> >>>       
> >> Hmm, Yes  you're right, although I'm still a bit doubtful about RAM pages.
> >>
> >> Wait. Now I see what's causing the problems. The code is  assuming that
> >> VM_PFNMAP vmas never map RAM pages. That's also an invalid assumption.
> >> See comments in mm/memory.c
> >>
> >> So probably the attribute check should be done for the insert_pfn path
> >> of VM_MIXEDMAP as well. That's not done today.
> >>
> >> So there are three distinct bugs at this point:
> >>
> >> 1) VMAs with VM_PFNMAP are incorrectly assumed to be linear if
> >> vma->vm_pgoff non-null.
> >> 2) VM_PFNMAP VMA PTEs are incorrectly assumed to never point to physical
> >> RAM.
> >> 3) There is no check for the insert_pfn  path of vm_insert_mixed().
> >>
> >>     
> >
> > Patch below will solve (1) above.
> >   
> 
> Yes, hmm, but how about remap_pfn_range() having an optimized function 
> to call into directly, instead of
> track_pfn_vma_new?

No. The idea was to have this flag set, only when full vma is mapped
using remap_pfn_range. In all other cases, insert_pfn or partial region
mapped in remap_pfn_range, we will use the slow path. Having said that,
the patch I had sent here missed one of the conditions. Refreshing the
patch.

> 
> > About (2), Yes. we can optimize the PAT code if we use struct page to track
> > PFNMAP as long at memory is backed by a struct page. It has some complications
> > with refcounting the number of mappings and related things. We are actively
> > looking at it. 
> 
> Cool. Still, there needs to be a check for non-io pages in vm_insert_pfn 
> to avoid hitting the WARN_ON_ONCE:
> + if (!pfn_valid(pfn)) //Pfn pointing to a non-ram page.
>     track_pfn_vma_new()

About the WARN_ON. I am not completely clear aboue how/why you are
getting the warning. You will only get WARN_ON, if driver is trying to
map actual RAM pages using remap_pfn_range or vm_insert_pfn. That is a
valid warning, I think. If RAM is intended to be mapped, then driver
should be using insert_page or insert_mixed. No? Are you getting the
warning even when you use vm_insert_page or vm_insert_mixed?

Thanks,
Venki