[PATCH] mm: do_generic_file_read() never gets a NULL 'filp' argument

[PATCH] mm: do_generic_file_read() never gets a NULL 'filp' argument

[Krishna Kumar]

From: Krishna Kumar <krkumar2@in.ibm.com>

The 'filp' argument to do_generic_file_read() is never NULL.

Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
---
 mm/filemap.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff -ruNp 2.6.27-rc7-org/mm/filemap.c 2.6.27-rc7-new/mm/filemap.c
--- 2.6.27-rc7-org/mm/filemap.c	2008-09-17 12:53:25.000000000 +0530
+++ 2.6.27-rc7-new/mm/filemap.c	2008-09-17 12:53:59.000000000 +0530
@@ -1186,8 +1186,7 @@ out:
 	ra->prev_pos |= prev_offset;
 
 	*ppos = ((loff_t)index << PAGE_CACHE_SHIFT) + offset;
-	if (filp)
-		file_accessed(filp);
+	file_accessed(filp);
 }
 
 int file_read_actor(read_descriptor_t *desc, struct page *page,

Re: [PATCH] mm: do_generic_file_read() never gets a NULL 'filp' argument

[Andrew Morton]

On Mon, 22 Sep 2008 11:46:10 +0530 Krishna Kumar <krkumar2@in.ibm.com> wrote:

> From: Krishna Kumar <krkumar2@in.ibm.com>
> 
> The 'filp' argument to do_generic_file_read() is never NULL.
> 

How did you verify this?

NFS used to like passing NULL file*'s into VFS functions.  I don't
recall whether it affected do_generic_file_read().

>  mm/filemap.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff -ruNp 2.6.27-rc7-org/mm/filemap.c 2.6.27-rc7-new/mm/filemap.c
> --- 2.6.27-rc7-org/mm/filemap.c	2008-09-17 12:53:25.000000000 +0530
> +++ 2.6.27-rc7-new/mm/filemap.c	2008-09-17 12:53:59.000000000 +0530
> @@ -1186,8 +1186,7 @@ out:
>  	ra->prev_pos |= prev_offset;
>  
>  	*ppos = ((loff_t)index << PAGE_CACHE_SHIFT) + offset;
> -	if (filp)
> -		file_accessed(filp);
> +	file_accessed(filp);
>  }
>  
>  int file_read_actor(read_descriptor_t *desc, struct page *page,

Re: [PATCH] mm: do_generic_file_read() never gets a NULL 'filp' argument

[Christoph Hellwig]

On Mon, Sep 22, 2008 at 01:33:38AM -0700, Andrew Morton wrote:
> On Mon, 22 Sep 2008 11:46:10 +0530 Krishna Kumar <krkumar2@in.ibm.com> wrote:
> 
> > From: Krishna Kumar <krkumar2@in.ibm.com>
> > 
> > The 'filp' argument to do_generic_file_read() is never NULL.
> > 
> 
> How did you verify this?
> 
> NFS used to like passing NULL file*'s into VFS functions.  I don't
> recall whether it affected do_generic_file_read().

The only thing nfsd passed a NULL file pointer in is ->fsync on
directories.  If anyone did pass a NULL file pointer into
do_generic_file_read it would oops in it's very first line of code.

Re: [PATCH] mm: do_generic_file_read() never gets a NULL 'filp' argument

[KOSAKI Motohiro]

> On Mon, 22 Sep 2008 11:46:10 +0530 Krishna Kumar <krkumar2@in.ibm.com> wrote:
> 
> > From: Krishna Kumar <krkumar2@in.ibm.com>
> > 
> > The 'filp' argument to do_generic_file_read() is never NULL.
> > 
> 
> How did you verify this?
> 
> NFS used to like passing NULL file*'s into VFS functions.  I don't
> recall whether it affected do_generic_file_read().

Andrew, current do_generic_file_read have filp dereference at the beginning of function.
So, I believe nobody pass to NULL.


static void do_generic_file_read(struct file *filp, loff_t *ppos,
                read_descriptor_t *desc, read_actor_t actor)
{
        struct address_space *mapping = filp->f_mapping;  /* here */
        struct inode *inode = mapping->host;
        struct file_ra_state *ra = &filp->f_ra;          /* ditto */
        pgoff_t index;
        pgoff_t last_index;
        pgoff_t prev_index;



> 
> >  mm/filemap.c |    3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff -ruNp 2.6.27-rc7-org/mm/filemap.c 2.6.27-rc7-new/mm/filemap.c
> > --- 2.6.27-rc7-org/mm/filemap.c	2008-09-17 12:53:25.000000000 +0530
> > +++ 2.6.27-rc7-new/mm/filemap.c	2008-09-17 12:53:59.000000000 +0530
> > @@ -1186,8 +1186,7 @@ out:
> >  	ra->prev_pos |= prev_offset;
> >  
> >  	*ppos = ((loff_t)index << PAGE_CACHE_SHIFT) + offset;
> > -	if (filp)
> > -		file_accessed(filp);
> > +	file_accessed(filp);
> >  }
> >  
> >  int file_read_actor(read_descriptor_t *desc, struct page *page,
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mm: do_generic_file_read() never gets a NULL 'filp' argument

[Andrew Morton]

On Mon, 22 Sep 2008 04:39:46 -0400 Christoph Hellwig <hch@infradead.org> wrote:

> On Mon, Sep 22, 2008 at 01:33:38AM -0700, Andrew Morton wrote:
> > On Mon, 22 Sep 2008 11:46:10 +0530 Krishna Kumar <krkumar2@in.ibm.com> wrote:
> > 
> > > From: Krishna Kumar <krkumar2@in.ibm.com>
> > > 
> > > The 'filp' argument to do_generic_file_read() is never NULL.
> > > 
> > 
> > How did you verify this?
> > 
> > NFS used to like passing NULL file*'s into VFS functions.  I don't
> > recall whether it affected do_generic_file_read().
> 
> The only thing nfsd passed a NULL file pointer in is ->fsync on
> directories.

NFS, not NFSD.  It was page_cache_read(), iirc.

Re: [PATCH] mm: do_generic_file_read() never gets a NULL 'filp' argument

[KOSAKI Motohiro]

Hi Andrew,

> > > > The 'filp' argument to do_generic_file_read() is never NULL.
> > > 
> > > How did you verify this?
> > > 
> > > NFS used to like passing NULL file*'s into VFS functions.  I don't
> > > recall whether it affected do_generic_file_read().
> > 
> > The only thing nfsd passed a NULL file pointer in is ->fsync on
> > directories.
> 
> NFS, not NFSD.  It was page_cache_read(), iirc.

Is it important?
Cristoph wrote following decision reason at last mail.
I believe he was right.

> If anyone did pass a NULL file pointer into
> do_generic_file_read it would oops in it's very first line of code.


IOW, do_generic_file_read() has following calling hieralcy.

generic_file_aio_read()
  +- do_generic_file_read()

and fs/nfs/file.c has

	const struct file_operations nfs_file_operations = {
	        .llseek         = nfs_file_llseek,
	        .read           = do_sync_read,
	        .write          = do_sync_write,
	        .aio_read       = nfs_file_read,
	        .aio_write      = nfs_file_write,

So, nfs doesn't use generic_file_aio_read.
it directly indicate nfs doesn't use do_generic_file_read().

in the other hand, page_cache_read() has following calling hieralcy.

filemap_fault()
  +- page_cache_read()
      +- a_ops->readpage()

So, page_cache_read is mmap() related helper function and
do_generic_file_read is read() related helper function.
they are independent IMHO.