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

From: Scott Branden
Date: Mon Jun 08 2020 - 18:29:43 EST

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