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. Then we can think about clamping to i_size, whether it is necessary and if it is, how to best implement it. Thanks, Miklos
