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

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

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