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

[Greg Ungerer <gerg@snapgear.com>]

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@snapgear.com>  wrote:
>> Hi Bob,
>>
>> On 09/06/11 20:30, Bob Liu wrote:
>>>
>>> On Wed, Jun 8, 2011 at 6:19 PM, Greg Ungerer<gerg@snapgear.com>  á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@gmail.com>
> 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@gmail.com>
> ---
>   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@snapgear.com
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@kvack.org.  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=mailto:"dont@kvack.org"> email@kvack.org </a>