fbmem: VM_IO set, but not propagated.

Hey,

This bug was found when Linux kernel was running under Xen.
In that scenario, any page that has VM_IO flag to it, means that it
MUST be a MMIO/VRAM backend memory , _not_ System RAM. That is what the
fbmem.c does: sets VM_IO, ioremaps the region - everything is peachy.

Well, not exactly. The vm_page_prot does not get the relevant
PTE flags set (_PAGE_IOMAP) which under Xen is a death-kneel to pages
that are referencing real physical devices but don't have that flag set.

Here is the patch:

Author: Daniel De Graaf [off-list ref]
Date:   Wed Jul 21 16:52:46 2010 -0400

    fb: propagate VM_IO to VMA.
    
    When we setup up the VMA flags for the mmap flag and we end up using
    the fallback mmap functionality we set the vma->vm_flags |= VM_IO.
    However we neglect to propagate the flag to the vma->vm_page_prot.
    
    This patch fixes this.
    
    Tested-by: Eamon Walsh [off-list ref]
    Signed-off-by: Konrad Rzeszutek Wilk [off-list ref]

diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 99bbd28..057433a 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c

@@ -1362,6 +1362,7 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
 	vma->vm_pgoff = off >> PAGE_SHIFT;
 	/* This is an IO map - tell maydump to skip this VMA */
 	vma->vm_flags |= VM_IO | VM_RESERVED;
+	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
 	fb_pgprotect(file, vma, off);
 	if (io_remap_pfn_range(vma, vma->vm_start, off >> PAGE_SHIFT,
 			     vma->vm_end - vma->vm_start, vma->vm_page_prot))

On Thu, 22 Jul 2010 17:31:28 -0400
Konrad Rzeszutek Wilk [off-list ref] wrote:

This bug was found when Linux kernel was running under Xen.
In that scenario, any page that has VM_IO flag to it, means that it
MUST be a MMIO/VRAM backend memory , _not_ System RAM. That is what the
fbmem.c does: sets VM_IO, ioremaps the region - everything is peachy.

Well, not exactly. The vm_page_prot does not get the relevant
PTE flags set (_PAGE_IOMAP) which under Xen is a death-kneel to pages
that are referencing real physical devices but don't have that flag set.

Here is the patch:

Author: Daniel De Graaf [off-list ref]
Date:   Wed Jul 21 16:52:46 2010 -0400

    fb: propagate VM_IO to VMA.
    
    When we setup up the VMA flags for the mmap flag and we end up using
    the fallback mmap functionality we set the vma->vm_flags |= VM_IO.
    However we neglect to propagate the flag to the vma->vm_page_prot.
    
    This patch fixes this.
    
    Tested-by: Eamon Walsh [off-list ref]
    Signed-off-by: Konrad Rzeszutek Wilk [off-list ref]

Confused.  We have From:Konrad and Author:Daniel and no signoff from Daniel.

I've committed the patch assuming that Daniel was the author, but
didn't sign off the patch.  Your signoff is sufficient for merging
purposes.

But maybe I was wrong.


I'm also assuming that we can merge this into 2.6.36 and not backport
it into -stable.  But maybe I'm wrong about that too!  Talk to me.

On 07/26/2010 06:38 PM, Andrew Morton wrote:

On Thu, 22 Jul 2010 17:31:28 -0400
Konrad Rzeszutek Wilk [off-list ref] wrote:

quoted

This bug was found when Linux kernel was running under Xen.
In that scenario, any page that has VM_IO flag to it, means that it
MUST be a MMIO/VRAM backend memory , _not_ System RAM. That is what the
fbmem.c does: sets VM_IO, ioremaps the region - everything is peachy.

Well, not exactly. The vm_page_prot does not get the relevant
PTE flags set (_PAGE_IOMAP) which under Xen is a death-kneel to pages
that are referencing real physical devices but don't have that flag set.

Here is the patch:

Author: Daniel De Graaf [off-list ref]
Date:   Wed Jul 21 16:52:46 2010 -0400

    fb: propagate VM_IO to VMA.
    
    When we setup up the VMA flags for the mmap flag and we end up using
    the fallback mmap functionality we set the vma->vm_flags |= VM_IO.
    However we neglect to propagate the flag to the vma->vm_page_prot.
    
    This patch fixes this.
    
    Tested-by: Eamon Walsh [off-list ref]
    Signed-off-by: Konrad Rzeszutek Wilk [off-list ref]

Confused.  We have From:Konrad and Author:Daniel and no signoff from Daniel.

I've committed the patch assuming that Daniel was the author, but
didn't sign off the patch.  Your signoff is sufficient for merging
purposes.

But maybe I was wrong.


I'm also assuming that we can merge this into 2.6.36 and not backport
it into -stable.  But maybe I'm wrong about that too!  Talk to me.

If it's useful to have a signoff line from me, you can add it; I guess
it wasn't clear in my previous reply that I was signing off on it.

Signed-off-by: Daniel De Graaf <redacted>

On Mon, Jul 26, 2010 at 03:38:12PM -0700, Andrew Morton wrote:

On Thu, 22 Jul 2010 17:31:28 -0400
Konrad Rzeszutek Wilk [off-list ref] wrote:

quoted

This bug was found when Linux kernel was running under Xen.
In that scenario, any page that has VM_IO flag to it, means that it
MUST be a MMIO/VRAM backend memory , _not_ System RAM. That is what the
fbmem.c does: sets VM_IO, ioremaps the region - everything is peachy.

Well, not exactly. The vm_page_prot does not get the relevant
PTE flags set (_PAGE_IOMAP) which under Xen is a death-kneel to pages
that are referencing real physical devices but don't have that flag set.

Here is the patch:

Author: Daniel De Graaf [off-list ref]
Date:   Wed Jul 21 16:52:46 2010 -0400

    fb: propagate VM_IO to VMA.
    
    When we setup up the VMA flags for the mmap flag and we end up using
    the fallback mmap functionality we set the vma->vm_flags |= VM_IO.
    However we neglect to propagate the flag to the vma->vm_page_prot.
    
    This patch fixes this.
    
    Tested-by: Eamon Walsh [off-list ref]
    Signed-off-by: Konrad Rzeszutek Wilk [off-list ref]

Confused.  We have From:Konrad and Author:Daniel and no signoff from Daniel.

So Daniel came up with the original fix, I've came to him and suggested
that perhaps we should use the vm_get_page_prot, and he agreed.

Since I've kept the patch in my branch of "Xen-KMS/DRM/TTM-fixes" I
figured I should post the patch upstream and hence I signed off on it.

I've committed the patch assuming that Daniel was the author, but
didn't sign off the patch.  Your signoff is sufficient for merging

Yes. He is the author. Let me refer back to my notes but I figured
if the patch has an Author: set there was no need for an extra S-o-b?

purposes.

But maybe I was wrong.


I'm also assuming that we can merge this into 2.6.36 and not backport
it into -stable.  But maybe I'm wrong about that too!  Talk to me.

No need to backport it.