Re: [PATCH] scsi: qla2xxx: Fix memory leaks in the error handling path of 'qla2x00_mem_alloc()'

[Dan Carpenter <dan.carpenter@oracle.com>]

On Thu, Nov 11, 2021 at 11:18:06AM +0100, Christophe JAILLET wrote:
> Le 11/11/2021 à 10:17, Dan Carpenter a écrit :
> > On Wed, Nov 10, 2021 at 10:11:34PM +0100, Christophe JAILLET wrote:
> > > In case of memory allocation failure, we should release many things and
> > > should not return directly.
> > > 
> > > The tricky part here, is that some (kzalloc + dma_pool_alloc) resources
> > > are allocated and stored in 'unusable' and a 'good' list.
> > > The 'good' list is then freed and only the 'unusable' list remains
> > > allocated.
> > > So, only this 'unusable' list is then freed in the error handling path of
> > > the function.
> > > 
> > > So, instead of adding even more code in this already huge function, just
> > > 'continue' (as already done if dma_pool_alloc() fails) instead of
> > > returning directly.
> > > 
> > > After the 'for' loop, we will then branch to the correct place of the
> > > error handling path when another memory allocation will (likely) fail
> > > afterward.
> > > 
> > > Fixes: 50b812755e97 ("scsi: qla2xxx: Fix DMA error when the DIF sg buffer crosses 4GB boundary")
> > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > ---
> > > Certainly not the best solution, but look 'safe' to me.
> > 
> > Your analysis seems correct, but this is deeply weird.
> I agree, deeply weird :)
> 
> >  It sort of looks
> > like this was debug code that was committed accidentally.  Neither
> > the "good" list nor the "unusable" are used except to print some debug
> > info:
> > 
> >                         ql_dbg_pci(ql_dbg_init, ha->pdev, 0x0024,
> >                             "%s: dif dma pool (good=%u unusable=%u)\n",
> >                             __func__, ha->pool.good.count,
> >                             ha->pool.unusable.count);
> > 
> > The good list is freed immediately, and then there is a no-op free in
> > qla2x00_mem_free().
> I agree.
> 
> > The unusable list is preserved until qla2x00_mem_free()
> > but not used anywhere.
> I agree.
> 
> The logic in commit '50b812755e97' puzzled me a lot.
> I wonder why the 128 magic number in the for loop.
> 
> My understanding is:
>    - try to allocate things at start-up
>    - check if this allocation crosses the 4G limit (see commit log)
>    - keep the "unusable" allocation allocated, so that this memory is
> reserved (i.e. wasted) and won't be allocated later (see usage of the
> dif_bundl_pool dma pool in [1])

Ah, I considered that but didn't follow through on the analysis all the
way.  Possible!

regards,
dan carpenter