Re: [PATCH v3 02/26] virtiofsd: Don't assume header layout

[Dr. David Alan Gilbert]

* Dr. David Alan Gilbert (git) (dgilbert@redhat.com) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@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@redhat.com>

Queued this 2/26 only

> ---
>  tools/virtiofsd/fuse_virtio.c | 94 +++++++++++++++++++++++++++--------
>  1 file changed, 73 insertions(+), 21 deletions(-)
> 
> diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> index 3e13997406..6dd73c9b72 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;
> +        }
> +
> +        skip -= sg[vec].iov_len;
> +    }
> +
> +    *sg_1stindex = vec;
> +    *sg_1stskip = 0;
> +    return skip == 0;
>  }
>  
>  /*
> @@ -457,6 +494,7 @@ static void fv_queue_worker(gpointer data, gpointer 
> user_data)
>      bool allocated_bufv = false;
>      struct fuse_bufvec bufv;
>      struct fuse_bufvec *pbufv;
> +    struct fuse_in_header inh;
>  
>      assert(se->bufsize > sizeof(struct fuse_in_header));
>  
> @@ -505,14 +543,15 @@ static void fv_queue_worker(gpointer data, gpointer 
> user_data)
>                   elem->index);
>          assert(0); /* TODO */
>      }
> -    /* Copy just the first element and look at it */
> -    copy_from_iov(&fbuf, 1, out_sg);
> +    /* Copy just the fuse_in_header and look at it */
> +    copy_from_iov(&fbuf, out_num, out_sg,
> +                  sizeof(struct fuse_in_header));
> +    memcpy(&inh, fbuf.mem, sizeof(struct fuse_in_header));
>  
>      pbufv = NULL; /* Compiler thinks an unitialised path */
> -    if (out_num > 2 &&
> -        out_sg[0].iov_len == sizeof(struct fuse_in_header) &&
> -        ((struct fuse_in_header *)fbuf.mem)->opcode == FUSE_WRITE &&
> -        out_sg[1].iov_len == sizeof(struct fuse_write_in)) {
> +    if (inh.opcode == FUSE_WRITE &&
> +        out_len >= (sizeof(struct fuse_in_header) +
> +                    sizeof(struct fuse_write_in))) {
>          /*
>           * For a write we don't actually need to copy the
>           * data, we can just do it straight out of guest memory
> @@ -521,15 +560,15 @@ static void fv_queue_worker(gpointer data, gpointer 
> user_data)
>           */
>          fuse_log(FUSE_LOG_DEBUG, "%s: Write special case\n", __func__);
>  
> -        /* copy the fuse_write_in header afte rthe fuse_in_header */
> -        fbuf.mem += out_sg->iov_len;
> -        copy_from_iov(&fbuf, 1, out_sg + 1);
> -        fbuf.mem -= out_sg->iov_len;
> -        fbuf.size = out_sg[0].iov_len + out_sg[1].iov_len;
> +        fbuf.size = copy_from_iov(&fbuf, out_num, out_sg,
> +                                  sizeof(struct fuse_in_header) +
> +                                  sizeof(struct fuse_write_in));
> +        /* That copy reread the in_header, make sure we use the original */
> +        memcpy(fbuf.mem, &inh, sizeof(struct fuse_in_header));
>  
>          /* Allocate the bufv, with space for the rest of the iov */
>          pbufv = malloc(sizeof(struct fuse_bufvec) +
> -                       sizeof(struct fuse_buf) * (out_num - 2));
> +                       sizeof(struct fuse_buf) * out_num);
>          if (!pbufv) {
>              fuse_log(FUSE_LOG_ERR, "%s: pbufv malloc failed\n",
>                      __func__);
> @@ -540,24 +579,37 @@ static void fv_queue_worker(gpointer data, gpointer 
> user_data)
>          pbufv->count = 1;
>          pbufv->buf[0] = fbuf;
>  
> -        size_t iovindex, pbufvindex;
> -        iovindex = 2; /* 2 headers, separate iovs */
> +        size_t iovindex, pbufvindex, iov_bytes_skip;
>          pbufvindex = 1; /* 2 headers, 1 fusebuf */
>  
> +        if (!skip_iov(out_sg, out_num,
> +                      sizeof(struct fuse_in_header) +
> +                      sizeof(struct fuse_write_in),
> +                      &iovindex, &iov_bytes_skip)) {
> +            fuse_log(FUSE_LOG_ERR, "%s: skip failed\n",
> +                    __func__);
> +            goto out;
> +        }
> +
>          for (; iovindex < out_num; iovindex++, pbufvindex++) {
>              pbufv->count++;
>              pbufv->buf[pbufvindex].pos = ~0; /* Dummy */
>              pbufv->buf[pbufvindex].flags = 0;
>              pbufv->buf[pbufvindex].mem = out_sg[iovindex].iov_base;
>              pbufv->buf[pbufvindex].size = out_sg[iovindex].iov_len;
> +
> +            if (iov_bytes_skip) {
> +                pbufv->buf[pbufvindex].mem += iov_bytes_skip;
> +                pbufv->buf[pbufvindex].size -= iov_bytes_skip;
> +                iov_bytes_skip = 0;
> +            }
>          }
>      } else {
>          /* Normal (non fast write) path */
>  
> -        /* Copy the rest of the buffer */
> -        fbuf.mem += out_sg->iov_len;
> -        copy_from_iov(&fbuf, out_num - 1, out_sg + 1);
> -        fbuf.mem -= out_sg->iov_len;
> +        copy_from_iov(&fbuf, out_num, out_sg, se->bufsize);
> +        /* That copy reread the in_header, make sure we use the original */
> +        memcpy(fbuf.mem, &inh, sizeof(struct fuse_in_header));
>          fbuf.size = out_len;
>  
>          /* TODO! Endianness of header */
> -- 
> 2.31.1
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK