[PATCH 0/2] Fix pointer arithmetic in indirect read for libvhost-user and libvduse

[PATCH 0/2] Fix pointer arithmetic in indirect read for libvhost-user and libvduse

[Temir Zharaspayev]

Hello! I have found a problem with virtqueue_read_indirect_desc function, which
was advancing pointer to struct as it was a byte pointer, so every element
comming after first chunk would be copied somewhere out of buffer.

As I understand this is cold path, but nevertheless worth fixing.

Also, exacly same problem in vduse_queue_read_indirect_desc function, because
as I understand it is a copy of virtqueue_read_indirect_desc with vduse
backend.

I was not sure if element of scattered buffer may end in the middle of
vring_desc struct data, so instead of writing
desc += read_len/sizeof(struct vring_desc)
have implemented fix with proper byte pointer arithmetic.

Sincerely,
Temir.

Temir Zharaspayev (2):
  libvhost-user: Fix pointer arithmetic in indirect read
  libvduse: Fix pointer arithmetic in indirect read

 subprojects/libvduse/libvduse.c           | 11 ++++++-----
 subprojects/libvhost-user/libvhost-user.c | 11 ++++++-----
 2 files changed, 12 insertions(+), 10 deletions(-)

-- 
2.34.1

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

[Temir Zharaspayev]

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;
     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;
     }
 
     return 0;
-- 
2.34.1

[PATCH 2/2] libvduse: Fix pointer arithmetic in indirect read

[Temir Zharaspayev]

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/libvduse/libvduse.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/subprojects/libvduse/libvduse.c b/subprojects/libvduse/libvduse.c
index 21ffbb5b8d..0b445fbc76 100644
--- a/subprojects/libvduse/libvduse.c
+++ b/subprojects/libvduse/libvduse.c
@@ -465,7 +465,7 @@ static int
 vduse_queue_read_indirect_desc(VduseDev *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))) {
@@ -476,17 +476,18 @@ vduse_queue_read_indirect_desc(VduseDev *dev, struct vring_desc *desc,
         return -1;
     }
 
+    dst_cursor = (uint8_t *) desc;
     while (len) {
         read_len = len;
-        ori_desc = iova_to_va(dev, &read_len, addr);
-        if (!ori_desc) {
+        src_cursor = iova_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;
     }
 
     return 0;
-- 
2.34.1

Re: [PATCH 0/2] Fix pointer arithmetic in indirect read for libvhost-user and libvduse

[Тимур]

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

Hello, I am very sorry for bothering community on a such minor problem
again, but I got no response for a few weeks, so maybe I have started
thread on a wrong mailing list, so I made an issue in gitlab issue tracker:
https://gitlab.com/qemu-project/qemu/-/issues/2149 referencing this thread.

Maybe, it would help attract proper eyes to such a simple problem, so no
one bothers in trying to fix it, albeit it lives in the codebase for some
time already and is being copied around.

Sincerely,
Temir.

сб, 13 янв. 2024 г. в 04:28, Temir Zharaspayev <masscry@gmail.com>:

> Hello! I have found a problem with virtqueue_read_indirect_desc function,
> which
> was advancing pointer to struct as it was a byte pointer, so every element
> comming after first chunk would be copied somewhere out of buffer.
>
> As I understand this is cold path, but nevertheless worth fixing.
>
> Also, exacly same problem in vduse_queue_read_indirect_desc function,
> because
> as I understand it is a copy of virtqueue_read_indirect_desc with vduse
> backend.
>
> I was not sure if element of scattered buffer may end in the middle of
> vring_desc struct data, so instead of writing
> desc += read_len/sizeof(struct vring_desc)
> have implemented fix with proper byte pointer arithmetic.
>
> Sincerely,
> Temir.
>
> Temir Zharaspayev (2):
>   libvhost-user: Fix pointer arithmetic in indirect read
>   libvduse: Fix pointer arithmetic in indirect read
>
>  subprojects/libvduse/libvduse.c           | 11 ++++++-----
>  subprojects/libvhost-user/libvhost-user.c | 11 ++++++-----
>  2 files changed, 12 insertions(+), 10 deletions(-)
>
> --
> 2.34.1
>
>

[-- Attachment #2: Type: text/html, Size: 2161 bytes --]

Re: [PATCH 0/2] Fix pointer arithmetic in indirect read for libvhost-user and libvduse

[Peter Maydell]

Temir: yeah, this was our fault, apologies for not responding.

Michael, David, Raphael -- looks like we unfortunately lost
track of this patchset -- could one of you have a look and
review it, please?

thanks
-- PMM

On Sun, 4 Feb 2024 at 09:42, Тимур <masscry@gmail.com> wrote:
>
> Hello, I am very sorry for bothering community on a such minor problem again, but I got no response for a few weeks, so maybe I have started thread on a wrong mailing list, so I made an issue in gitlab issue tracker: https://gitlab.com/qemu-project/qemu/-/issues/2149 referencing this thread.
>
> Maybe, it would help attract proper eyes to such a simple problem, so no one bothers in trying to fix it, albeit it lives in the codebase for some time already and is being copied around.
>
> Sincerely,
> Temir.
>
> сб, 13 янв. 2024 г. в 04:28, Temir Zharaspayev <masscry@gmail.com>:
>>
>> Hello! I have found a problem with virtqueue_read_indirect_desc function, which
>> was advancing pointer to struct as it was a byte pointer, so every element
>> comming after first chunk would be copied somewhere out of buffer.
>>
>> As I understand this is cold path, but nevertheless worth fixing.
>>
>> Also, exacly same problem in vduse_queue_read_indirect_desc function, because
>> as I understand it is a copy of virtqueue_read_indirect_desc with vduse
>> backend.
>>
>> I was not sure if element of scattered buffer may end in the middle of
>> vring_desc struct data, so instead of writing
>> desc += read_len/sizeof(struct vring_desc)
>> have implemented fix with proper byte pointer arithmetic.
>>
>> Sincerely,
>> Temir.
>>
>> Temir Zharaspayev (2):
>>   libvhost-user: Fix pointer arithmetic in indirect read
>>   libvduse: Fix pointer arithmetic in indirect read
>>
>>  subprojects/libvduse/libvduse.c           | 11 ++++++-----
>>  subprojects/libvhost-user/libvhost-user.c | 11 ++++++-----
>>  2 files changed, 12 insertions(+), 10 deletions(-)
>>
>> --
>> 2.34.

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

[Daniel P. Berrangé]

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;
>      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 :|

Re: [PATCH 0/2] Fix pointer arithmetic in indirect read for libvhost-user and libvduse

[Daniel P. Berrangé]

Adding Michael back to the CC, since he's the designated
maintainer for libvhost-user/

Michael, could you give these patches a review since
they've been pending for many months now.

On Sun, Feb 04, 2024 at 12:41:31PM +0300, Тимур wrote:
> Hello, I am very sorry for bothering community on a such minor problem
> again, but I got no response for a few weeks, so maybe I have started
> thread on a wrong mailing list, so I made an issue in gitlab issue tracker:
> https://gitlab.com/qemu-project/qemu/-/issues/2149 referencing this thread.
> 
> Maybe, it would help attract proper eyes to such a simple problem, so no
> one bothers in trying to fix it, albeit it lives in the codebase for some
> time already and is being copied around.
> 
> Sincerely,
> Temir.
> 
> сб, 13 янв. 2024 г. в 04:28, Temir Zharaspayev <masscry@gmail.com>:
> 
> > Hello! I have found a problem with virtqueue_read_indirect_desc function,
> > which
> > was advancing pointer to struct as it was a byte pointer, so every element
> > comming after first chunk would be copied somewhere out of buffer.
> >
> > As I understand this is cold path, but nevertheless worth fixing.
> >
> > Also, exacly same problem in vduse_queue_read_indirect_desc function,
> > because
> > as I understand it is a copy of virtqueue_read_indirect_desc with vduse
> > backend.
> >
> > I was not sure if element of scattered buffer may end in the middle of
> > vring_desc struct data, so instead of writing
> > desc += read_len/sizeof(struct vring_desc)
> > have implemented fix with proper byte pointer arithmetic.
> >
> > Sincerely,
> > Temir.
> >
> > Temir Zharaspayev (2):
> >   libvhost-user: Fix pointer arithmetic in indirect read
> >   libvduse: Fix pointer arithmetic in indirect read
> >
> >  subprojects/libvduse/libvduse.c           | 11 ++++++-----
> >  subprojects/libvhost-user/libvhost-user.c | 11 ++++++-----
> >  2 files changed, 12 insertions(+), 10 deletions(-)
> >
> > --
> > 2.34.1
> >
> >

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 :|

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 :|
>
>