Received-SPF: pass (google.com: domain of code@wizofe.uk designates 185.20.209.243 as permitted sender) client-ip=185.20.209.243;
Date: Wed, 31 Oct 2018 21:00:35 +0000
From: Ioannis Valasakis <code@wizofe.uk>
To: Julia Lawall <julia.lawall@lip6.fr>
Cc: Sasha Levin <sashal@kernel.org>, outreachy-kernel@googlegroups.com,
	gregkh@linuxfoundation.org, alexander.levin@microsoft.com
Subject: Re: [Outreachy kernel] [PATCH v2] staging: gasket Create a memory
 allocation path
Message-ID: <20181031210035.GA13539@kvasir.local>
References: <20181031164753.GA14140@kvasir.local>
 <20181031165439.GU194472@sasha-vm>
 <20181031205048.GA29504@kvasir.local>
 <alpine.DEB.2.21.1810312155300.2391@hadrien>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <alpine.DEB.2.21.1810312155300.2391@hadrien>
User-Agent: Mutt/1.10.1 (2018-07-13)

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