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

[Christophe JAILLET]

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@xxxxxxxxxx>
> > ---
> > 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])
   - hope that tying 128 allocations is enough and that no "unusable 
allocation" will be done at run-time.

In other words, I tried to convinced myself that there was a real logic, 
even if unperfect.


Even if the above description is correct and if it works as expected in 
RL, it real looks like an overkill!


Now that I reread code around 'dif_local_dma_alloc' usage, I'm tempt to 
agree with your feeling about debug code.

CJ

[1]: 
https://elixir.bootlin.com/linux/v5.15.1/source/drivers/scsi/qla2xxx/qla_iocb.c#L1138

> regards,
> dan carpenter