diff mbox

vhost: Fix vhostfd leak in error branch

Message ID 1417166789-1960-1-git-send-email-arei.gonglei@huawei.com
State New
Headers show

Commit Message

Gonglei (Arei) Nov. 28, 2014, 9:26 a.m. UTC 
From: Gonglei <arei.gonglei@huawei.com>

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/scsi/vhost-scsi.c | 1 +
 hw/virtio/vhost.c    | 2 ++
 2 files changed, 3 insertions(+)

Comments

Jason Wang Dec. 1, 2014, 5:03 a.m. UTC | #1 
On Fri, Nov 28, 2014 at 5:26 PM, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/scsi/vhost-scsi.c | 1 +
>  hw/virtio/vhost.c    | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index 308b393..dcb2bc5 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -233,6 +233,7 @@ static void vhost_scsi_realize(DeviceState *dev, 
> Error **errp)
>                                 vhost_dummy_handle_output);
>      if (err != NULL) {
>          error_propagate(errp, err);
> +        close(vhostfd);
>          return;
>      }
>  
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 5d7c40a..5a12861 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -817,10 +817,12 @@ int vhost_dev_init(struct vhost_dev *hdev, void 
> *opaque,
>      int i, r;
>  
>      if (vhost_set_backend_type(hdev, backend_type) < 0) {
> +        close((uintptr_t)opaque);
>          return -1;
>      }
>  
>      if (hdev->vhost_ops->vhost_backend_init(hdev, opaque) < 0) {
> +        close((uintptr_t)opaque);
>          return -errno;
>      }
>  

Patch looks fine.

I wonder whether setting errno and goto fail would be better here?
This will let vhost_backend_cleanup() to do the cleanup, e.g closeing
fd or purging queue (for vhost uesr).
Gonglei (Arei) Dec. 1, 2014, 6:27 a.m. UTC | #2 
On 2014/12/1 13:03, Jason Wang wrote:

> 
> 
> On Fri, Nov 28, 2014 at 5:26 PM, arei.gonglei@huawei.com wrote:
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> ---
>>  hw/scsi/vhost-scsi.c | 1 +
>>  hw/virtio/vhost.c    | 2 ++
>>  2 files changed, 3 insertions(+)
>>
>> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
>> index 308b393..dcb2bc5 100644
>> --- a/hw/scsi/vhost-scsi.c
>> +++ b/hw/scsi/vhost-scsi.c
>> @@ -233,6 +233,7 @@ static void vhost_scsi_realize(DeviceState *dev, 
>> Error **errp)
>>                                 vhost_dummy_handle_output);
>>      if (err != NULL) {
>>          error_propagate(errp, err);
>> +        close(vhostfd);
>>          return;
>>      }
>>  
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index 5d7c40a..5a12861 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -817,10 +817,12 @@ int vhost_dev_init(struct vhost_dev *hdev, void 
>> *opaque,
>>      int i, r;
>>  
>>      if (vhost_set_backend_type(hdev, backend_type) < 0) {
>> +        close((uintptr_t)opaque);
>>          return -1;
>>      }
>>  
>>      if (hdev->vhost_ops->vhost_backend_init(hdev, opaque) < 0) {
>> +        close((uintptr_t)opaque);
>>          return -errno;
>>      }
>>  
> 
> Patch looks fine.
> 
> I wonder whether setting errno and goto fail would be better here?
> This will let vhost_backend_cleanup() to do the cleanup, e.g closeing
> fd or purging queue (for vhost uesr).
> 

Hi, Jason
Actually, vhost_backend_init() can not fail for both vhost-usr
and vhost-backend-type-kernel  at present. Besides, vhost-usr'
s vhost_backend_cleanup() just set dev->opaque to 0,
don't purge queues.

Regards,
-Gonglei
Jason Wang Dec. 1, 2014, 7:52 a.m. UTC | #3 
On Mon, Dec 1, 2014 at 2:27 PM, Gonglei <arei.gonglei@huawei.com> wrote:
> On 2014/12/1 13:03, Jason Wang wrote:
> 
>>  
>>  
>>  On Fri, Nov 28, 2014 at 5:26 PM, arei.gonglei@huawei.com wrote:
>>>  From: Gonglei <arei.gonglei@huawei.com>
>>> 
>>>  Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>>>  ---
>>>   hw/scsi/vhost-scsi.c | 1 +
>>>   hw/virtio/vhost.c    | 2 ++
>>>   2 files changed, 3 insertions(+)
>>> 
>>>  diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
>>>  index 308b393..dcb2bc5 100644
>>>  --- a/hw/scsi/vhost-scsi.c
>>>  +++ b/hw/scsi/vhost-scsi.c
>>>  @@ -233,6 +233,7 @@ static void vhost_scsi_realize(DeviceState 
>>> *dev, 
>>>  Error **errp)
>>>                                  vhost_dummy_handle_output);
>>>       if (err != NULL) {
>>>           error_propagate(errp, err);
>>>  +        close(vhostfd);
>>>           return;
>>>       }
>>>   
>>>  diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>>>  index 5d7c40a..5a12861 100644
>>>  --- a/hw/virtio/vhost.c
>>>  +++ b/hw/virtio/vhost.c
>>>  @@ -817,10 +817,12 @@ int vhost_dev_init(struct vhost_dev *hdev, 
>>> void 
>>>  *opaque,
>>>       int i, r;
>>>   
>>>       if (vhost_set_backend_type(hdev, backend_type) < 0) {
>>>  +        close((uintptr_t)opaque);
>>>           return -1;
>>>       }
>>>   
>>>       if (hdev->vhost_ops->vhost_backend_init(hdev, opaque) < 0) {
>>>  +        close((uintptr_t)opaque);
>>>           return -errno;
>>>       }
>>>   
>>  
>>  Patch looks fine.
>>  
>>  I wonder whether setting errno and goto fail would be better here?
>>  This will let vhost_backend_cleanup() to do the cleanup, e.g 
>> closeing
>>  fd or purging queue (for vhost uesr).
>>  
> 
> Hi, Jason
> Actually, vhost_backend_init() can not fail for both vhost-usr
> and vhost-backend-type-kernel  at present. Besides, vhost-usr'
> s vhost_backend_cleanup() just set dev->opaque to 0,
> don't purge queues.


I see, thanks for explaining.

Reviewed-by: Jason Wang <jasowang@redhat.com>
Michael S. Tsirkin Dec. 1, 2014, 9:37 a.m. UTC | #4 
On Fri, Nov 28, 2014 at 05:26:29PM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>

Peter, could you pick this up for 2.2 please?

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>


> ---
>  hw/scsi/vhost-scsi.c | 1 +
>  hw/virtio/vhost.c    | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index 308b393..dcb2bc5 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -233,6 +233,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
>                                 vhost_dummy_handle_output);
>      if (err != NULL) {
>          error_propagate(errp, err);
> +        close(vhostfd);
>          return;
>      }
>  
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 5d7c40a..5a12861 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -817,10 +817,12 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>      int i, r;
>  
>      if (vhost_set_backend_type(hdev, backend_type) < 0) {
> +        close((uintptr_t)opaque);
>          return -1;
>      }
>  
>      if (hdev->vhost_ops->vhost_backend_init(hdev, opaque) < 0) {
> +        close((uintptr_t)opaque);
>          return -errno;
>      }
>  
> -- 
> 1.7.12.4
>
Peter Maydell Dec. 1, 2014, 1:23 p.m. UTC | #5 
On 1 December 2014 at 09:37, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Fri, Nov 28, 2014 at 05:26:29PM +0800, arei.gonglei@huawei.com wrote:
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>
> Peter, could you pick this up for 2.2 please?
>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Applied, thanks.

-- PMM
diff mbox

Patch

diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 308b393..dcb2bc5 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -233,6 +233,7 @@  static void vhost_scsi_realize(DeviceState *dev, Error **errp)
                                vhost_dummy_handle_output);
     if (err != NULL) {
         error_propagate(errp, err);
+        close(vhostfd);
         return;
     }
 
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 5d7c40a..5a12861 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -817,10 +817,12 @@  int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
     int i, r;
 
     if (vhost_set_backend_type(hdev, backend_type) < 0) {
+        close((uintptr_t)opaque);
         return -1;
     }
 
     if (hdev->vhost_ops->vhost_backend_init(hdev, opaque) < 0) {
+        close((uintptr_t)opaque);
         return -errno;
     }