On 2/6/20 8:14 PM, Miklos Szeredi wrote:
On Wed, Feb 5, 2020 at 3:37 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
On Mon, Feb 3, 2020 at 8:37 AM Xiao Yang <ice_yangxiao@xxxxxxx> wrote:
Buffered read in fuse normally goes via:
-> generic_file_buffered_read()
------------------------------
-> fuse_readpages()
-> fuse_send_readpages()
or
-> fuse_readpage() [if fuse_readpages() fails to get page]
-> fuse_do_readpage()
------------------------------
-> fuse_simple_request()
Buffered read changes original offset to page-aligned length by left-shift
and extends original count to be multiples of PAGE_SIZE and then fuse
forwards these new parameters to a userspace process, so it is possible
for the resulting offset(e.g page-aligned offset + extended count) to
exceed the whole file size(even the max value of off_t) when the userspace
process does read with new parameters.
xfstests generic/525 gets "pread: Invalid argument" error on virtiofs
because it triggers this issue. See the following explanation:
PAGE_SIZE: 4096, file size: 2^63 - 1
Original: offset: 2^63 - 2, count: 1
Changed by buffered read: offset: 2^63 - 4096, count: 4096
New offset + new count exceeds the file size as well as LLONG_MAX
Thanks for the report and analysis.
However this patch may corrupt the cache if i_size changes between
calls to fuse_page_length(). (e.g. first page length set to 33;
second page length to 45; then 33-4095 will be zeroed and 4096-4140
will be filled from offset 33-77). This will be mitigated by the
pages being invalidated when i_size changes
(fuse_change_attributes()). Filling the pages with wrong data is not
a good idea regardless.
I think the best approach is first to just fix the xfstest reported
bug by clamping the end offset to LLONG_MAX. That's a simple patch,
independent of i_size, and hence trivial to verify and hard to mess
up.
Applied a fix and pushed to:
git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#for-next
Hi Miklos,
Sorry for the late reply.
You have applied a fix quickly when I am going to send a patch today.
Just one comment for your fix:
I think we need to add the overflow check in fuse_send_readpages() and
fuse_do_readpage().
Because generic_file_buffered_read() will call fuse_readpage() if
fuse_readpages() fails to get page.
Thanks,
Xiao Yang
Thanks,
Miklos
