[PATCH] staging: gasket Create a memory allocation path

[PATCH] staging: gasket Create a memory allocation path

[Ioannis Valasakis]

toddpoynor@google.com, benchan@chromium.org
Bcc: 
Subject: [PATCH] staging: gasket: Create a memory allocation  error handling
 path
Reply-To: 

Create an error handling path for memory allocation. It avoid repeating
the same assignments and returns the respective ENOMEM.

Signed-off-by: Ioannis Valasakis <code@wizofe.uk>
---
 drivers/staging/gasket/gasket_interrupt.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/gasket/gasket_interrupt.c b/drivers/staging/gasket/gasket_interrupt.c
index 1cfbc120f228..cdfb72af1c52 100644
--- a/drivers/staging/gasket/gasket_interrupt.c
+++ b/drivers/staging/gasket/gasket_interrupt.c
@@ -348,27 +348,21 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
 					       sizeof(struct msix_entry),
 					       GFP_KERNEL);
 	if (!interrupt_data->msix_entries) {
-		kfree(interrupt_data);
-		return -ENOMEM;
+		goto err_alloc;
 	}
 
 	interrupt_data->eventfd_ctxs = kcalloc(num_interrupts,
 					       sizeof(struct eventfd_ctx *),
 					       GFP_KERNEL);
 	if (!interrupt_data->eventfd_ctxs) {
-		kfree(interrupt_data->msix_entries);
-		kfree(interrupt_data);
-		return -ENOMEM;
+		goto err_alloc;
 	}
 
 	interrupt_data->interrupt_counts = kcalloc(num_interrupts,
 						   sizeof(ulong),
 						   GFP_KERNEL);
 	if (!interrupt_data->interrupt_counts) {
-		kfree(interrupt_data->eventfd_ctxs);
-		kfree(interrupt_data->msix_entries);
-		kfree(interrupt_data);
-		return -ENOMEM;
+		goto err_alloc;
 	}
 
 	switch (interrupt_data->type) {
@@ -402,6 +396,12 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
 				    interrupt_sysfs_attrs);
 
 	return 0;
+
+err_alloc:
+	kfree(interrupt_data);
+	kfree(interrupt_data->msix_entries);
+	kfree(interrupt_data->eventfd_ctxs);
+	return -ENOMEM;
 }
 
 static void
-- 
2.19.1

Re: [Outreachy kernel] [PATCH] staging: gasket Create a memory allocation path

[Vaishali Thakkar]

On Mon, Oct 29, 2018 at 7:37 PM Ioannis Valasakis <code@wizofe.uk> wrote:
>
> toddpoynor@google.com, benchan@chromium.org
> Bcc:
> Subject: [PATCH] staging: gasket: Create a memory allocation  error handling
>  path
> Reply-To:

Hi Ioannis,

This didn't work properly. Please send again and always check by sending
it to yourself first. :)

> Create an error handling path for memory allocation. It avoid repeating
> the same assignments and returns the respective ENOMEM.
>
> Signed-off-by: Ioannis Valasakis <code@wizofe.uk>
> ---
>  drivers/staging/gasket/gasket_interrupt.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/gasket/gasket_interrupt.c b/drivers/staging/gasket/gasket_interrupt.c
> index 1cfbc120f228..cdfb72af1c52 100644
> --- a/drivers/staging/gasket/gasket_interrupt.c
> +++ b/drivers/staging/gasket/gasket_interrupt.c
> @@ -348,27 +348,21 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
>                                                sizeof(struct msix_entry),
>                                                GFP_KERNEL);
>         if (!interrupt_data->msix_entries) {
> -               kfree(interrupt_data);
> -               return -ENOMEM;
> +               goto err_alloc;
>         }
>
>         interrupt_data->eventfd_ctxs = kcalloc(num_interrupts,
>                                                sizeof(struct eventfd_ctx *),
>                                                GFP_KERNEL);
>         if (!interrupt_data->eventfd_ctxs) {
> -               kfree(interrupt_data->msix_entries);
> -               kfree(interrupt_data);
> -               return -ENOMEM;
> +               goto err_alloc;
>         }
>
>         interrupt_data->interrupt_counts = kcalloc(num_interrupts,
>                                                    sizeof(ulong),
>                                                    GFP_KERNEL);
>         if (!interrupt_data->interrupt_counts) {
> -               kfree(interrupt_data->eventfd_ctxs);
> -               kfree(interrupt_data->msix_entries);
> -               kfree(interrupt_data);
> -               return -ENOMEM;
> +               goto err_alloc;
>         }
>
>         switch (interrupt_data->type) {
> @@ -402,6 +396,12 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
>                                     interrupt_sysfs_attrs);
>
>         return 0;
> +
> +err_alloc:
> +       kfree(interrupt_data);
> +       kfree(interrupt_data->msix_entries);
> +       kfree(interrupt_data->eventfd_ctxs);
> +       return -ENOMEM;
>  }
>
>  static void
> --
> 2.19.1
>
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20181029140731.GA18826%40kvasir.local.
> For more options, visit https://groups.google.com/d/optout.

Re: [Outreachy kernel] [PATCH] staging: gasket Create a memory allocation path

[Julia Lawall]

> +err_alloc:
> +	kfree(interrupt_data);
> +	kfree(interrupt_data->msix_entries);
> +	kfree(interrupt_data->eventfd_ctxs);
> +	return -ENOMEM;

Besides Vaishali's suggestion, could you also make specific tags for
specific cases?  Please first read this message from Dan Carpenter:

https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ

julia

Re: [PATCH] staging: gasket Create a memory allocation path

[Greg KH]

On Mon, Oct 29, 2018 at 02:07:31PM +0000, Ioannis Valasakis wrote:
> +err_alloc:
> +	kfree(interrupt_data);
> +	kfree(interrupt_data->msix_entries);
> +	kfree(interrupt_data->eventfd_ctxs);

Think about the three lines here, and try to find the bug :)

This is not going to work at all, sorry, please look up what kfree()
does.

thanks,

greg k-h

Re: [Outreachy kernel] [PATCH] staging: gasket Create a memory allocation path

[Sasha Levin]

On Mon, Oct 29, 2018 at 03:14:56PM +0100, Julia Lawall wrote:
>> +err_alloc:
>> +	kfree(interrupt_data);
>> +	kfree(interrupt_data->msix_entries);
>> +	kfree(interrupt_data->eventfd_ctxs);
>> +	return -ENOMEM;
>
>Besides Vaishali's suggestion, could you also make specific tags for
>specific cases?  Please first read this message from Dan Carpenter:
>
>https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ

You'll also notice that you made the very same mistake Dan describes in
his post :)

You first freed interrupt_data, and proceeded to try and free
interrupt_data->msix_entries, which will attempt to dereference
interrupt_data which you freed the line before.

--
Thanks,
Sasha

Re: [PATCH] staging: gasket Create a memory allocation path

[Ioannis Valasakis]

On Mon, Oct 29, 2018 at 03:31:16PM +0100, Greg KH wrote:
> On Mon, Oct 29, 2018 at 02:07:31PM +0000, Ioannis Valasakis wrote:
> > +err_alloc:
> > +	kfree(interrupt_data);
> > +	kfree(interrupt_data->msix_entries);
> > +	kfree(interrupt_data->eventfd_ctxs);
> 
> Think about the three lines here, and try to find the bug :)

Right, I'll check this. The article from Dan was very useful. Thanks
Julia, Greg and Sasha!

> This is not going to work at all, sorry, please look up what kfree()
> does.
> 
> thanks,
> 
> greg k-h

I am going to resubmit this when corrected. 

ta
ioannis