Message-ID: <ea16c19e-bd60-82ec-4825-05e233667f9f@broadcom.com>
Date:   Mon, 8 Jun 2020 15:29:22 -0700
From:   Scott Branden <scott.branden@...adcom.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     Luis Chamberlain <mcgrof@...nel.org>,
        Wolfram Sang <wsa@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        David Brown <david.brown@...aro.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Shuah Khan <shuah@...nel.org>, bjorn.andersson@...aro.org,
        Shuah Khan <skhan@...uxfoundation.org>,
        Arnd Bergmann <arnd@...db.de>,
        Mimi Zohar <zohar@...ux.ibm.com>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-fsdevel@...r.kernel.org,
        BCM Kernel Feedback <bcm-kernel-feedback-list@...adcom.com>,
        Olof Johansson <olof@...om.net>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Colin Ian King <colin.king@...onical.com>,
        Kees Cook <keescook@...omium.org>,
        Takashi Iwai <tiwai@...e.de>, linux-kselftest@...r.kernel.org,
        Andy Gross <agross@...nel.org>,
        linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        Christoph Hellwig <hch@...radead.org>
Subject: Re: [PATCH v7 1/8] fs: introduce kernel_pread_file* support

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.

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.
>> +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.

The partial read option is then needed further in the depths of the 
kernel read for IMA operation as IMA does
things differently for optimization of whether it is OK to do a partial 
read of the file or not.
>> +{
>> +	loff_t alloc_size;
>> +	loff_t buf_pos;
>> +	loff_t read_end;
>> +	loff_t i_size;
>>   	ssize_t bytes = 0;
>>   	int ret;
>>   
> Look, it's not your fault, but this is a great example of how we end
> up with atrocious interfaces.  Someone comes along and implements a
> simple DWIM interface that solves their problem.  Then somebody else
> adds a slight variant that solves their problem, and so on and so on,
> and we end up with this bonkers API where the arguments literally change
> meaning depending on other arguments.
I don't see what arguments are changing meaning.  Please explain what is 
changing meaning.
The diff below is for kernel_pread_file, not kernel_read_file. Perhaps 
that is where your confusion is.
>
>> @@ -950,21 +955,31 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>>   		ret = -EINVAL;
>>   		goto out;
>>   	}
>> -	if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
>> +
>> +	/* Default read to end of file */
>> +	read_end = i_size;
>> +
>> +	/* Allow reading partial portion of file */
>> +	if ((opt == KERNEL_PREAD_PART) &&
>> +	    (i_size > (pos + max_size)))
>> +		read_end = pos + max_size;
>> +
>> +	alloc_size = read_end - pos;
>> +	if (i_size > SIZE_MAX || (max_size > 0 && alloc_size > max_size)) {
>>   		ret = -EFBIG;
>>   		goto out;
> ... like that.
like what?  We need to determine how much of the file to read based on 
size of file, position in file, and max size we can read.
>
> 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.
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.

Your proposed change doesn't exist and is not simple as it sounds or 
meet all the needs of the existing kernel_read_file
function, IMA, and new partial kernel_pread_file?

Patch v7 does not break existing functions or rearchitect things in a 
dramatic way.  They fit into existing code,
will not break the existing codepaths (which some didn't even have a 
test case until I added one), and can
be improved upon as need with your vmap_file_range or others once those 
have been developed, tested, and
proven by someone.

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.
Thanks,
Scott

Powered by blists - more mailing lists