mpt3sas: move dereference under check

PATCH In review Dan Carpenter 1 participant 2 messages Mar 11, 2013 – Mar 14, 2013 1 message not in the archive

Subsystems (3)

SCSI Kernel janitors LSI Logic MPT fusion drivers (FC/SAS/SPI)

lore

Share & tools

lore

Dan Carpenter

author

2013-03-11 11:40 UTC #0 lore raw

PATCH 1/1 mpt3sas: move dereference under check

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

Signed-off-by Dan Carpenter <[email protected]>

drivers/scsi/mpt3sas/mpt3sas_base.c +4 -2

index 1836003..06a84ef 100644
@@ -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));

A message that is not in the archive (sent off-list or not yet received): [email protected]

Dan Carpenter

author

2013-03-14 14:34 UTC #2 lore raw

Quoted 10 lines · Sreekanth

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.

Quoted 3 lines

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