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

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

[Ioannis Valasakis]

Fix multiple error allocation path by using re-direction.

Signed-off-by: Ioannis Valasakis <code@wizofe.uk>
---
Changes in v2:
	- Specific tags for memory allocation
	- Free the memory in the right way as for kfre

drivers/staging/gasket/gasket_interrupt.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/gasket/gasket_interrupt.c b/drivers/staging/gasket/gasket_interrupt.c
index cdfb72af1c52..23232ea8f4d9 100644
--- a/drivers/staging/gasket/gasket_interrupt.c
+++ b/drivers/staging/gasket/gasket_interrupt.c
@@ -348,21 +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) {
-		goto err_alloc;
+		goto err_free_msix;
 	}
 
 	interrupt_data->eventfd_ctxs = kcalloc(num_interrupts,
 					       sizeof(struct eventfd_ctx *),
 					       GFP_KERNEL);
 	if (!interrupt_data->eventfd_ctxs) {
-		goto err_alloc;
+		goto err_free_eventfd;
 	}
 
 	interrupt_data->interrupt_counts = kcalloc(num_interrupts,
 						   sizeof(ulong),
 						   GFP_KERNEL);
 	if (!interrupt_data->interrupt_counts) {
-		goto err_alloc;
+		goto err_free_data;
 	}
 
 	switch (interrupt_data->type) {
@@ -397,10 +397,12 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
 
 	return 0;
 
-err_alloc:
-	kfree(interrupt_data);
-	kfree(interrupt_data->msix_entries);
+err_free_msix:
 	kfree(interrupt_data->eventfd_ctxs);
+err_free_eventfd:
+	kfree(interrupt_data->msix_entries);
+err_free_data:
+	kfree(interrupt_data);
 	return -ENOMEM;
 }
 
-- 
2.19.1

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

[Sasha Levin]

On Wed, Oct 31, 2018 at 04:47:54PM +0000, Ioannis Valasakis wrote:
>Fix multiple error allocation path by using re-direction.
>
>Signed-off-by: Ioannis Valasakis <code@wizofe.uk>
>---
>Changes in v2:
>	- Specific tags for memory allocation
>	- Free the memory in the right way as for kfre
>
>drivers/staging/gasket/gasket_interrupt.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/staging/gasket/gasket_interrupt.c b/drivers/staging/gasket/gasket_interrupt.c
>index cdfb72af1c52..23232ea8f4d9 100644
>--- a/drivers/staging/gasket/gasket_interrupt.c
>+++ b/drivers/staging/gasket/gasket_interrupt.c
>@@ -348,21 +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) {
>-		goto err_alloc;
>+		goto err_free_msix;
> 	}
>
> 	interrupt_data->eventfd_ctxs = kcalloc(num_interrupts,
> 					       sizeof(struct eventfd_ctx *),
> 					       GFP_KERNEL);
> 	if (!interrupt_data->eventfd_ctxs) {
>-		goto err_alloc;
>+		goto err_free_eventfd;
> 	}
>
> 	interrupt_data->interrupt_counts = kcalloc(num_interrupts,
> 						   sizeof(ulong),
> 						   GFP_KERNEL);
> 	if (!interrupt_data->interrupt_counts) {
>-		goto err_alloc;
>+		goto err_free_data;
> 	}
>
> 	switch (interrupt_data->type) {
>@@ -397,10 +397,12 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
>
> 	return 0;
>
>-err_alloc:
>-	kfree(interrupt_data);
>-	kfree(interrupt_data->msix_entries);
>+err_free_msix:
> 	kfree(interrupt_data->eventfd_ctxs);
>+err_free_eventfd:
>+	kfree(interrupt_data->msix_entries);
>+err_free_data:
>+	kfree(interrupt_data);
> 	return -ENOMEM;
> }

With this code, what will happen if, for example, the last
allocation (interrupt_data->interrupt_counts) fails?

--
Thanks,
Sasha

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

[Julia Lawall]

On Wed, 31 Oct 2018, Ioannis Valasakis wrote:

> Fix multiple error allocation path by using re-direction.

Beside the semantic problem that has already been pointed out, I find the
word "re-direction" a bit strange, or a bit vague.  What is redirected?
Could it be something like "Collect the common error handling code at the
end of the function"?

I also have the impression that this patch is on top of your previous
incorrect patch, instead of the original code.  You should always work
against the code in Greg's tree.

julia

>
> Signed-off-by: Ioannis Valasakis <code@wizofe.uk>
> ---
> Changes in v2:
> 	- Specific tags for memory allocation
> 	- Free the memory in the right way as for kfre
>
> drivers/staging/gasket/gasket_interrupt.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/gasket/gasket_interrupt.c b/drivers/staging/gasket/gasket_interrupt.c
> index cdfb72af1c52..23232ea8f4d9 100644
> --- a/drivers/staging/gasket/gasket_interrupt.c
> +++ b/drivers/staging/gasket/gasket_interrupt.c
> @@ -348,21 +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) {
> -		goto err_alloc;
> +		goto err_free_msix;
>  	}
>
>  	interrupt_data->eventfd_ctxs = kcalloc(num_interrupts,
>  					       sizeof(struct eventfd_ctx *),
>  					       GFP_KERNEL);
>  	if (!interrupt_data->eventfd_ctxs) {
> -		goto err_alloc;
> +		goto err_free_eventfd;
>  	}
>
>  	interrupt_data->interrupt_counts = kcalloc(num_interrupts,
>  						   sizeof(ulong),
>  						   GFP_KERNEL);
>  	if (!interrupt_data->interrupt_counts) {
> -		goto err_alloc;
> +		goto err_free_data;
>  	}
>
>  	switch (interrupt_data->type) {
> @@ -397,10 +397,12 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
>
>  	return 0;
>
> -err_alloc:
> -	kfree(interrupt_data);
> -	kfree(interrupt_data->msix_entries);
> +err_free_msix:
>  	kfree(interrupt_data->eventfd_ctxs);
> +err_free_eventfd:
> +	kfree(interrupt_data->msix_entries);
> +err_free_data:
> +	kfree(interrupt_data);
>  	return -ENOMEM;
>  }
>
> --
> 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/20181031164753.GA14140%40kvasir.local.
> For more options, visit https://groups.google.com/d/optout.
>

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

[Ioannis Valasakis]

On Wed, Oct 31, 2018 at 12:54:39PM -0400, Sasha Levin wrote:
> On Wed, Oct 31, 2018 at 04:47:54PM +0000, Ioannis Valasakis wrote:
> > Fix multiple error allocation path by using re-direction.
> > 
> > Signed-off-by: Ioannis Valasakis <code@wizofe.uk>
> > ---
> > Changes in v2:
> > 	- Specific tags for memory allocation
> > 	- Free the memory in the right way as for kfre
> > 
> > drivers/staging/gasket/gasket_interrupt.c | 14 ++++++++------
> > 1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/staging/gasket/gasket_interrupt.c b/drivers/staging/gasket/gasket_interrupt.c
> > index cdfb72af1c52..23232ea8f4d9 100644
> > --- a/drivers/staging/gasket/gasket_interrupt.c
> > +++ b/drivers/staging/gasket/gasket_interrupt.c
> > @@ -348,21 +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) {
> > -		goto err_alloc;
> > +		goto err_free_msix;
> > 	}
> > 
> > 	interrupt_data->eventfd_ctxs = kcalloc(num_interrupts,
> > 					       sizeof(struct eventfd_ctx *),
> > 					       GFP_KERNEL);
> > 	if (!interrupt_data->eventfd_ctxs) {
> > -		goto err_alloc;
> > +		goto err_free_eventfd;
> > 	}
> > 
> > 	interrupt_data->interrupt_counts = kcalloc(num_interrupts,
> > 						   sizeof(ulong),
> > 						   GFP_KERNEL);
> > 	if (!interrupt_data->interrupt_counts) {
> > -		goto err_alloc;
> > +		goto err_free_data;
> > 	}
> > 
> > 	switch (interrupt_data->type) {
> > @@ -397,10 +397,12 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
> > 
> > 	return 0;
> > 
> > -err_alloc:
> > -	kfree(interrupt_data);
> > -	kfree(interrupt_data->msix_entries);
> > +err_free_msix:
> > 	kfree(interrupt_data->eventfd_ctxs);
> > +err_free_eventfd:
> > +	kfree(interrupt_data->msix_entries);
> > +err_free_data:
> > +	kfree(interrupt_data);
> > 	return -ENOMEM;
> > }
> 
> With this code, what will happen if, for example, the last
> allocation (interrupt_data->interrupt_counts) fails?
It took my some time but I think I saw it; the mistake is that
if interrupt_data->interrupt_counts fails it should 'goto
err_free_msix'.

Same if interrupt_data->msix_entries fails it should 'goto
err_free_data'.

Is that right?

(I got very ambitious with the labels and I mixed it up ;) Any way to
make the goto labels make sense?)


> 
> --
> Thanks,
> Sasha

Best,
Ioannis

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

[Julia Lawall]

On Wed, 31 Oct 2018, Ioannis Valasakis wrote:

> On Wed, Oct 31, 2018 at 12:54:39PM -0400, Sasha Levin wrote:
> > On Wed, Oct 31, 2018 at 04:47:54PM +0000, Ioannis Valasakis wrote:
> > > Fix multiple error allocation path by using re-direction.
> > >
> > > Signed-off-by: Ioannis Valasakis <code@wizofe.uk>
> > > ---
> > > Changes in v2:
> > > 	- Specific tags for memory allocation
> > > 	- Free the memory in the right way as for kfre
> > >
> > > drivers/staging/gasket/gasket_interrupt.c | 14 ++++++++------
> > > 1 file changed, 8 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/staging/gasket/gasket_interrupt.c b/drivers/staging/gasket/gasket_interrupt.c
> > > index cdfb72af1c52..23232ea8f4d9 100644
> > > --- a/drivers/staging/gasket/gasket_interrupt.c
> > > +++ b/drivers/staging/gasket/gasket_interrupt.c
> > > @@ -348,21 +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) {
> > > -		goto err_alloc;
> > > +		goto err_free_msix;
> > > 	}
> > >
> > > 	interrupt_data->eventfd_ctxs = kcalloc(num_interrupts,
> > > 					       sizeof(struct eventfd_ctx *),
> > > 					       GFP_KERNEL);
> > > 	if (!interrupt_data->eventfd_ctxs) {
> > > -		goto err_alloc;
> > > +		goto err_free_eventfd;
> > > 	}
> > >
> > > 	interrupt_data->interrupt_counts = kcalloc(num_interrupts,
> > > 						   sizeof(ulong),
> > > 						   GFP_KERNEL);
> > > 	if (!interrupt_data->interrupt_counts) {
> > > -		goto err_alloc;
> > > +		goto err_free_data;
> > > 	}
> > >
> > > 	switch (interrupt_data->type) {
> > > @@ -397,10 +397,12 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
> > >
> > > 	return 0;
> > >
> > > -err_alloc:
> > > -	kfree(interrupt_data);
> > > -	kfree(interrupt_data->msix_entries);
> > > +err_free_msix:
> > > 	kfree(interrupt_data->eventfd_ctxs);
> > > +err_free_eventfd:
> > > +	kfree(interrupt_data->msix_entries);
> > > +err_free_data:
> > > +	kfree(interrupt_data);
> > > 	return -ENOMEM;
> > > }
> >
> > With this code, what will happen if, for example, the last
> > allocation (interrupt_data->interrupt_counts) fails?
> It took my some time but I think I saw it; the mistake is that
> if interrupt_data->interrupt_counts fails it should 'goto
> err_free_msix'.
>
> Same if interrupt_data->msix_entries fails it should 'goto
> err_free_data'.
>
> Is that right?
>
> (I got very ambitious with the labels and I mixed it up ;) Any way to
> make the goto labels make sense?)

I don't think that the choice of labels is a problem.  Basically, you
should normally release things in the opposite order of the one in which
they were allocated.  At the beginning of your function, you can just do a
return, because nothing has been allocated yet.  Then when you have
successfully allocated something, if you fail afterwards you should go to
the very end of the function, because you have only one thing to free.
When you have successfully allocated something else, if you fail
afterwards, you should jump not quite to the end, so that you free both
things that have been allocated, etc.

julia

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

[Ioannis Valasakis]

On Wed, Oct 31, 2018 at 09:40:47PM +0100, Julia Lawall wrote:
> 
> 
> On Wed, 31 Oct 2018, Ioannis Valasakis wrote:
> 
> > Fix multiple error allocation path by using re-direction.
> 
> Beside the semantic problem that has already been pointed out, I find the
> word "re-direction" a bit strange, or a bit vague.  What is redirected?
> Could it be something like "Collect the common error handling code at the
> end of the function"?
> 
> I also have the impression that this patch is on top of your previous
> incorrect patch, instead of the original code.  You should always work
> against the code in Greg's tree.
Thanks Julia. I am going to rebase on greg's staging and resend the
patch (ah fourth one!). Sorry that I am bothering all of you but in
every step I am learning something new. Thanks for being patient and
helpful!
> 
> julia
> 
> >
> > Signed-off-by: Ioannis Valasakis <code@wizofe.uk>
> > ---
> > Changes in v2:
> > 	- Specific tags for memory allocation
> > 	- Free the memory in the right way as for kfre
> >
> > drivers/staging/gasket/gasket_interrupt.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/staging/gasket/gasket_interrupt.c b/drivers/staging/gasket/gasket_interrupt.c
> > index cdfb72af1c52..23232ea8f4d9 100644
> > --- a/drivers/staging/gasket/gasket_interrupt.c
> > +++ b/drivers/staging/gasket/gasket_interrupt.c
> > @@ -348,21 +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) {
> > -		goto err_alloc;
> > +		goto err_free_msix;
> >  	}
> >
> >  	interrupt_data->eventfd_ctxs = kcalloc(num_interrupts,
> >  					       sizeof(struct eventfd_ctx *),
> >  					       GFP_KERNEL);
> >  	if (!interrupt_data->eventfd_ctxs) {
> > -		goto err_alloc;
> > +		goto err_free_eventfd;
> >  	}
> >
> >  	interrupt_data->interrupt_counts = kcalloc(num_interrupts,
> >  						   sizeof(ulong),
> >  						   GFP_KERNEL);
> >  	if (!interrupt_data->interrupt_counts) {
> > -		goto err_alloc;
> > +		goto err_free_data;
> >  	}
> >
> >  	switch (interrupt_data->type) {
> > @@ -397,10 +397,12 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
> >
> >  	return 0;
> >
> > -err_alloc:
> > -	kfree(interrupt_data);
> > -	kfree(interrupt_data->msix_entries);
> > +err_free_msix:
> >  	kfree(interrupt_data->eventfd_ctxs);
> > +err_free_eventfd:
> > +	kfree(interrupt_data->msix_entries);
> > +err_free_data:
> > +	kfree(interrupt_data);
> >  	return -ENOMEM;
> >  }
> >
> > --
> > 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/20181031164753.GA14140%40kvasir.local.
> > For more options, visit https://groups.google.com/d/optout.
> >

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

[Ioannis Valasakis]

On Wed, Oct 31, 2018 at 09:57:58PM +0100, Julia Lawall wrote:
> 
> 
> On Wed, 31 Oct 2018, Ioannis Valasakis wrote:
> 
> > On Wed, Oct 31, 2018 at 12:54:39PM -0400, Sasha Levin wrote:
> > > On Wed, Oct 31, 2018 at 04:47:54PM +0000, Ioannis Valasakis wrote:
> > > > Fix multiple error allocation path by using re-direction.
> > > >
> > > > Signed-off-by: Ioannis Valasakis <code@wizofe.uk>
> > > > ---
> > > > Changes in v2:
> > > > 	- Specific tags for memory allocation
> > > > 	- Free the memory in the right way as for kfre
> > > >
> > > > drivers/staging/gasket/gasket_interrupt.c | 14 ++++++++------
> > > > 1 file changed, 8 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/gasket/gasket_interrupt.c b/drivers/staging/gasket/gasket_interrupt.c
> > > > index cdfb72af1c52..23232ea8f4d9 100644
> > > > --- a/drivers/staging/gasket/gasket_interrupt.c
> > > > +++ b/drivers/staging/gasket/gasket_interrupt.c
> > > > @@ -348,21 +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) {
> > > > -		goto err_alloc;
> > > > +		goto err_free_msix;
> > > > 	}
> > > >
> > > > 	interrupt_data->eventfd_ctxs = kcalloc(num_interrupts,
> > > > 					       sizeof(struct eventfd_ctx *),
> > > > 					       GFP_KERNEL);
> > > > 	if (!interrupt_data->eventfd_ctxs) {
> > > > -		goto err_alloc;
> > > > +		goto err_free_eventfd;
> > > > 	}
> > > >
> > > > 	interrupt_data->interrupt_counts = kcalloc(num_interrupts,
> > > > 						   sizeof(ulong),
> > > > 						   GFP_KERNEL);
> > > > 	if (!interrupt_data->interrupt_counts) {
> > > > -		goto err_alloc;
> > > > +		goto err_free_data;
> > > > 	}
> > > >
> > > > 	switch (interrupt_data->type) {
> > > > @@ -397,10 +397,12 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
> > > >
> > > > 	return 0;
> > > >
> > > > -err_alloc:
> > > > -	kfree(interrupt_data);
> > > > -	kfree(interrupt_data->msix_entries);
> > > > +err_free_msix:
> > > > 	kfree(interrupt_data->eventfd_ctxs);
> > > > +err_free_eventfd:
> > > > +	kfree(interrupt_data->msix_entries);
> > > > +err_free_data:
> > > > +	kfree(interrupt_data);
> > > > 	return -ENOMEM;
> > > > }
> > >
> > > With this code, what will happen if, for example, the last
> > > allocation (interrupt_data->interrupt_counts) fails?
> > It took my some time but I think I saw it; the mistake is that
> > if interrupt_data->interrupt_counts fails it should 'goto
> > err_free_msix'.
> >
> > Same if interrupt_data->msix_entries fails it should 'goto
> > err_free_data'.
> >
> > Is that right?
> >
> > (I got very ambitious with the labels and I mixed it up ;) Any way to
> > make the goto labels make sense?)
> 
> I don't think that the choice of labels is a problem.  Basically, you
> should normally release things in the opposite order of the one in which
> they were allocated.  At the beginning of your function, you can just do a
> return, because nothing has been allocated yet.  Then when you have
> successfully allocated something, if you fail afterwards you should go to
> the very end of the function, because you have only one thing to free.
> When you have successfully allocated something else, if you fail
> afterwards, you should jump not quite to the end, so that you free both
> things that have been allocated, etc.
Indeed that is what I meant. Badly phrased but I released things the
wrong way around. I send a v3 of the patch (although as I just noticed
your latest comment, if that makes sense I am going to rebase against
greg's staging and resend it).

Thank you 

Ioannis

> 
> julia

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

[Julia Lawall]

On Wed, 31 Oct 2018, Ioannis Valasakis wrote:

> On Wed, Oct 31, 2018 at 09:57:58PM +0100, Julia Lawall wrote:
> >
> >
> > On Wed, 31 Oct 2018, Ioannis Valasakis wrote:
> >
> > > On Wed, Oct 31, 2018 at 12:54:39PM -0400, Sasha Levin wrote:
> > > > On Wed, Oct 31, 2018 at 04:47:54PM +0000, Ioannis Valasakis wrote:
> > > > > Fix multiple error allocation path by using re-direction.
> > > > >
> > > > > Signed-off-by: Ioannis Valasakis <code@wizofe.uk>
> > > > > ---
> > > > > Changes in v2:
> > > > > 	- Specific tags for memory allocation
> > > > > 	- Free the memory in the right way as for kfre
> > > > >
> > > > > drivers/staging/gasket/gasket_interrupt.c | 14 ++++++++------
> > > > > 1 file changed, 8 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/drivers/staging/gasket/gasket_interrupt.c b/drivers/staging/gasket/gasket_interrupt.c
> > > > > index cdfb72af1c52..23232ea8f4d9 100644
> > > > > --- a/drivers/staging/gasket/gasket_interrupt.c
> > > > > +++ b/drivers/staging/gasket/gasket_interrupt.c
> > > > > @@ -348,21 +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) {
> > > > > -		goto err_alloc;
> > > > > +		goto err_free_msix;
> > > > > 	}
> > > > >
> > > > > 	interrupt_data->eventfd_ctxs = kcalloc(num_interrupts,
> > > > > 					       sizeof(struct eventfd_ctx *),
> > > > > 					       GFP_KERNEL);
> > > > > 	if (!interrupt_data->eventfd_ctxs) {
> > > > > -		goto err_alloc;
> > > > > +		goto err_free_eventfd;
> > > > > 	}
> > > > >
> > > > > 	interrupt_data->interrupt_counts = kcalloc(num_interrupts,
> > > > > 						   sizeof(ulong),
> > > > > 						   GFP_KERNEL);
> > > > > 	if (!interrupt_data->interrupt_counts) {
> > > > > -		goto err_alloc;
> > > > > +		goto err_free_data;
> > > > > 	}
> > > > >
> > > > > 	switch (interrupt_data->type) {
> > > > > @@ -397,10 +397,12 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
> > > > >
> > > > > 	return 0;
> > > > >
> > > > > -err_alloc:
> > > > > -	kfree(interrupt_data);
> > > > > -	kfree(interrupt_data->msix_entries);
> > > > > +err_free_msix:
> > > > > 	kfree(interrupt_data->eventfd_ctxs);
> > > > > +err_free_eventfd:
> > > > > +	kfree(interrupt_data->msix_entries);
> > > > > +err_free_data:
> > > > > +	kfree(interrupt_data);
> > > > > 	return -ENOMEM;
> > > > > }
> > > >
> > > > With this code, what will happen if, for example, the last
> > > > allocation (interrupt_data->interrupt_counts) fails?
> > > It took my some time but I think I saw it; the mistake is that
> > > if interrupt_data->interrupt_counts fails it should 'goto
> > > err_free_msix'.
> > >
> > > Same if interrupt_data->msix_entries fails it should 'goto
> > > err_free_data'.
> > >
> > > Is that right?
> > >
> > > (I got very ambitious with the labels and I mixed it up ;) Any way to
> > > make the goto labels make sense?)
> >
> > I don't think that the choice of labels is a problem.  Basically, you
> > should normally release things in the opposite order of the one in which
> > they were allocated.  At the beginning of your function, you can just do a
> > return, because nothing has been allocated yet.  Then when you have
> > successfully allocated something, if you fail afterwards you should go to
> > the very end of the function, because you have only one thing to free.
> > When you have successfully allocated something else, if you fail
> > afterwards, you should jump not quite to the end, so that you free both
> > things that have been allocated, etc.
> Indeed that is what I meant. Badly phrased but I released things the
> wrong way around. I send a v3 of the patch (although as I just noticed
> your latest comment, if that makes sense I am going to rebase against
> greg's staging and resend it).

Please send the version against Greg's tree.  It would be easiest to see
if the patch is correct if one can see the actual starting point.

julia