1. Patchew
  2. QEMU
  3. View series

[PATCH] virtio: vdpa: omit check return of g_malloc

Li Qiang posted 1 patch 3 years, 8 months ago
Test docker-quick@centos7 passed
Test docker-mingw@fedora passed
Test checkpatch passed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200819144309.67579-1-liq3ea@163.com
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>
hw/virtio/vhost-vdpa.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
[PATCH] virtio: vdpa: omit check return of g_malloc
Posted by Li Qiang 3 years, 8 months ago 
If g_malloc fails, the application will be terminated.
No need to check the return value of g_malloc.

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

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 4580f3efd8..403ae3ae07 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -320,10 +320,8 @@ static int vhost_vdpa_set_config(struct vhost_dev *dev, const uint8_t *data,
     struct vhost_vdpa_config *config;
     int ret;
     unsigned long config_size = offsetof(struct vhost_vdpa_config, buf);
+
     config = g_malloc(size + config_size);
-    if (config == NULL) {
-        return -1;
-    }
     config->off = offset;
     config->len = size;
     memcpy(config->buf, data, size);
@@ -340,9 +338,6 @@ static int vhost_vdpa_get_config(struct vhost_dev *dev, uint8_t *config,
     int ret;
 
     v_config = g_malloc(config_len + config_size);
-    if (v_config == NULL) {
-        return -1;
-    }
     v_config->len = config_len;
     v_config->off = 0;
     ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_CONFIG, v_config);
-- 
2.17.1
Re: [PATCH] virtio: vdpa: omit check return of g_malloc
Posted by Philippe Mathieu-Daudé 3 years, 8 months ago 
On 8/19/20 4:43 PM, Li Qiang wrote:
> If g_malloc fails, the application will be terminated.

Which we don't want... better to use g_try_malloc() instead?

> No need to check the return value of g_malloc.
> 
> Signed-off-by: Li Qiang <liq3ea@163.com>
> ---
>  hw/virtio/vhost-vdpa.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 4580f3efd8..403ae3ae07 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -320,10 +320,8 @@ static int vhost_vdpa_set_config(struct vhost_dev *dev, const uint8_t *data,
>      struct vhost_vdpa_config *config;
>      int ret;
>      unsigned long config_size = offsetof(struct vhost_vdpa_config, buf);
> +
>      config = g_malloc(size + config_size);
> -    if (config == NULL) {
> -        return -1;
> -    }
>      config->off = offset;
>      config->len = size;
>      memcpy(config->buf, data, size);
> @@ -340,9 +338,6 @@ static int vhost_vdpa_get_config(struct vhost_dev *dev, uint8_t *config,
>      int ret;
>  
>      v_config = g_malloc(config_len + config_size);
> -    if (v_config == NULL) {
> -        return -1;
> -    }
>      v_config->len = config_len;
>      v_config->off = 0;
>      ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_CONFIG, v_config);
>
Re: [PATCH] virtio: vdpa: omit check return of g_malloc
Posted by Li Qiang 3 years, 8 months ago 
Philippe Mathieu-Daudé <philmd@redhat.com> 于2020年8月19日周三 下午11:07写道：
>
> On 8/19/20 4:43 PM, Li Qiang wrote:
> > If g_malloc fails, the application will be terminated.
>
> Which we don't want... better to use g_try_malloc() instead?

I don't think so. If g_malloc return NULL it means a critical
situation I think terminate the application
is OK. Though I don't find any rule/practices the qemu code base uses
g_malloc far more than
g_try_malloc.

Thanks,
Li Qiang

>
> > No need to check the return value of g_malloc.
> >
> > Signed-off-by: Li Qiang <liq3ea@163.com>
> > ---
> >  hw/virtio/vhost-vdpa.c | 7 +------
> >  1 file changed, 1 insertion(+), 6 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 4580f3efd8..403ae3ae07 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -320,10 +320,8 @@ static int vhost_vdpa_set_config(struct vhost_dev *dev, const uint8_t *data,
> >      struct vhost_vdpa_config *config;
> >      int ret;
> >      unsigned long config_size = offsetof(struct vhost_vdpa_config, buf);
> > +
> >      config = g_malloc(size + config_size);
> > -    if (config == NULL) {
> > -        return -1;
> > -    }
> >      config->off = offset;
> >      config->len = size;
> >      memcpy(config->buf, data, size);
> > @@ -340,9 +338,6 @@ static int vhost_vdpa_get_config(struct vhost_dev *dev, uint8_t *config,
> >      int ret;
> >
> >      v_config = g_malloc(config_len + config_size);
> > -    if (v_config == NULL) {
> > -        return -1;
> > -    }
> >      v_config->len = config_len;
> >      v_config->off = 0;
> >      ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_CONFIG, v_config);
> >
>
Re: [PATCH] virtio: vdpa: omit check return of g_malloc
Posted by Alex Bennée 3 years, 7 months ago 
Li Qiang <liq3ea@gmail.com> writes:

> Philippe Mathieu-Daudé <philmd@redhat.com> 于2020年8月19日周三 下午11:07写道：
>>
>> On 8/19/20 4:43 PM, Li Qiang wrote:
>> > If g_malloc fails, the application will be terminated.
>>
>> Which we don't want... better to use g_try_malloc() instead?
>
> I don't think so. If g_malloc return NULL it means a critical
> situation I think terminate the application
> is OK. Though I don't find any rule/practices the qemu code base uses
> g_malloc far more than
> g_try_malloc.

g_try_malloc is only for cases you could recover from, by either
deferring or doing something else. A straight out of memory failure is
fatal.

Arguably a bunch of the try_malloc's in the code base should be straight
mallocs. The ELF loaders load_symbols does it because I guess having the
symbols is a bonus and you could still run the program if a) there was
enough memory to run and b) your symbol table was very large.

>
> Thanks,
> Li Qiang
>
>>
>> > No need to check the return value of g_malloc.
>> >
>> > Signed-off-by: Li Qiang <liq3ea@163.com>
>> > ---
>> >  hw/virtio/vhost-vdpa.c | 7 +------
>> >  1 file changed, 1 insertion(+), 6 deletions(-)
>> >
>> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>> > index 4580f3efd8..403ae3ae07 100644
>> > --- a/hw/virtio/vhost-vdpa.c
>> > +++ b/hw/virtio/vhost-vdpa.c
>> > @@ -320,10 +320,8 @@ static int vhost_vdpa_set_config(struct vhost_dev *dev, const uint8_t *data,
>> >      struct vhost_vdpa_config *config;
>> >      int ret;
>> >      unsigned long config_size = offsetof(struct vhost_vdpa_config, buf);
>> > +
>> >      config = g_malloc(size + config_size);
>> > -    if (config == NULL) {
>> > -        return -1;
>> > -    }
>> >      config->off = offset;
>> >      config->len = size;
>> >      memcpy(config->buf, data, size);
>> > @@ -340,9 +338,6 @@ static int vhost_vdpa_get_config(struct vhost_dev *dev, uint8_t *config,
>> >      int ret;
>> >
>> >      v_config = g_malloc(config_len + config_size);
>> > -    if (v_config == NULL) {
>> > -        return -1;
>> > -    }
>> >      v_config->len = config_len;
>> >      v_config->off = 0;
>> >      ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_CONFIG, v_config);
>> >
>>


-- 
Alex Bennée
Re: [PATCH] virtio: vdpa: omit check return of g_malloc
Posted by Eric Blake 3 years, 7 months ago 
On 9/18/20 8:12 AM, Alex Bennée wrote:
> 
> Li Qiang <liq3ea@gmail.com> writes:
> 
>> Philippe Mathieu-Daudé <philmd@redhat.com> 于2020年8月19日周三 下午11:07写道：
>>>
>>> On 8/19/20 4:43 PM, Li Qiang wrote:
>>>> If g_malloc fails, the application will be terminated.
>>>
>>> Which we don't want... better to use g_try_malloc() instead?
>>
>> I don't think so. If g_malloc return NULL it means a critical
>> situation I think terminate the application
>> is OK. Though I don't find any rule/practices the qemu code base uses
>> g_malloc far more than
>> g_try_malloc.
> 
> g_try_malloc is only for cases you could recover from, by either
> deferring or doing something else. A straight out of memory failure is
> fatal.
> 
> Arguably a bunch of the try_malloc's in the code base should be straight
> mallocs. The ELF loaders load_symbols does it because I guess having the
> symbols is a bonus and you could still run the program if a) there was
> enough memory to run and b) your symbol table was very large.

If the amount of memory being allocated is under user control (such as 
from an elf header or qcow2 image), you want g_try_malloc() to prevent 
against malicious users requesting outrageous amounts.  But if it is 
solely under programmer control, where the maximum amount requested is 
not going to be a problem unless you are already truly out of memory for 
other reasons, g_malloc() is appropriate.  A potential rule of thumb 
might be that if you know it is less than 1M of memory, it's not worth 
trying to recover from.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org
Re: [PATCH] virtio: vdpa: omit check return of g_malloc
Posted by Laurent Vivier 3 years, 7 months ago 
On 19/08/2020 16:43, Li Qiang wrote:
> If g_malloc fails, the application will be terminated.
> No need to check the return value of g_malloc.
> 
> Signed-off-by: Li Qiang <liq3ea@163.com>
> ---
>  hw/virtio/vhost-vdpa.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 4580f3efd8..403ae3ae07 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -320,10 +320,8 @@ static int vhost_vdpa_set_config(struct vhost_dev *dev, const uint8_t *data,
>      struct vhost_vdpa_config *config;
>      int ret;
>      unsigned long config_size = offsetof(struct vhost_vdpa_config, buf);
> +
>      config = g_malloc(size + config_size);
> -    if (config == NULL) {
> -        return -1;
> -    }
>      config->off = offset;
>      config->len = size;
>      memcpy(config->buf, data, size);
> @@ -340,9 +338,6 @@ static int vhost_vdpa_get_config(struct vhost_dev *dev, uint8_t *config,
>      int ret;
>  
>      v_config = g_malloc(config_len + config_size);
> -    if (v_config == NULL) {
> -        return -1;
> -    }
>      v_config->len = config_len;
>      v_config->off = 0;
>      ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_CONFIG, v_config);
> 

Reviewed-by: Laurent Vivier <lvivier@redhat.com>
Re: [PATCH] virtio: vdpa: omit check return of g_malloc
Posted by Laurent Vivier 3 years, 7 months ago 
Le 18/09/2020 à 14:53, Laurent Vivier a écrit :
> On 19/08/2020 16:43, Li Qiang wrote:
>> If g_malloc fails, the application will be terminated.
>> No need to check the return value of g_malloc.
>>
>> Signed-off-by: Li Qiang <liq3ea@163.com>
>> ---
>>  hw/virtio/vhost-vdpa.c | 7 +------
>>  1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>> index 4580f3efd8..403ae3ae07 100644
>> --- a/hw/virtio/vhost-vdpa.c
>> +++ b/hw/virtio/vhost-vdpa.c
>> @@ -320,10 +320,8 @@ static int vhost_vdpa_set_config(struct vhost_dev *dev, const uint8_t *data,
>>      struct vhost_vdpa_config *config;
>>      int ret;
>>      unsigned long config_size = offsetof(struct vhost_vdpa_config, buf);
>> +
>>      config = g_malloc(size + config_size);
>> -    if (config == NULL) {
>> -        return -1;
>> -    }
>>      config->off = offset;
>>      config->len = size;
>>      memcpy(config->buf, data, size);
>> @@ -340,9 +338,6 @@ static int vhost_vdpa_get_config(struct vhost_dev *dev, uint8_t *config,
>>      int ret;
>>  
>>      v_config = g_malloc(config_len + config_size);
>> -    if (v_config == NULL) {
>> -        return -1;
>> -    }
>>      v_config->len = config_len;
>>      v_config->off = 0;
>>      ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_CONFIG, v_config);
>>
> 
> Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> 
> 

Applied to my trivial-patches branch.

Thanks,
Laurent
Re: [PATCH] virtio: vdpa: omit check return of g_malloc
Posted by Alex Bennée 3 years, 7 months ago 
Li Qiang <liq3ea@163.com> writes:

> If g_malloc fails, the application will be terminated.
> No need to check the return value of g_malloc.
>
> Signed-off-by: Li Qiang <liq3ea@163.com>

I think this is fine because a failure to config the dev would just
cause an error_report later and it's extremely unlikely you don't have
enough memory to get that far anyway.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.