Fix MAP_PRIVATE mmap() of files and devices where the data in the backing store
might be mapped directly. Use the BDI_CAP_MAP_DIRECT capability flag to govern
whether or not we should be trying to map a file directly. This can be used to
determine whether or not a region has been filled in at the point where we call
do_mmap_shared() or do_mmap_private().
The BDI_CAP_MAP_DIRECT capability flag is cleared by validate_mmap_request() if
there's any reason we can't use it. It's also cleared in do_mmap_pgoff() if
f_op->get_unmapped_area() fails.
Without this fix, attempting to run a program from a RomFS image on a
non-mappable MTD partition results in a BUG as the kernel attempts XIP, and
this can be caught in gdb:
Program received signal SIGABRT, Aborted.
0xc005dce8 in add_nommu_region (region=<value optimized out>) at mm/nommu.c:547
(gdb) bt
#0 0xc005dce8 in add_nommu_region (region=<value optimized out>) at mm/nommu.c:547
#1 0xc005f168 in do_mmap_pgoff (file=0xc31a6620, addr=<value optimized out>, len=3808, prot=3, flags=6146, pgoff=0) at mm/nommu.c:1373
#2 0xc00a96b8 in elf_fdpic_map_file (params=0xc33fbbec, file=0xc31a6620, mm=0xc31bef60, what=0xc0213144 "executable") at mm.h:1145
#3 0xc00aa8b4 in load_elf_fdpic_binary (bprm=0xc316cb00, regs=<value optimized out>) at fs/binfmt_elf_fdpic.c:343
#4 0xc006b588 in search_binary_handler (bprm=0x6, regs=0xc33fbce0) at fs/exec.c:1234
#5 0xc006c648 in do_execve (filename=<value optimized out>, argv=0xc3ad14cc, envp=0xc3ad1460, regs=0xc33fbce0) at fs/exec.c:1356
#6 0xc0008cf0 in sys_execve (name=<value optimized out>, argv=0xc3ad14cc, envp=0xc3ad1460) at arch/frv/kernel/process.c:263
#7 0xc00075dc in __syscall_call () at arch/frv/kernel/entry.S:897
Note that this fix does the following commit differently:
commit a190887b58c32d19c2eee007c5eb8faa970a69ba
Author: David Howells <[email protected]>
Date: Sat Sep 5 11:17:07 2009 -0700
nommu: fix error handling in do_mmap_pgoff()
Reported-by: Graff Yang <[email protected]>
Signed-off-by: David Howells <[email protected]>
Cc: Pekka Enberg <[email protected]>
Cc: Paul Mundt <[email protected]>
Cc: Mel Gorman <[email protected]>
Cc: Greg Ungerer <[email protected]>
---
mm/nommu.c | 34 ++++++++++++----------------------
1 files changed, 12 insertions(+), 22 deletions(-)
diff --git a/mm/nommu.c b/mm/nommu.c
index c459aec..cc24d9f 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -1074,7 +1074,7 @@ static int do_mmap_shared_file(struct vm_area_struct *vma)
ret = vma->vm_file->f_op->mmap(vma->vm_file, vma);
if (ret == 0) {
vma->vm_region->vm_top = vma->vm_region->vm_end;
- return ret;
+ return 0;
}
if (ret != -ENOSYS)
return ret;
@@ -1091,7 +1091,8 @@ static int do_mmap_shared_file(struct vm_area_struct *vma)
*/
static int do_mmap_private(struct vm_area_struct *vma,
struct vm_region *region,
- unsigned long len)
+ unsigned long len,
+ unsigned long capabilities)
{
struct page *pages;
unsigned long total, point, n, rlen;
@@ -1102,13 +1103,13 @@ static int do_mmap_private(struct vm_area_struct *vma,
* shared mappings on devices or memory
* - VM_MAYSHARE will be set if it may attempt to share
*/
- if (vma->vm_file) {
+ if (capabilities & BDI_CAP_MAP_DIRECT) {
ret = vma->vm_file->f_op->mmap(vma->vm_file, vma);
if (ret == 0) {
/* shouldn't return success if we're not sharing */
BUG_ON(!(vma->vm_flags & VM_MAYSHARE));
vma->vm_region->vm_top = vma->vm_region->vm_end;
- return ret;
+ return 0;
}
if (ret != -ENOSYS)
return ret;
@@ -1346,7 +1347,7 @@ unsigned long do_mmap_pgoff(struct file *file,
* - this is the hook for quasi-memory character devices to
* tell us the location of a shared mapping
*/
- if (file && file->f_op->get_unmapped_area) {
+ if (capabilities & BDI_CAP_MAP_DIRECT) {
addr = file->f_op->get_unmapped_area(file, addr, len,
pgoff, flags);
if (IS_ERR((void *) addr)) {
@@ -1370,15 +1371,17 @@ unsigned long do_mmap_pgoff(struct file *file,
}
vma->vm_region = region;
- add_nommu_region(region);
- /* set up the mapping */
+ /* set up the mapping
+ * - the region is filled in if BDI_CAP_MAP_DIRECT is still set
+ */
if (file && vma->vm_flags & VM_SHARED)
ret = do_mmap_shared_file(vma);
else
- ret = do_mmap_private(vma, region, len);
+ ret = do_mmap_private(vma, region, len, capabilities);
if (ret < 0)
- goto error_put_region;
+ goto error_just_free;
+ add_nommu_region(region);
/* okay... we have a mapping; now we have to register it */
result = vma->vm_start;
@@ -1396,19 +1399,6 @@ share:
kleave(" = %lx", result);
return result;
-error_put_region:
- __put_nommu_region(region);
- if (vma) {
- if (vma->vm_file) {
- fput(vma->vm_file);
- if (vma->vm_flags & VM_EXECUTABLE)
- removed_exe_file_vma(vma->vm_mm);
- }
- kmem_cache_free(vm_area_cachep, vma);
- }
- kleave(" = %d [pr]", ret);
- return ret;
-
error_just_free:
up_write(&nommu_region_sem);
error:
Hi David,
On Thu, Sep 24, 2009 at 5:13 PM, David Howells <[email protected]> wrote:
> Fix MAP_PRIVATE mmap() of files and devices where the data in the backing store
> might be mapped directly. ?Use the BDI_CAP_MAP_DIRECT capability flag to govern
> whether or not we should be trying to map a file directly. ?This can be used to
> determine whether or not a region has been filled in at the point where we call
> do_mmap_shared() or do_mmap_private().
>
> The BDI_CAP_MAP_DIRECT capability flag is cleared by validate_mmap_request() if
> there's any reason we can't use it. ?It's also cleared in do_mmap_pgoff() if
> f_op->get_unmapped_area() fails.
>
>
> Without this fix, attempting to run a program from a RomFS image on a
> non-mappable MTD partition results in a BUG as the kernel attempts XIP, and
> this can be caught in gdb:
>
> Program received signal SIGABRT, Aborted.
> 0xc005dce8 in add_nommu_region (region=<value optimized out>) at mm/nommu.c:547
> (gdb) bt
> #0 ?0xc005dce8 in add_nommu_region (region=<value optimized out>) at mm/nommu.c:547
> #1 ?0xc005f168 in do_mmap_pgoff (file=0xc31a6620, addr=<value optimized out>, len=3808, prot=3, flags=6146, pgoff=0) at mm/nommu.c:1373
> #2 ?0xc00a96b8 in elf_fdpic_map_file (params=0xc33fbbec, file=0xc31a6620, mm=0xc31bef60, what=0xc0213144 "executable") at mm.h:1145
> #3 ?0xc00aa8b4 in load_elf_fdpic_binary (bprm=0xc316cb00, regs=<value optimized out>) at fs/binfmt_elf_fdpic.c:343
> #4 ?0xc006b588 in search_binary_handler (bprm=0x6, regs=0xc33fbce0) at fs/exec.c:1234
> #5 ?0xc006c648 in do_execve (filename=<value optimized out>, argv=0xc3ad14cc, envp=0xc3ad1460, regs=0xc33fbce0) at fs/exec.c:1356
> #6 ?0xc0008cf0 in sys_execve (name=<value optimized out>, argv=0xc3ad14cc, envp=0xc3ad1460) at arch/frv/kernel/process.c:263
> #7 ?0xc00075dc in __syscall_call () at arch/frv/kernel/entry.S:897
I have no idea about the BDI_CAP_MAP_DIRECT bits but...
> Note that this fix does the following commit differently:
>
> ? ? ? ?commit a190887b58c32d19c2eee007c5eb8faa970a69ba
> ? ? ? ?Author: David Howells <[email protected]>
> ? ? ? ?Date: ? Sat Sep 5 11:17:07 2009 -0700
> ? ? ? ?nommu: fix error handling in do_mmap_pgoff()
...changes to this part of the code look like nice cleanup to me.
Acked-by: Pekka Enberg <[email protected]>
On Thu, 24 Sep 2009 15:13:10 +0100
David Howells <[email protected]> wrote:
> Fix MAP_PRIVATE mmap() of files and devices where the data in the backing store
> might be mapped directly. Use the BDI_CAP_MAP_DIRECT capability flag to govern
> whether or not we should be trying to map a file directly. This can be used to
> determine whether or not a region has been filled in at the point where we call
> do_mmap_shared() or do_mmap_private().
>
> The BDI_CAP_MAP_DIRECT capability flag is cleared by validate_mmap_request() if
> there's any reason we can't use it. It's also cleared in do_mmap_pgoff() if
> f_op->get_unmapped_area() fails.
>
The patch seems -stable-worthy but I see no cc:stable here?
> Note that this fix does the following commit differently:
>
> commit a190887b58c32d19c2eee007c5eb8faa970a69ba
> Author: David Howells <[email protected]>
> Date: Sat Sep 5 11:17:07 2009 -0700
> nommu: fix error handling in do_mmap_pgoff()
I don't understand what you're saying here.
> Reported-by: Graff Yang <[email protected]>
> Signed-off-by: David Howells <[email protected]>
> Cc: Pekka Enberg <[email protected]>
> Cc: Paul Mundt <[email protected]>
> Cc: Mel Gorman <[email protected]>
> Cc: Greg Ungerer <[email protected]>
Andrew Morton <[email protected]> wrote:
> > Note that this fix does the following commit differently:
> >
> > commit a190887b58c32d19c2eee007c5eb8faa970a69ba
> > Author: David Howells <[email protected]>
> > Date: Sat Sep 5 11:17:07 2009 -0700
> > nommu: fix error handling in do_mmap_pgoff()
>
> I don't understand what you're saying here.
Sorry, I was attempting to point out that this appears to revert some of that
patch - whilst this is true, it then implements the fix differently.
David
On Thu, Sep 24, 2009 at 03:13:10PM +0100, David Howells wrote:
> Fix MAP_PRIVATE mmap() of files and devices where the data in the backing store
> might be mapped directly. Use the BDI_CAP_MAP_DIRECT capability flag to govern
> whether or not we should be trying to map a file directly. This can be used to
> determine whether or not a region has been filled in at the point where we call
> do_mmap_shared() or do_mmap_private().
>
> The BDI_CAP_MAP_DIRECT capability flag is cleared by validate_mmap_request() if
> there's any reason we can't use it. It's also cleared in do_mmap_pgoff() if
> f_op->get_unmapped_area() fails.
>
>
> Without this fix, attempting to run a program from a RomFS image on a
> non-mappable MTD partition results in a BUG as the kernel attempts XIP, and
> this can be caught in gdb:
>
> Program received signal SIGABRT, Aborted.
> 0xc005dce8 in add_nommu_region (region=<value optimized out>) at mm/nommu.c:547
> (gdb) bt
> #0 0xc005dce8 in add_nommu_region (region=<value optimized out>) at mm/nommu.c:547
> #1 0xc005f168 in do_mmap_pgoff (file=0xc31a6620, addr=<value optimized out>, len=3808, prot=3, flags=6146, pgoff=0) at mm/nommu.c:1373
> #2 0xc00a96b8 in elf_fdpic_map_file (params=0xc33fbbec, file=0xc31a6620, mm=0xc31bef60, what=0xc0213144 "executable") at mm.h:1145
> #3 0xc00aa8b4 in load_elf_fdpic_binary (bprm=0xc316cb00, regs=<value optimized out>) at fs/binfmt_elf_fdpic.c:343
> #4 0xc006b588 in search_binary_handler (bprm=0x6, regs=0xc33fbce0) at fs/exec.c:1234
> #5 0xc006c648 in do_execve (filename=<value optimized out>, argv=0xc3ad14cc, envp=0xc3ad1460, regs=0xc33fbce0) at fs/exec.c:1356
> #6 0xc0008cf0 in sys_execve (name=<value optimized out>, argv=0xc3ad14cc, envp=0xc3ad1460) at arch/frv/kernel/process.c:263
> #7 0xc00075dc in __syscall_call () at arch/frv/kernel/entry.S:897
>
>
> Note that this fix does the following commit differently:
>
> commit a190887b58c32d19c2eee007c5eb8faa970a69ba
> Author: David Howells <[email protected]>
> Date: Sat Sep 5 11:17:07 2009 -0700
> nommu: fix error handling in do_mmap_pgoff()
>
> Reported-by: Graff Yang <[email protected]>
> Signed-off-by: David Howells <[email protected]>
> Cc: Pekka Enberg <[email protected]>
> Cc: Paul Mundt <[email protected]>
> Cc: Mel Gorman <[email protected]>
> Cc: Greg Ungerer <[email protected]>
Acked-by: Paul Mundt <[email protected]>
On Fri, 25 Sep 2009 01:39:17 +0100 David Howells <[email protected]> wrote:
> Andrew Morton <[email protected]> wrote:
>
> > > Note that this fix does the following commit differently:
> > >
> > > commit a190887b58c32d19c2eee007c5eb8faa970a69ba
> > > Author: David Howells <[email protected]>
> > > Date: Sat Sep 5 11:17:07 2009 -0700
> > > nommu: fix error handling in do_mmap_pgoff()
> >
> > I don't understand what you're saying here.
>
> Sorry, I was attempting to point out that this appears to revert some of that
> patch - whilst this is true, it then implements the fix differently.
>
OK.
And what are your opinions on the -stable desirability?
Andrew Morton <[email protected]> wrote:
> And what are your opinions on the -stable desirability?
I think that it would be a good idea to add it to stable.
David
On Thu, Sep 24, 2009 at 10:13 PM, David Howells <[email protected]> wrote:
> Fix MAP_PRIVATE mmap() of files and devices where the data in the backing store
> might be mapped directly. Use the BDI_CAP_MAP_DIRECT capability flag to govern
> whether or not we should be trying to map a file directly. This can be used to
> determine whether or not a region has been filled in at the point where we call
> do_mmap_shared() or do_mmap_private().
>
> The BDI_CAP_MAP_DIRECT capability flag is cleared by validate_mmap_request() if
> there's any reason we can't use it. It's also cleared in do_mmap_pgoff() if
> f_op->get_unmapped_area() fails.
>
>
> Without this fix, attempting to run a program from a RomFS image on a
> non-mappable MTD partition results in a BUG as the kernel attempts XIP, and
> this can be caught in gdb:
>
> Program received signal SIGABRT, Aborted.
> 0xc005dce8 in add_nommu_region (region=<value optimized out>) at mm/nommu.c:547
> (gdb) bt
> #0 0xc005dce8 in add_nommu_region (region=<value optimized out>) at mm/nommu.c:547
> #1 0xc005f168 in do_mmap_pgoff (file=0xc31a6620, addr=<value optimized out>, len=3808, prot=3, flags=6146, pgoff=0) at mm/nommu.c:1373
> #2 0xc00a96b8 in elf_fdpic_map_file (params=0xc33fbbec, file=0xc31a6620, mm=0xc31bef60, what=0xc0213144 "executable") at mm.h:1145
> #3 0xc00aa8b4 in load_elf_fdpic_binary (bprm=0xc316cb00, regs=<value optimized out>) at fs/binfmt_elf_fdpic.c:343
> #4 0xc006b588 in search_binary_handler (bprm=0x6, regs=0xc33fbce0) at fs/exec.c:1234
> #5 0xc006c648 in do_execve (filename=<value optimized out>, argv=0xc3ad14cc, envp=0xc3ad1460, regs=0xc33fbce0) at fs/exec.c:1356
> #6 0xc0008cf0 in sys_execve (name=<value optimized out>, argv=0xc3ad14cc, envp=0xc3ad1460) at arch/frv/kernel/process.c:263
> #7 0xc00075dc in __syscall_call () at arch/frv/kernel/entry.S:897
>
>
> Note that this fix does the following commit differently:
>
> commit a190887b58c32d19c2eee007c5eb8faa970a69ba
> Author: David Howells <[email protected]>
> Date: Sat Sep 5 11:17:07 2009 -0700
> nommu: fix error handling in do_mmap_pgoff()
>
> Reported-by: Graff Yang <[email protected]>
> Signed-off-by: David Howells <[email protected]>
> Cc: Pekka Enberg <[email protected]>
> Cc: Paul Mundt <[email protected]>
> Cc: Mel Gorman <[email protected]>
> Cc: Greg Ungerer <[email protected]>
> ---
>
> mm/nommu.c | 34 ++++++++++++----------------------
> 1 files changed, 12 insertions(+), 22 deletions(-)
>
>
> diff --git a/mm/nommu.c b/mm/nommu.c
> index c459aec..cc24d9f 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -1074,7 +1074,7 @@ static int do_mmap_shared_file(struct vm_area_struct *vma)
> ret = vma->vm_file->f_op->mmap(vma->vm_file, vma);
> if (ret == 0) {
> vma->vm_region->vm_top = vma->vm_region->vm_end;
> - return ret;
> + return 0;
> }
> if (ret != -ENOSYS)
> return ret;
> @@ -1091,7 +1091,8 @@ static int do_mmap_shared_file(struct vm_area_struct *vma)
> */
> static int do_mmap_private(struct vm_area_struct *vma,
> struct vm_region *region,
> - unsigned long len)
> + unsigned long len,
> + unsigned long capabilities)
> {
> struct page *pages;
> unsigned long total, point, n, rlen;
> @@ -1102,13 +1103,13 @@ static int do_mmap_private(struct vm_area_struct *vma,
> * shared mappings on devices or memory
> * - VM_MAYSHARE will be set if it may attempt to share
> */
> - if (vma->vm_file) {
> + if (capabilities & BDI_CAP_MAP_DIRECT) {
This will breaks many drivers, e.g. some frame-buffer drivers, on NOMMU system.
Because they don't have get_unmapped_area().
These drivers depend on it's mmap() to return the frame-buffer base address.
-Graff
> ret = vma->vm_file->f_op->mmap(vma->vm_file, vma);
> if (ret == 0) {
> /* shouldn't return success if we're not sharing */
> BUG_ON(!(vma->vm_flags & VM_MAYSHARE));
> vma->vm_region->vm_top = vma->vm_region->vm_end;
> - return ret;
> + return 0;
> }
> if (ret != -ENOSYS)
> return ret;
> @@ -1346,7 +1347,7 @@ unsigned long do_mmap_pgoff(struct file *file,
> * - this is the hook for quasi-memory character devices to
> * tell us the location of a shared mapping
> */
> - if (file && file->f_op->get_unmapped_area) {
> + if (capabilities & BDI_CAP_MAP_DIRECT) {
> addr = file->f_op->get_unmapped_area(file, addr, len,
> pgoff, flags);
> if (IS_ERR((void *) addr)) {
> @@ -1370,15 +1371,17 @@ unsigned long do_mmap_pgoff(struct file *file,
> }
>
> vma->vm_region = region;
> - add_nommu_region(region);
>
> - /* set up the mapping */
> + /* set up the mapping
> + * - the region is filled in if BDI_CAP_MAP_DIRECT is still set
> + */
> if (file && vma->vm_flags & VM_SHARED)
> ret = do_mmap_shared_file(vma);
> else
> - ret = do_mmap_private(vma, region, len);
> + ret = do_mmap_private(vma, region, len, capabilities);
> if (ret < 0)
> - goto error_put_region;
> + goto error_just_free;
> + add_nommu_region(region);
>
> /* okay... we have a mapping; now we have to register it */
> result = vma->vm_start;
> @@ -1396,19 +1399,6 @@ share:
> kleave(" = %lx", result);
> return result;
>
> -error_put_region:
> - __put_nommu_region(region);
> - if (vma) {
> - if (vma->vm_file) {
> - fput(vma->vm_file);
> - if (vma->vm_flags & VM_EXECUTABLE)
> - removed_exe_file_vma(vma->vm_mm);
> - }
> - kmem_cache_free(vm_area_cachep, vma);
> - }
> - kleave(" = %d [pr]", ret);
> - return ret;
> -
> error_just_free:
> up_write(&nommu_region_sem);
> error:
>
>
--
-Graff
graff yang <[email protected]> wrote:
> > + ? ? ? if (capabilities & BDI_CAP_MAP_DIRECT) {
>
> This will breaks many drivers, e.g. some frame-buffer drivers, on NOMMU
> system. Because they don't have get_unmapped_area(). These drivers depend
> on it's mmap() to return the frame-buffer base address.
Then they won't work on NOMMU. Read Documentation/nommu-mmap.txt:
============================================
PROVIDING SHAREABLE CHARACTER DEVICE SUPPORT
============================================
To provide shareable character device support, a driver must provide a
file->f_op->get_unmapped_area() operation. The mmap() routines will
call this to get a proposed address for the mapping. This may return an
error if it doesn't wish to honour the mapping because it's too long,
at a weird offset, under some unsupported combination of flags or
whatever.
The chardev driver doesn't provide the virtual address through its mmap() fop
in MMU-mode either - that's provided by do_mmap_pgoff().
David
On Fri, Sep 25, 2009 at 4:24 PM, David Howells <[email protected]> wrote:
> graff yang <[email protected]> wrote:
>
>> > + if (capabilities & BDI_CAP_MAP_DIRECT) {
>>
>> This will breaks many drivers, e.g. some frame-buffer drivers, on NOMMU
>> system. Because they don't have get_unmapped_area(). These drivers depend
>> on it's mmap() to return the frame-buffer base address.
>
> Then they won't work on NOMMU. Read Documentation/nommu-mmap.txt:
>
> ============================================
> PROVIDING SHAREABLE CHARACTER DEVICE SUPPORT
> ============================================
>
> To provide shareable character device support, a driver must provide a
> file->f_op->get_unmapped_area() operation. The mmap() routines will
> call this to get a proposed address for the mapping. This may return an
> error if it doesn't wish to honour the mapping because it's too long,
> at a weird offset, under some unsupported combination of flags or
> whatever.
>
> The chardev driver doesn't provide the virtual address through its mmap() fop
> in MMU-mode either - that's provided by do_mmap_pgoff().
>
> David
>
OK, I think do_mmap_pgoff() now looks nice with your patch. Merely,
at least the alsa/fb driver
will crash due to the vm_region changes.
The alsa guys are fixing this issue:
http://mailman.alsa-project.org/pipermail/alsa-devel/2009-March/015802.html
Is there any other things be affected?
--
-Graff