[Virtio-fs] [PATCH] virtiofs: Fix false positive warning

[Stefan Hajnoczi]

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 at redhat.com>
> Signed-off-by: Vivek Goyal <vgoyal at 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.

Stefan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virtio-fs/attachments/20201006/d651bcbc/attachment.sig>