2022-03-15 22:18:03

by Xiaoke Wang

Subject: [PATCH v2] staging: mmal-vchiq: add a check for the return of vmalloc()

From: Xiaoke Wang <[email protected]>

vmalloc() is a memory allocation API which can return NULL when some
internal memory errors happen. So it is better to check the return
value of it to catch the error in time.

Signed-off-by: Xiaoke Wang <[email protected]>
---
ChangeLog:
v1->v2 jump to the proper location and remove redundant instruction.
drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
index 76d3f03..ff4b484 100644
--- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
+++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
@@ -1909,6 +1909,10 @@ int vchiq_mmal_init(struct vchiq_mmal_instance **out_instance)
mutex_init(&instance->vchiq_mutex);

instance->bulk_scratch = vmalloc(PAGE_SIZE);
+ if (!instance->bulk_scratch) {
+ err = -ENOMEM;
+ goto err_free;
+ }
instance->vchiq_instance = vchiq_instance;

mutex_init(&instance->context_map_lock);
--

2022-03-16 16:08:04

by Greg Kroah-Hartman

Subject: Re: [PATCH v2] staging: mmal-vchiq: add a check for the return of vmalloc()

On Tue, Mar 15, 2022 at 10:42:07PM +0800, [email protected] wrote:
> From: Xiaoke Wang <[email protected]>
>
> vmalloc() is a memory allocation API which can return NULL when some
> internal memory errors happen. So it is better to check the return
> value of it to catch the error in time.
>
> Signed-off-by: Xiaoke Wang <[email protected]>
> ---
> ChangeLog:
> v1->v2 jump to the proper location and remove redundant instruction.
> drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> index 76d3f03..ff4b484 100644
> --- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> +++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> @@ -1909,6 +1909,10 @@ int vchiq_mmal_init(struct vchiq_mmal_instance **out_instance)
> mutex_init(&instance->vchiq_mutex);
>
> instance->bulk_scratch = vmalloc(PAGE_SIZE);
> + if (!instance->bulk_scratch) {
> + err = -ENOMEM;
> + goto err_free;
> + }
> instance->vchiq_instance = vchiq_instance;
>
> mutex_init(&instance->context_map_lock);
> --

Your change just crashed the kernel :(

Please be more careful.

greg k-h

2022-03-17 03:42:06

by Greg Kroah-Hartman

Subject: Re: [PATCH v2] staging: mmal-vchiq: add a check for the return of vmalloc()

On Wed, Mar 16, 2022 at 12:23:05AM +0800, Xiaoke Wang wrote:
> On Tue, 15 Mar 2022 16:27:38 +0100, Greg KH wrote:
> &gt; Your change just crashed the kernel :(
> &gt;
> &gt; Please be more careful
>
> I am sorry. I ever been told that vfree(NULL) or kfree(NULL) is safe,

Sorry, you are right, I was thinking that there would be an error value
there. My mistake.

greg k-h

2022-03-17 03:45:32

by Xiaoke Wang

Subject: Re: [PATCH v2] staging: mmal-vchiq: add a check for the return of vmalloc()

On Tue, 15 Mar 2022 16:27:38 +0100, Greg KH wrote:
&gt; Your change just crashed the kernel :(
&gt;
&gt; Please be more careful

I am sorry. I ever been told that vfree(NULL) or kfree(NULL) is safe,
so I just jump to the tag `err_free` in this version.
I will resend one by adding a new proper tag.
And in fact, I did not find where -&gt;bulk_scratch is used.

Regards,
Xiaoke Wang

2022-03-17 04:03:40

by Dan Carpenter

Subject: Re: [PATCH v2] staging: mmal-vchiq: add a check for the return of vmalloc()

On Tue, Mar 15, 2022 at 04:27:38PM +0100, Greg KH wrote:
> On Tue, Mar 15, 2022 at 10:42:07PM +0800, [email protected] wrote:
> > From: Xiaoke Wang <[email protected]>
> >
> > vmalloc() is a memory allocation API which can return NULL when some
> > internal memory errors happen. So it is better to check the return
> > value of it to catch the error in time.
> >
> > Signed-off-by: Xiaoke Wang <[email protected]>
> > ---
> > ChangeLog:
> > v1->v2 jump to the proper location and remove redundant instruction.
> > drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> > index 76d3f03..ff4b484 100644
> > --- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> > +++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c
> > @@ -1909,6 +1909,10 @@ int vchiq_mmal_init(struct vchiq_mmal_instance **out_instance)
> > mutex_init(&instance->vchiq_mutex);
> >
> > instance->bulk_scratch = vmalloc(PAGE_SIZE);
> > + if (!instance->bulk_scratch) {

Is ->bulk_scratch even used anywhere?

regards,
dan carpenter