Re: [PATCH] ramfs: fix memleak on no-mmu arch

[Hugh Dickins <hughd@google.com>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4382 bytes --]

On Fri, 1 Apr 2011, Bob Liu wrote:
> Hi, Andrew
> 
> cc'd some folks working on nommu.
> 
> On Tue, Mar 29, 2011 at 8:02 AM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > On Mon, 28 Mar 2011 13:32:35 +0800
> > Bob Liu <lliubbo@gmail.com> wrote:
> >
> >> On no-mmu arch, there is a memleak duirng shmem test.
> >> The cause of this memleak is ramfs_nommu_expand_for_mapping() added page
> >> refcount to 2 which makes iput() can't free that pages.
> >>
> >> The simple test file is like this:
> >> int main(void)
> >> {
> >>       int i;
> >>       key_t k = ftok("/etc", 42);
> >>
> >>       for ( i=0; i<100; ++i) {
> >>               int id = shmget(k, 10000, 0644|IPC_CREAT);
> >>               if (id == -1) {
> >>                       printf("shmget error\n");
> >>               }
> >>               if(shmctl(id, IPC_RMID, NULL ) == -1) {
> >>                       printf("shm  rm error\n");
> >>                       return -1;
> >>               }
> >>       }
> >>       printf("run ok...\n");
> >>       return 0;
> >> }
> >>
> >> ...
> >>
> >> diff --git a/fs/ramfs/file-nommu.c b/fs/ramfs/file-nommu.c
> >> index 9eead2c..fbb0b47 100644
> >> --- a/fs/ramfs/file-nommu.c
> >> +++ b/fs/ramfs/file-nommu.c
> >> @@ -112,6 +112,7 @@ int ramfs_nommu_expand_for_mapping(struct inode *inode, size_t newsize)
> >>               SetPageDirty(page);
> >>
> >>               unlock_page(page);
> >> +             put_page(page);
> >>       }
> >>
> >>       return 0;
> >
> > Something is still wrong here.
> >
> > A live, in-use page should have a refcount of three.  One for the
> > existence of the page, one for its presence on the page LRU and one for
> > its existence in the pagecache radix tree.
> >
> > So allocation should do:
> >
> >        alloc_pages()
> >        add_to_page_cache()
> >        add_to_lru()
> >
> > and deallocation should do
> >
> >        remove_from_lru()
> >        remove_from_page_cache()
> >        put_page()
> >
> > If this protocol is followed correctly, there is no need to do a
> > put_page() during the allocation/setup phase!
> >
> > I suspect that the problem in nommu really lies in the
> > deallocation/teardown phase.
> >
> 
> What about below patch ?
> 
> BTW: It seems that in MMU cases shmem pages are freed during memory reclaim,
> since I didn't find the direct free place.
> I am not sure maybe I got something wrong ?
> 
> Signed-off-by: Bob Liu <lliubbo@gmail.com>

No, I really think this ramfs_evict_inode() thing is a quite
unnecessary complication: your little put_page() patch much nicer.

Hugh

> ---
>  fs/ramfs/file-nommu.c |    1 +
>  fs/ramfs/inode.c      |   22 ++++++++++++++++++++++
>  2 files changed, 23 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/ramfs/file-nommu.c b/fs/ramfs/file-nommu.c
> index fbb0b47..11f48eb 100644
> --- a/fs/ramfs/file-nommu.c
> +++ b/fs/ramfs/file-nommu.c
> @@ -114,6 +114,7 @@ int ramfs_nommu_expand_for_mapping(struct inode
> *inode, size_t newsize)
>  		unlock_page(page);
>  		put_page(page);
>  	}
> +	inode->i_private = pages;
> 
>  	return 0;
> 
> diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c
> index eacb166..e446d9f 100644
> --- a/fs/ramfs/inode.c
> +++ b/fs/ramfs/inode.c
> @@ -151,9 +151,31 @@ static const struct inode_operations
> ramfs_dir_inode_operations = {
>  	.rename		= simple_rename,
>  };
> 
> +#ifndef CONFIG_MMU
> +static void ramfs_evict_inode(struct inode *inode)
> +{
> +	int i;
> +	struct page *free_pages = (struct page *)inode->i_private;
> +
> +	/*
> +	 * for nommu arch, need an extra put_page so that pages gotten
> +	 * by ramfs_nommu_expand_for_mapping() can be freed
> +	 */
> +	for (i = 0; i < inode->i_data.nrpages; i++)
> +		put_page(free_pages + i);
> +
> +	if (inode->i_data.nrpages)
> +		truncate_inode_pages(&inode->i_data, 0);
> +	end_writeback(inode);
> +}
> +#endif
> +
>  static const struct super_operations ramfs_ops = {
>  	.statfs		= simple_statfs,
>  	.drop_inode	= generic_delete_inode,
> +#ifndef CONFIG_MMU
> +	.evict_inode    = ramfs_evict_inode,
> +#endif
>  	.show_options	= generic_show_options,
>  };
> 
> -- 
> 1.6.3.3