Re: [PATCH v2] nommu: add page_align to mmap

[Greg Ungerer <gerg@xxxxxxxxxxxx>]

Hi Bob,

On 10/06/11 15:39, Bob Liu wrote:
> Hi, Greg
>
> On Fri, Jun 10, 2011 at 11:51 AM, Greg Ungerer<gerg@xxxxxxxxxxxx>  wrote:
> > Hi Bob,
> >
> > On 09/06/11 20:30, Bob Liu wrote:
> > > On Wed, Jun 8, 2011 at 6:19 PM, Greg Ungerer<gerg@xxxxxxxxxxxx>  áwrote:
> > > > > > > > When booting on a ColdFire (m68knommu) target the init process (or
> > > > > > > > there abouts at least) fails. Last console messages are:
> > > > > > > >
> > > > > > > > ...
> > > > > > > > VFS: Mounted root (romfs filesystem) readonly on device 31:0.
> > > > > > > > Freeing unused kernel memory: 52k freed (0x401aa000 - 0x401b6000)
> > > > > > > > Unable to mmap process text, errno 22
> > > > > > >
> > > > > > > Oh, bad news. I will try to reproduce it on my board.
> > > > > > > If you are free please enable debug in nommu.c and then we can see
> > > > > > > what
> > > > > > > caused the problem.
> > > > > >
> > > > > > Yep, with debug on:
> > > > > >
> > > > > > ¡...
> > > > > > VFS: Mounted root (romfs filesystem) readonly on device 31:0.
> > > > > > Freeing unused kernel memory: 52k freed (0x4018c000 - 0x40198000)
> > > > > > ==>  á├ído_mmap_pgoff(,0,6780,5,1002,0)
> > > > > > <== do_mmap_pgoff() = -22
> > > > > > Unable to mmap process text, errno 22
> > > > >
> > > > > Since I can't reproduce this problem, could you please attach the
> > > > > whole dmesg log with nommu debug on or
> > > > > you can step into to see why errno 22 is returned, is it returned by
> > > > > do_mmap_private()?
> > > >
> > > > There was no other debug messages with debug turned on in nommu.c.
> > > > (I can give you the boot msgs before this if you want, but there
> > > > was no nommu.c debug in it).
> > > >
> > > > But I did trace it into do_mmap_pgoff() to see what was failing.
> > > > It fails based on the return value from:
> > > >
> > > > addr = file->f_op->get_unmapped_area(file, addr, len,
> > > > á á á á á á á á á á á á á á á á á á á á á ípgoff, flags);
> > >
> > > Thanks for this information.
> > > But it's a callback function. I still can't know what's the problem maybe.
> > > Would you do me a favor to do more trace to see where it callback to,
> > > fs or some driver etc..?
> >
> > Its calling to romfs_get_unmapped_area() [fs/romfs/mmap-nommu.c]. It is
> > being called with:
> >
> > áromfs_get_unmapped_area(addr=0,len=7000,pgoff=0,flags=1002)
> >
> > This is failing the first size check because isize comes back
> > as 0x6ca8, and this is smaller then len (0x7000). Thus returning
> > -EINVAL.
>
> I look into file fs/romfs/mmap-nommu.c based on your trace.
> In my opinion, romfs_get_unmapped_area() in mmap-nommu.c is buggy.
> Would you please try below commit.

Yep, this fixes it. I think David (Howells) originally made
those changes, not sure if he wants to add anything here.

Thanks
Greg



> Thanks a lot.
>
> from 786add5286ffb476807cb198d7b2c5455e9fb533 Mon Sep 17 00:00:00 2001
> From: Bob Liu<lliubbo@xxxxxxxxx>
> Date: Fri, 10 Jun 2011 13:34:48 +0800
> Subject: [PATCH] romfs: fix romfs_get_unmapped_area() param check
>
> romfs_get_unmapped_area() check len param without considering PAGE_ALIGN which
> will cause do_mmap_pgoff() return -EINVAL error after commit f67d9b1576c.
>
> This patch fix the param check by changing it to the same way as function
> ramfs_nommu_get_unmapped_area() did in ramfs/file-nommu.c.
>
> Signed-off-by: Bob Liu<lliubbo@xxxxxxxxx>
> ---
>   fs/romfs/mmap-nommu.c |    8 ++++++--
>   1 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/romfs/mmap-nommu.c b/fs/romfs/mmap-nommu.c
> index f0511e8..eed9942 100644
> --- a/fs/romfs/mmap-nommu.c
> +++ b/fs/romfs/mmap-nommu.c
> @@ -27,14 +27,18 @@ static unsigned long
> romfs_get_unmapped_area(struct file *file,
>   {
>          struct inode *inode = file->f_mapping->host;
>          struct mtd_info *mtd = inode->i_sb->s_mtd;
> -       unsigned long isize, offset;
> +       unsigned long isize, offset, maxpages, lpages;
>
>          if (!mtd)
>                  goto cant_map_directly;
>
> +       /* the mapping mustn't extend beyond the EOF */
> +       lpages = (len + PAGE_SIZE - 1)>>  PAGE_SHIFT;
>          isize = i_size_read(inode);
>          offset = pgoff<<  PAGE_SHIFT;
> -       if (offset>  isize || len>  isize || offset>  isize - len)
> +
> +       maxpages = (isize + PAGE_SIZE - 1)>>  PAGE_SHIFT;
> +       if ((pgoff>= maxpages) || (maxpages - pgoff<  lpages))
>                  return (unsigned long) -EINVAL;
>
>          /* we need to call down to the MTD layer to do the actual mapping */
> --
> 1.6.3.3
>
> > That code is trying to map the contents of the file /bin/init
> > directly from the romfs filesystem (which is in RAM). The init
> > binary is 0x6ca8 bytes in size (that is the isize above).


--
------------------------------------------------------------------------
Greg Ungerer  --  Principal Engineer        EMAIL:     gerg@xxxxxxxxxxxx
SnapGear Group, McAfee                      PHONE:       +61 7 3435 2888
8 Gardner Close                             FAX:         +61 7 3217 5323
Milton, QLD, 4064, Australia                WEB: http://www.SnapGear.com

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href