[PATCH] hw: virtio-mem: detach the element fromt the virtqueue when error occurs

[PATCH] hw: virtio-mem: detach the element fromt the virtqueue when error occurs

[Li Qiang]

If error occurs while processing the virtio request we should call
'virtqueue_detach_element' to detach the element from the virtqueue
before free the elem.

Signed-off-by: Li Qiang <liq3ea@163.com>
---
 hw/virtio/virtio-mem.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index 7740fc613f..5ac6c3ec67 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -318,8 +318,7 @@ static void virtio_mem_handle_request(VirtIODevice *vdev, VirtQueue *vq)
         if (iov_to_buf(elem->out_sg, elem->out_num, 0, &req, len) < len) {
             virtio_error(vdev, "virtio-mem protocol violation: invalid request"
                          " size: %d", len);
-            g_free(elem);
-            return;
+            goto out_free;
         }
 
         if (iov_size(elem->in_sg, elem->in_num) <
@@ -327,8 +326,7 @@ static void virtio_mem_handle_request(VirtIODevice *vdev, VirtQueue *vq)
             virtio_error(vdev, "virtio-mem protocol violation: not enough space"
                          " for response: %zu",
                          iov_size(elem->in_sg, elem->in_num));
-            g_free(elem);
-            return;
+            goto out_free;
         }
 
         type = le16_to_cpu(req.type);
@@ -348,12 +346,15 @@ static void virtio_mem_handle_request(VirtIODevice *vdev, VirtQueue *vq)
         default:
             virtio_error(vdev, "virtio-mem protocol violation: unknown request"
                          " type: %d", type);
-            g_free(elem);
-            return;
+            goto out_free;
         }
 
         g_free(elem);
     }
+
+out_free:
+    virtqueue_detach_element(vq, elem, 0);
+    g_free(elem);
 }
 
 static void virtio_mem_get_config(VirtIODevice *vdev, uint8_t *config_data)
-- 
2.17.1

Re: [PATCH] hw: virtio-mem: detach the element fromt the virtqueue when error occurs

[David Hildenbrand]

On 13.08.20 18:46, Li Qiang wrote:

For now we use "virtio-mem:" for the subject, without the "hw: "part.

> If error occurs while processing the virtio request we should call
> 'virtqueue_detach_element' to detach the element from the virtqueue
> before free the elem.

What's the effect of this? In all cases we trigger a virtio_error(), so
do we really have to bother?

> 
> Signed-off-by: Li Qiang <liq3ea@163.com>
> ---
>  hw/virtio/virtio-mem.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
> index 7740fc613f..5ac6c3ec67 100644
> --- a/hw/virtio/virtio-mem.c
> +++ b/hw/virtio/virtio-mem.c
> @@ -318,8 +318,7 @@ static void virtio_mem_handle_request(VirtIODevice *vdev, VirtQueue *vq)
>          if (iov_to_buf(elem->out_sg, elem->out_num, 0, &req, len) < len) {
>              virtio_error(vdev, "virtio-mem protocol violation: invalid request"
>                           " size: %d", len);
> -            g_free(elem);
> -            return;
> +            goto out_free;
>          }
>  
>          if (iov_size(elem->in_sg, elem->in_num) <
> @@ -327,8 +326,7 @@ static void virtio_mem_handle_request(VirtIODevice *vdev, VirtQueue *vq)
>              virtio_error(vdev, "virtio-mem protocol violation: not enough space"
>                           " for response: %zu",
>                           iov_size(elem->in_sg, elem->in_num));
> -            g_free(elem);
> -            return;
> +            goto out_free;
>          }
>  
>          type = le16_to_cpu(req.type);
> @@ -348,12 +346,15 @@ static void virtio_mem_handle_request(VirtIODevice *vdev, VirtQueue *vq)
>          default:
>              virtio_error(vdev, "virtio-mem protocol violation: unknown request"
>                           " type: %d", type);
> -            g_free(elem);
> -            return;
> +            goto out_free;
>          }
>  
>          g_free(elem);
>      }
> +
> +out_free:
> +    virtqueue_detach_element(vq, elem, 0);
> +    g_free(elem);
>  }
>  
>  static void virtio_mem_get_config(VirtIODevice *vdev, uint8_t *config_data)
> 


-- 
Thanks,

David / dhildenb

Re: [PATCH] hw: virtio-mem: detach the element fromt the virtqueue when error occurs

[Li Qiang]

David Hildenbrand <david@redhat.com> 于2020年8月14日周五 上午1:15写道：
>
> On 13.08.20 18:46, Li Qiang wrote:
>
> For now we use "virtio-mem:" for the subject, without the "hw: "part.
>
> > If error occurs while processing the virtio request we should call
> > 'virtqueue_detach_element' to detach the element from the virtqueue
> > before free the elem.
>
> What's the effect of this? In all cases we trigger a virtio_error(), so
> do we really have to bother?
>

Though the 'in_use' will be reset to 0 while reseting the virtio device.
The mapped sglist will not be unammped.
There maybe some undesired behavior.  CC Paolo to make a confirmation.

Thanks,
Li Qiang

> >
> > Signed-off-by: Li Qiang <liq3ea@163.com>
> > ---
> >  hw/virtio/virtio-mem.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
> > index 7740fc613f..5ac6c3ec67 100644
> > --- a/hw/virtio/virtio-mem.c
> > +++ b/hw/virtio/virtio-mem.c
> > @@ -318,8 +318,7 @@ static void virtio_mem_handle_request(VirtIODevice *vdev, VirtQueue *vq)
> >          if (iov_to_buf(elem->out_sg, elem->out_num, 0, &req, len) < len) {
> >              virtio_error(vdev, "virtio-mem protocol violation: invalid request"
> >                           " size: %d", len);
> > -            g_free(elem);
> > -            return;
> > +            goto out_free;
> >          }
> >
> >          if (iov_size(elem->in_sg, elem->in_num) <
> > @@ -327,8 +326,7 @@ static void virtio_mem_handle_request(VirtIODevice *vdev, VirtQueue *vq)
> >              virtio_error(vdev, "virtio-mem protocol violation: not enough space"
> >                           " for response: %zu",
> >                           iov_size(elem->in_sg, elem->in_num));
> > -            g_free(elem);
> > -            return;
> > +            goto out_free;
> >          }
> >
> >          type = le16_to_cpu(req.type);
> > @@ -348,12 +346,15 @@ static void virtio_mem_handle_request(VirtIODevice *vdev, VirtQueue *vq)
> >          default:
> >              virtio_error(vdev, "virtio-mem protocol violation: unknown request"
> >                           " type: %d", type);
> > -            g_free(elem);
> > -            return;
> > +            goto out_free;
> >          }
> >
> >          g_free(elem);
> >      }
> > +
> > +out_free:
> > +    virtqueue_detach_element(vq, elem, 0);
> > +    g_free(elem);
> >  }
> >
> >  static void virtio_mem_get_config(VirtIODevice *vdev, uint8_t *config_data)
> >
>
>
> --
> Thanks,
>
> David / dhildenb
>

Re: [PATCH] hw: virtio-mem: detach the element fromt the virtqueue when error occurs

[David Hildenbrand]

On 14.08.20 03:01, Li Qiang wrote:
> David Hildenbrand <david@redhat.com> 于2020年8月14日周五 上午1:15写道：
>>
>> On 13.08.20 18:46, Li Qiang wrote:
>>
>> For now we use "virtio-mem:" for the subject, without the "hw: "part.
>>
>>> If error occurs while processing the virtio request we should call
>>> 'virtqueue_detach_element' to detach the element from the virtqueue
>>> before free the elem.
>>
>> What's the effect of this? In all cases we trigger a virtio_error(), so
>> do we really have to bother?
>>
> 
> Though the 'in_use' will be reset to 0 while reseting the virtio device.
> The mapped sglist will not be unammped.
> There maybe some undesired behavior.  CC Paolo to make a confirmation.

Looking at hw/virtio/virtio-crypto.c, this seems to be the right thing
to do.

Can you please respin, avoiding adding the label, only inserting the 3
separate virtqueue_detach_element() calls?

Thanks!

-- 
Thanks,

David / dhildenb