[Virtio-fs] [PATCH] virtiofsd: Don't assume header layout

[Stefan Hajnoczi]

On Thu, Mar 11, 2021 at 02:58:09PM +0000, Dr. David Alan Gilbert wrote:
> * Stefan Hajnoczi (stefanha at redhat.com) wrote:
> > On Thu, Mar 04, 2021 at 05:45:36PM +0000, Dr. David Alan Gilbert (git) wrote:
> > > From: "Dr. David Alan Gilbert" <dgilbert at redhat.com>
> > > 
> > > virtiofsd incorrectly assumed a fixed set of header layout in the virt
> > > queue; assuming that the fuse and write headers were conveniently
> > > separated from the data;  the spec doesn't allow us to take that
> > > convenience, so fix it up to deal with it the hard way.
> > > 
> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert at redhat.com>
> > > ---
> > >  tools/virtiofsd/fuse_virtio.c | 84 ++++++++++++++++++++++++++---------
> > >  1 file changed, 63 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> > > index 523ee64fb7..55e2e057ec 100644
> > > --- a/tools/virtiofsd/fuse_virtio.c
> > > +++ b/tools/virtiofsd/fuse_virtio.c
> > > @@ -129,18 +129,55 @@ static void fv_panic(VuDev *dev, const char *err)
> > >   * Copy from an iovec into a fuse_buf (memory only)
> > >   * Caller must ensure there is space
> > >   */
> > > -static void copy_from_iov(struct fuse_buf *buf, size_t out_num,
> > > -                          const struct iovec *out_sg)
> > > +static size_t copy_from_iov(struct fuse_buf *buf, size_t out_num,
> > > +                            const struct iovec *out_sg,
> > > +                            size_t max)
> > >  {
> > >      void *dest = buf->mem;
> > > +    size_t copied = 0;
> > >  
> > > -    while (out_num) {
> > > +    while (out_num && max) {
> > >          size_t onelen = out_sg->iov_len;
> > > +        onelen = MIN(onelen, max);
> > >          memcpy(dest, out_sg->iov_base, onelen);
> > >          dest += onelen;
> > > +        copied += onelen;
> > >          out_sg++;
> > >          out_num--;
> > > +        max -= onelen;
> > >      }
> > > +
> > > +    return copied;
> > > +}
> > > +
> > > +/*
> > > + * Skip 'skip' bytes in the iov; 'sg_1stindex' is set as
> > > + * the index for the 1st iovec to read data from, and
> > > + * 'sg_1stskip' is the number of bytes to skip in that entry.
> > > + *
> > > + * Returns True if there are at least 'skip' bytes in the iovec
> > > + *
> > > + */
> > > +static bool skip_iov(const struct iovec *sg, size_t sg_size,
> > > +                     size_t skip,
> > > +                     size_t *sg_1stindex, size_t *sg_1stskip)
> > > +{
> > > +    size_t vec;
> > > +
> > > +    for (vec = 0; vec < sg_size; vec++) {
> > > +        if (sg[vec].iov_len > skip) {
> > > +            *sg_1stskip = skip;
> > > +            *sg_1stindex = vec;
> > > +
> > > +            return true;
> > > +        }
> > > +
>  (1)         skip -= sg[vec].iov_len;
> > > +    }
> > > +
>  (2)     *sg_1stindex = vec;
> > > +    *sg_1stskip = 0;
>  (3)     return skip == 0;
> > >  }
> > 
> > This comment does not match the code: "Returns True if there are at
> > least 'skip' bytes in the iovec". When skip == iov_length(sg, sg_size)
> > the function returns false.
> 
> I'm not seeing how; in that case, it'll loop around executing (1) for
> each vector, so skip will reduce, then it falls out of the for loop
> and if it was equal to the size of the vector, (1) would have subtracted
> the whole vector out, so skip would now be 0, hence the 'skip == 0' at 2 and
> so it should return true.

You're right. I misread the code!

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/20210311/2042e9fc/attachment.sig>