Re: [PATCH v7 1/8] fs: introduce kernel_pread_file* support

From: Matthew Wilcox <willy@infradead.org>
To: Scott Branden <scott.branden@broadcom.com>
Cc: Luis Chamberlain <mcgrof@kernel.org>,
	Wolfram Sang <wsa@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	David Brown <david.brown@linaro.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Shuah Khan <shuah@kernel.org>,
	bjorn.andersson@linaro.org,
	Shuah Khan <skhan@linuxfoundation.org>,
	Arnd Bergmann <arnd@arndb.de>, Mimi Zohar <zohar@linux.ibm.com>,
	"Rafael J . Wysocki" <rafael@kernel.org>,
	linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-fsdevel@vger.kernel.org,
	BCM Kernel Feedback <bcm-kernel-feedback-list@broadcom.com>,
	Olof Johansson <olof@lixom.net>,
	Andrew Morton <akpm@linux-foundation.org>,
	Dan Carpenter <dan.carpenter@oracle.com>,
	Colin Ian King <colin.king@canonical.com>,
	Kees Cook <keescook@chromium.org>, Takashi Iwai <tiwai@suse.de>,
	linux-kselftest@vger.kernel.org, Andy Gross <agross@kernel.org>,
	linux-integrity@vger.kernel.org,
	linux-security-module@vger.kernel.org,
	Christoph Hellwig <hch@infradead.org>
Subject: Re: [PATCH v7 1/8] fs: introduce kernel_pread_file* support
Date: Tue, 9 Jun 2020 06:21:51 -0700	[thread overview]
Message-ID: <20200609132151.GC19604@bombadil.infradead.org> (raw)
In-Reply-To: <ea16c19e-bd60-82ec-4825-05e233667f9f@broadcom.com>

On Mon, Jun 08, 2020 at 03:29:22PM -0700, Scott Branden wrote:
> Hi Matthew,
> 
> I am requesting the experts in the filesystem subsystem to come to a
> consensus here.
> This is not my area of expertise at all but every time I have addressed all
> of the
> outstanding concerns someone else comes along and raises another one.

I appreciate it's frustrating for you, but this is the nature of
patch review.  I haven't even read the first five or so submissions.
I can see them in my inbox and they look like long threads.  I'm not
particularly inclined to read them.  I happened to read v6, and reacted
to the API being ugly.

> Please see me comments below.
> 
> On 2020-06-06 8:52 a.m., Matthew Wilcox wrote:
> > On Fri, Jun 05, 2020 at 10:04:51PM -0700, Scott Branden wrote:
> > > -int kernel_read_file(struct file *file, void **buf, loff_t *size,
> > > -		     loff_t max_size, enum kernel_read_file_id id)
> > > -{
> > > -	loff_t i_size, pos;
> Please note that how checkpatch generated the diff here.  The code
> modifications
> below are for a new function kernel_pread_file, they do not modify the
> existing API
> kernel_read_file.  kernel_read_file requests the ENTIRE file is read.  So we
> need to be
> able to differentiate whether it is ok to read just a portion of the file or
> not.

You've gone about this in entirely the wrong way though.  This enum to
read the entire file or a partial is just bad design.

> > > +int kernel_pread_file(struct file *file, void **buf, loff_t *size,
> > > +		      loff_t pos, loff_t max_size,
> > > +		      enum kernel_pread_opt opt,
> > > +		      enum kernel_read_file_id id)
> So, to share common code a new kernel_pread_opt needed to be added in order
> to specify whether
> it was ok to read a partial file or not, and provide an offset into the file
> where to begin reading.
> The meaning of parameters doesn't change in the bonkers API. max_size still
> means max size, etc.
> These options are needed so common code can be shared with kernel_read_file
> api.

Does pread() in userspace take seven parameters?  No.  It takes four.
What you're doing is taking all the complexity of all of the interfaces
and stuffing it all down into the bottom function instead of handling
some of the complexity in the wrapper functions.  For example, you
could support the functionality of 'max_size' in kernel_read_file()
and leave it out of the kernel_pread_file() interface.

> > I think what we actually want is:
> > 
> > ssize_t vmap_file_range(struct file *, loff_t start, loff_t end, void **bufp);
> > void vunmap_file_range(struct file *, void *buf);
> > 
> > If end > i_size, limit the allocation to i_size.  Returns the number
> > of bytes allocated, or a negative errno.  Writes the pointer allocated
> > to *bufp.  Internally, it should use the page cache to read in the pages
> > (taking appropriate reference counts).  Then it maps them using vmap()
> > instead of copying them to a private vmalloc() array.
> > kernel_read_file() can be converted to use this API.  The users will
> > need to be changed to call kernel_read_end(struct file *file, void *buf)
> > instead of vfree() so it can call allow_write_access() for them.
> > 
> > vmap_file_range() has a lot of potential uses.  I'm surprised we don't
> > have it already, to be honest.
> Such a change sounds like it could be done in a later patch series.
> It's an incomplete solution.  It would work for some of the needed
> operations but not others.
> For kernel_read_file, I don't see how in your new API it indicates if the
> end of the file was reached or not.

That's the point.  It doesn't.  If a caller needs that, then they can
figure that out themselves.

> Also, please note that buffers may be preallocated  and shouldn't be freed
> by the kernel in some cases and
> allocated and freed by the kernel in others.

You're trying to build the swiss army knife of functions.  Swiss army
knives are useful, but they're no good for carving a steak.

> I would like the experts here to decide on what needs to be done so we can
> move forward
> and get kernel_pread_file support added soon.

You know, you haven't even said _why_ you want this.  The cover letter
just says "I want this", and doesn't say why it's needed.