Re: [PATCH 1/2] libvhost-user: Fix pointer arithmetic in indirect read

[Raphael Norwitz]

The change looks right to me. As is, it looks like the code is
skipping over descriptors when the intent should be to bounce data
into a single descriptor.

I agree the variable rename should go in as a separate change.

On Thu, Apr 18, 2024 at 6:56 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Sat, Jan 13, 2024 at 04:27:40AM +0300, Temir Zharaspayev wrote:
> > When zero-copy usage of indirect descriptors buffer table isn't
> > possible, library gather scattered memory chunks in a local copy.
> > This commit fixes the issue with pointer arithmetic for the local copy
> > buffer.
> >
> > Signed-off-by: Temir Zharaspayev <masscry@gmail.com>
> > ---
> >  subprojects/libvhost-user/libvhost-user.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/subprojects/libvhost-user/libvhost-user.c 
> > b/subprojects/libvhost-user/libvhost-user.c
> > index 6684057370..e952c098a3 100644
> > --- a/subprojects/libvhost-user/libvhost-user.c
> > +++ b/subprojects/libvhost-user/libvhost-user.c
> > @@ -2307,7 +2307,7 @@ static int
> >  virtqueue_read_indirect_desc(VuDev *dev, struct vring_desc *desc,
> >                               uint64_t addr, size_t len)
> >  {
> > -    struct vring_desc *ori_desc;
> > +    uint8_t *src_cursor, *dst_cursor;
> >      uint64_t read_len;
> >
> >      if (len > (VIRTQUEUE_MAX_SIZE * sizeof(struct vring_desc))) {
> > @@ -2318,17 +2318,18 @@ virtqueue_read_indirect_desc(VuDev *dev, struct 
> > vring_desc *desc,
> >          return -1;
> >      }
> >
> > +    dst_cursor = (uint8_t *) desc;

Nit - no space on cast

> >      while (len) {
> >          read_len = len;
> > -        ori_desc = vu_gpa_to_va(dev, &read_len, addr);
> > -        if (!ori_desc) {
> > +        src_cursor = vu_gpa_to_va(dev, &read_len, addr);
> > +        if (!src_cursor) {
> >              return -1;
> >          }
> >
> > -        memcpy(desc, ori_desc, read_len);
> > +        memcpy(dst_cursor, src_cursor, read_len);
> >          len -= read_len;
> >          addr += read_len;
> > -        desc += read_len;
> > +        dst_cursor += read_len;
>
> The ori_desc -> src_cursor changes don't look to have any functional
> effect. Having that change present obscures the functional part of
> the patch, which is this line. FWIW, it is generally preferrable to
> not mix functional and non-functional changes in the same patch
>
> It now interprets 'read_len' as the number of bytes to increment the
> address by, rather than incrementing by the number of elements of
> size 'sizeof(struct vring_desc)'.
>
> I don't know enough about this area of QEMU code to say which
> semantics were desired, so I'll defer to the Michael as maintainer
> to give a formal review.
>
>
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>
>