[patch] [SCSI] mpt3sas: move dereference under check

[patch] [SCSI] mpt3sas: move dereference under check

[Dan Carpenter]

pci_pool_free() dereferences "ioc->sense_dma_pool" but we check it
for NULL on the following line.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 1836003..06a84ef 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -2479,9 +2479,11 @@ _base_release_memory_pools(struct MPT3SAS_ADAPTER *ioc)
 	}
 
 	if (ioc->sense) {
-		pci_pool_free(ioc->sense_dma_pool, ioc->sense, ioc->sense_dma);
-		if (ioc->sense_dma_pool)
+		if (ioc->sense_dma_pool) {
+			pci_pool_free(ioc->sense_dma_pool, ioc->sense,
+				      ioc->sense_dma);
 			pci_pool_destroy(ioc->sense_dma_pool);
+		}
 		dexitprintk(ioc, pr_info(MPT3SAS_FMT
 			"sense_pool(0x%p): free\n",
 			ioc->name, ioc->sense));

Re: [patch] [SCSI] mpt3sas: move dereference under check

[Dan Carpenter]

On Thu, Mar 14, 2013 at 06:52:35PM +0530, Reddy, Sreekanth wrote:
> Hi Dan Carpenter,
> 
> While analyzing this patch, I have added some debugging prints to
> print the address referenced by the IOC->sense_dma_pool before and
> after pci_pool_free() API and I have observed that the address
> referenced by this pointer is same before and after calling
> pci_pool_free() API.  Please let me know if you have any case
> where you have seen ioc->sense_dma_pool is dereferenced after
> calling pci_pool_free API.
> 
> And we are checking this ioc->sense_dma_pool pointer to NULL value
> for safe side even it is not assigned to NULL value anywhere in
> the driver code.

Thanks for looking at this.

I hope you don't mind, but I've added linux-scsi back to the CC.

This is a static checker which is complaining that the code is not
checking for NULL consistently.  If "ioc->sense_dma_pool" is NULL
then we will crash inside pci_pool_free().  The NULL dereference is
on the line where we do:

mm/dmapool.c
   391  void dma_pool_free(struct dma_pool *pool, void *vaddr, dma_addr_t dma)
   392  {
   393          struct dma_page *page;
   394          unsigned long flags;
   395          unsigned int offset;
   396  
   397          spin_lock_irqsave(&pool->lock, flags);
                                  ^^^^^^^^^^^
This dereference will cause a crash because we call pci_pool_free()
without testing for NULL.

   398          page = pool_find_page(pool, dma);

In other words:

drivers/scsi/mpt3sas/mpt3sas_base.c
  2481          if (ioc->sense) {
  2482                  pci_pool_free(ioc->sense_dma_pool, ioc->sense, ioc->sense_dma);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
We crash on this line before we reach the NULL test on the next
line.

  2483                  if (ioc->sense_dma_pool)
  2484                          pci_pool_destroy(ioc->sense_dma_pool);

I've looked at the code, and you're right that if (ioc->sense) {
is non-NULL that means ->sense_dma_pool is non-NULL.  ioc->sense is
allocated from the DMA pool.

I would like to just remove the test and silence the warning
message.

regards,
dan carpenter