Re: [RFC PATCH v2 02/11] vfs: define a generic function to read a file from the kernel

[Mimi Zohar <zohar@linux.vnet.ibm.com>]

On Wed, 2016-01-20 at 02:09 +0100, Luis R. Rodriguez wrote:
> On Mon, Jan 18, 2016 at 10:11:17AM -0500, Mimi Zohar wrote:
> > diff --git a/fs/exec.c b/fs/exec.c
> > index b06623a..6d623c2 100644
> > --- a/fs/exec.c
> > +++ b/fs/exec.c
> > @@ -831,6 +832,58 @@ int kernel_read(struct file *file, loff_t offset,
> >  
> >  EXPORT_SYMBOL(kernel_read);
> >  
> > +int kernel_read_file(struct file *file, void **buf, loff_t *size,
> > +		     loff_t max_size)
> > +{
> > +	loff_t i_size, pos;
> > +	ssize_t bytes = 0;
> > +	int ret;
> > +
> > +	if (!S_ISREG(file_inode(file)->i_mode))
> > +		return -EINVAL;
> > +
> > +	i_size = i_size_read(file_inode(file));
> > +	if (max_size > 0 && i_size > max_size)
> > +		return -EFBIG;
> 
> loff_t is a __kernel_loff_t, which in turn is a long long, and that's
> signed. We don't catch a negative value here, for max_size, we could
> return -EINVAL if its < 0.
> 
> > +	if (i_size == 0)
> > +		return -EINVAL;
> 
> Likewise for i_size. The setter of the size will depend on how the
> code calling this routine setup the struct file passed.
> 
> So how about adding a i_size <= 0 check here as well here?
> At least fw_read_file_contents() has historically done this,
> so if this generic read is going to skip that I'd like to
> see why. We're unifying so I rather be more pedantic.
> 
> Provided this is addressed feel free to peg:
> 
> Reviewed-by: Luis R. Rodriguez <mcgrof@suse.com>

Don't know how I missed that.  Thanks!

Mimi