On Mon, Oct 05, 2020 at 01:45:31PM -0400, Vivek Goyal wrote: > virtiofs currently maps various buffers in scatter gather list and it looks > at number of pages (ap->pages) and assumes that same number of pages will > be used both for input and output (sg_count_fuse_req()), and calculates > total number of scatterlist elements accordingly. > > But looks like this assumption is not valid in all the cases. For example, > Cai Qian reported that trinity, triggers warning with virtiofs sometimes. > A closer look revealed that if one calls ioctl(fd, 0x5a004000, buf), it > will trigger following warning. > > WARN_ON(out_sgs + in_sgs != total_sgs) > > In this case, total_sgs = 8, out_sgs=4, in_sgs=3. Number of pages is 2 > (ap->pages), but out_sgs are using both the pages but in_sgs are using > only one page. (fuse_do_ioctl() sets out_size to one page). > > So existing WARN_ON() seems to be wrong. Instead of total_sgs, it should > be max_sgs and make sure out_sgs and in_sgs don't cross max_sgs. This > will allow input and output pages numbers to be different. > > Reported-by: Qian Cai <cai@xxxxxxxxxx> > Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx> > Link: https://lore.kernel.org/linux-fsdevel/5ea77e9f6cb8c2db43b09fbd4158ab2d8c066a0a.camel@xxxxxxxxxx/ > --- > fs/fuse/virtio_fs.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c > index da3ede268604..3f4f2fa0bb96 100644 > --- a/fs/fuse/virtio_fs.c > +++ b/fs/fuse/virtio_fs.c > @@ -1110,17 +1110,17 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq, > unsigned int argbuf_used = 0; > unsigned int out_sgs = 0; > unsigned int in_sgs = 0; > - unsigned int total_sgs; > + unsigned int max_sgs; > unsigned int i; > int ret; > bool notify; > struct fuse_pqueue *fpq; > > /* Does the sglist fit on the stack? */ > - total_sgs = sg_count_fuse_req(req); sg_count_fuse_req() should be exact. It's risky to treat it as a maximum unless all cases where in_sgs + out_sgs < total_sgs are understood. Even then, it's still possible that new bugs introduced to the code will go undetected due to the weaker WARN_ON() condition. Do you have the values of the relevant fuse_req and fuse_args_pages fields so we can understand exactly what happened? I think the issue is that sg_count_fuse_req() doesn't use the fuse_page_desc size field. Stefan
Attachment:
signature.asc
Description: PGP signature
