diff mbox series

[1/2] libvhost-user: return early on virtqueue errors

Message ID 20200921113420.154378-2-stefanha@redhat.com
State New
Headers show
Series libvhost-user: return after vu_panic() | expand

Commit Message

Stefan Hajnoczi Sept. 21, 2020, 11:34 a.m. UTC
vu_panic() is not guaranteed to exit the program. Return early when
errors are encountered.

Note that libvhost-user does not have an "unmap" operation for mapped
descriptors. Therefore it is correct to return without explicit cleanup.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 contrib/libvhost-user/libvhost-user.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

Comments

Philippe Mathieu-Daudé Sept. 21, 2020, 2:26 p.m. UTC | #1
On 9/21/20 1:34 PM, Stefan Hajnoczi wrote:
> vu_panic() is not guaranteed to exit the program. Return early when
> errors are encountered.
> 
> Note that libvhost-user does not have an "unmap" operation for mapped
> descriptors. Therefore it is correct to return without explicit cleanup.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  contrib/libvhost-user/libvhost-user.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c
> index 53f16bdf08..27626e629a 100644
> --- a/contrib/libvhost-user/libvhost-user.c
> +++ b/contrib/libvhost-user/libvhost-user.c
> @@ -2407,7 +2407,7 @@ vu_queue_set_notification(VuDev *dev, VuVirtq *vq, int enable)
>      }
>  }
>  
> -static void
> +static bool
>  virtqueue_map_desc(VuDev *dev,
>                     unsigned int *p_num_sg, struct iovec *iov,
>                     unsigned int max_num_sg, bool is_write,
> @@ -2419,7 +2419,7 @@ virtqueue_map_desc(VuDev *dev,
>  
>      if (!sz) {
>          vu_panic(dev, "virtio: zero sized buffers are not allowed");
> -        return;
> +        return false;
>      }
>  
>      while (sz) {
> @@ -2427,13 +2427,13 @@ virtqueue_map_desc(VuDev *dev,
>  
>          if (num_sg == max_num_sg) {
>              vu_panic(dev, "virtio: too many descriptors in indirect table");
> -            return;
> +            return false;
>          }
>  
>          iov[num_sg].iov_base = vu_gpa_to_va(dev, &len, pa);
>          if (iov[num_sg].iov_base == NULL) {
>              vu_panic(dev, "virtio: invalid address for buffers");
> -            return;
> +            return false;
>          }
>          iov[num_sg].iov_len = len;
>          num_sg++;
> @@ -2442,6 +2442,7 @@ virtqueue_map_desc(VuDev *dev,
>      }
>  
>      *p_num_sg = num_sg;
> +    return true;
>  }

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>  
>  static void *
> @@ -2479,6 +2480,7 @@ vu_queue_map_desc(VuDev *dev, VuVirtq *vq, unsigned int idx, size_t sz)
>      if (desc[i].flags & VRING_DESC_F_INDIRECT) {
>          if (desc[i].len % sizeof(struct vring_desc)) {
>              vu_panic(dev, "Invalid size for indirect buffer table");
> +            return NULL;
>          }
>  
>          /* loop over the indirect descriptor table */
> @@ -2506,22 +2508,27 @@ vu_queue_map_desc(VuDev *dev, VuVirtq *vq, unsigned int idx, size_t sz)
>      /* Collect all the descriptors */
>      do {
>          if (desc[i].flags & VRING_DESC_F_WRITE) {
> -            virtqueue_map_desc(dev, &in_num, iov + out_num,
> -                               VIRTQUEUE_MAX_SIZE - out_num, true,
> -                               desc[i].addr, desc[i].len);
> +            if (!virtqueue_map_desc(dev, &in_num, iov + out_num,
> +                                    VIRTQUEUE_MAX_SIZE - out_num, true,
> +                                    desc[i].addr, desc[i].len)) {
> +                return NULL;
> +            }
>          } else {
>              if (in_num) {
>                  vu_panic(dev, "Incorrect order for descriptors");
>                  return NULL;
>              }
> -            virtqueue_map_desc(dev, &out_num, iov,
> -                               VIRTQUEUE_MAX_SIZE, false,
> -                               desc[i].addr, desc[i].len);
> +            if (!virtqueue_map_desc(dev, &out_num, iov,
> +                                    VIRTQUEUE_MAX_SIZE, false,
> +                                    desc[i].addr, desc[i].len)) {
> +                return NULL;
> +            }
>          }
>  
>          /* If we've got too many, that implies a descriptor loop. */
>          if ((in_num + out_num) > max) {
>              vu_panic(dev, "Looped descriptor");
> +            return NULL;
>          }
>          rc = virtqueue_read_next_desc(dev, desc, i, max, &i);
>      } while (rc == VIRTQUEUE_READ_DESC_MORE);
>
diff mbox series

Patch

diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c
index 53f16bdf08..27626e629a 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -2407,7 +2407,7 @@  vu_queue_set_notification(VuDev *dev, VuVirtq *vq, int enable)
     }
 }
 
-static void
+static bool
 virtqueue_map_desc(VuDev *dev,
                    unsigned int *p_num_sg, struct iovec *iov,
                    unsigned int max_num_sg, bool is_write,
@@ -2419,7 +2419,7 @@  virtqueue_map_desc(VuDev *dev,
 
     if (!sz) {
         vu_panic(dev, "virtio: zero sized buffers are not allowed");
-        return;
+        return false;
     }
 
     while (sz) {
@@ -2427,13 +2427,13 @@  virtqueue_map_desc(VuDev *dev,
 
         if (num_sg == max_num_sg) {
             vu_panic(dev, "virtio: too many descriptors in indirect table");
-            return;
+            return false;
         }
 
         iov[num_sg].iov_base = vu_gpa_to_va(dev, &len, pa);
         if (iov[num_sg].iov_base == NULL) {
             vu_panic(dev, "virtio: invalid address for buffers");
-            return;
+            return false;
         }
         iov[num_sg].iov_len = len;
         num_sg++;
@@ -2442,6 +2442,7 @@  virtqueue_map_desc(VuDev *dev,
     }
 
     *p_num_sg = num_sg;
+    return true;
 }
 
 static void *
@@ -2479,6 +2480,7 @@  vu_queue_map_desc(VuDev *dev, VuVirtq *vq, unsigned int idx, size_t sz)
     if (desc[i].flags & VRING_DESC_F_INDIRECT) {
         if (desc[i].len % sizeof(struct vring_desc)) {
             vu_panic(dev, "Invalid size for indirect buffer table");
+            return NULL;
         }
 
         /* loop over the indirect descriptor table */
@@ -2506,22 +2508,27 @@  vu_queue_map_desc(VuDev *dev, VuVirtq *vq, unsigned int idx, size_t sz)
     /* Collect all the descriptors */
     do {
         if (desc[i].flags & VRING_DESC_F_WRITE) {
-            virtqueue_map_desc(dev, &in_num, iov + out_num,
-                               VIRTQUEUE_MAX_SIZE - out_num, true,
-                               desc[i].addr, desc[i].len);
+            if (!virtqueue_map_desc(dev, &in_num, iov + out_num,
+                                    VIRTQUEUE_MAX_SIZE - out_num, true,
+                                    desc[i].addr, desc[i].len)) {
+                return NULL;
+            }
         } else {
             if (in_num) {
                 vu_panic(dev, "Incorrect order for descriptors");
                 return NULL;
             }
-            virtqueue_map_desc(dev, &out_num, iov,
-                               VIRTQUEUE_MAX_SIZE, false,
-                               desc[i].addr, desc[i].len);
+            if (!virtqueue_map_desc(dev, &out_num, iov,
+                                    VIRTQUEUE_MAX_SIZE, false,
+                                    desc[i].addr, desc[i].len)) {
+                return NULL;
+            }
         }
 
         /* If we've got too many, that implies a descriptor loop. */
         if ((in_num + out_num) > max) {
             vu_panic(dev, "Looped descriptor");
+            return NULL;
         }
         rc = virtqueue_read_next_desc(dev, desc, i, max, &i);
     } while (rc == VIRTQUEUE_READ_DESC_MORE);