Re: [PATCH] virtiofs: Fix false positive warning

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4752 bytes --]

On Tue, Oct 06, 2020 at 03:09:49PM -0400, Vivek Goyal wrote:
> On Tue, Oct 06, 2020 at 04:39:33PM +0100, Stefan Hajnoczi wrote:
> > 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@redhat.com>
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > Link: https://lore.kernel.org/linux-fsdevel/5ea77e9f6cb8c2db43b09fbd4158ab2d8c066a0a.camel@redhat.com/
> > > ---
> > >  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.
> 
> Hi Stefan,
> 
> I revised the patch. How about following. This calculates number of
> sgs accurately by going through ap->descs and size fields.
> 
> Thanks
> Vivek
> 
> From 24b590ebc2ffc8ed02c013b11818af89d0b135ba Mon Sep 17 00:00:00 2001
> From: Vivek Goyal <vgoyal@redhat.com>
> Date: Tue, 6 Oct 2020 14:53:06 -0400
> Subject: [PATCH 1/1] virtiofs: Calculate number of scatter-gather elements
>  accurately
> 
> 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. In this case, fuse_do_ioctl() sets different size values
> for input and output.
> 
> args->in_args[args->in_numargs - 1].size == 6656
> args->out_args[args->out_numargs - 1].size == 4096
> 
> So current method of calculating how many scatter-gather list elements
> will be used is not accurate. Make calculations more precise by parsing
> size and ap->descs.
> 
> Reported-by: Qian Cai <cai@redhat.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> Link: https://lore.kernel.org/linux-fsdevel/5ea77e9f6cb8c2db43b09fbd4158ab2d8c066a0a.camel@redhat.com/
> ---
>  fs/fuse/virtio_fs.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]