On Thu, Feb 6, 2020 at 1:33 PM Xiao Yang <ice_yangxiao@xxxxxxx> wrote: > > 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, fixed. Miklos
