On Sat, 2014-03-08 at 00:21 +0100, Maurizio Lombardi wrote:
> Hi Eddie,
>
> On Fri, Mar 07, 2014 at 11:21:45AM -0800, Eddie Wai wrote:
> > Hello Maurizio,
> >
> > Thanks for looking into this. The patch looks good, but I would like to
> > extend it with the following allocation failure portion to completely
> > remove the memory leak. Please incorporate this into your next
> > submission.
>
> Thanks for the patch you sent,
>
> maybe it is even better if I modify bnx2fc_allocate_hash_table() in such a way
> that, in case of error, it frees all the memory it has allocated.
>
> Note that your patch does not free the table if
> "hba->hash_tbl_pbl = dma_alloc_coherent(...)" fails (line 2058).
>
> It just requires a very little change to your patch, I'll send it tomorrow.
You're right, the hash_tbl_segments[i] entries will also need to be
freed upon the hash_tbl_pbl allocation failure. Thanks again!
>
> Regards,
> Maurizio Lombardi
>
> >
> > Thanks,
> > Eddie
> >
> > @@ -2020,7 +2026,7 @@ static int bnx2fc_allocate_hash_table(struct
> > bnx2fc_hba *hba)
> > dma_segment_array = kzalloc(dma_segment_array_size, GFP_KERNEL);
> > if (!dma_segment_array) {
> > printk(KERN_ERR PFX "hash table pointers (dma) alloc
> > failed\n");
> > - return -ENOMEM;
> > + goto free_hash_tbl_seg;
> > }
> >
> > for (i = 0; i < segment_count; ++i) {
> > @@ -2039,7 +2045,7 @@ static int bnx2fc_allocate_hash_table(struct
> > bnx2fc_hba *hba)
> > hba->hash_tbl_segments[i] = NULL;
> > }
> > kfree(dma_segment_array);
> > - return -ENOMEM;
> > + goto free_hash_tbl_seg;
> > }
> > memset(hba->hash_tbl_segments[i], 0,
> > BNX2FC_HASH_TBL_CHUNK_SIZE);
> > @@ -2077,6 +2083,11 @@ static int bnx2fc_allocate_hash_table(struct
> > bnx2fc_hba *hba)
> > }
> > kfree(dma_segment_array);
> > return 0;
> > +
> > +free_hash_tbl_seg:
> > + kfree(hba->hash_tbl_segments);
> > + hba->hash_tbl_segments = NULL;
> > + return -ENOMEM;
> > }
> >
> >
> > On Fri, 2014-03-07 at 15:37 +0100, Maurizio Lombardi wrote:
> > > If bnx2fc_allocate_hash_table() for some reasons fails, it is possible that the
> > > hash_tbl_segments or the hash_tbl_pbl pointers are NULL.
> > > In this case bnx2fc_free_hash_table() will panic the system.
> > >
> > > this patch also fixes a memory leak, the hash_tbl_segments pointer was never
> > > freed.
> > >
> > > Signed-off-by: Maurizio Lombardi <mlombard@xxxxxxxxxx>
> > > ---
> > > drivers/scsi/bnx2fc/bnx2fc_hwi.c | 34 ++++++++++++++++++++--------------
> > > 1 file changed, 20 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/scsi/bnx2fc/bnx2fc_hwi.c b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
> > > index 261af2a..f83bae4 100644
> > > --- a/drivers/scsi/bnx2fc/bnx2fc_hwi.c
> > > +++ b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
> > > @@ -1968,21 +1968,27 @@ static void bnx2fc_free_hash_table(struct bnx2fc_hba *hba)
> > > int segment_count;
> > > u32 *pbl;
> > >
> > > - segment_count = hba->hash_tbl_segment_count;
> > > -
> > > - pbl = hba->hash_tbl_pbl;
> > > - for (i = 0; i < segment_count; ++i) {
> > > - dma_addr_t dma_address;
> > > -
> > > - dma_address = le32_to_cpu(*pbl);
> > > - ++pbl;
> > > - dma_address += ((u64)le32_to_cpu(*pbl)) << 32;
> > > - ++pbl;
> > > - dma_free_coherent(&hba->pcidev->dev,
> > > - BNX2FC_HASH_TBL_CHUNK_SIZE,
> > > - hba->hash_tbl_segments[i],
> > > - dma_address);
> > > + if (hba->hash_tbl_segments) {
> > > +
> > > + pbl = hba->hash_tbl_pbl;
> > > + if (pbl) {
> > > + segment_count = hba->hash_tbl_segment_count;
> > > + for (i = 0; i < segment_count; ++i) {
> > > + dma_addr_t dma_address;
> > > +
> > > + dma_address = le32_to_cpu(*pbl);
> > > + ++pbl;
> > > + dma_address += ((u64)le32_to_cpu(*pbl)) << 32;
> > > + ++pbl;
> > > + dma_free_coherent(&hba->pcidev->dev,
> > > + BNX2FC_HASH_TBL_CHUNK_SIZE,
> > > + hba->hash_tbl_segments[i],
> > > + dma_address);
> > > + }
> > > + }
> > >
> > > + kfree(hba->hash_tbl_segments);
> > > + hba->hash_tbl_segments = NULL;
> > > }
> > >
> > > if (hba->hash_tbl_pbl) {
> >
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
