[bug report] scsi: mpt3sas: Handle RDPQ DMA allocation in same 4G region

[bug report] scsi: mpt3sas: Handle RDPQ DMA allocation in same 4G region

[Dan Carpenter]

Hello Suganath Prabu,

The patch 8012209eb26b: "scsi: mpt3sas: Handle RDPQ DMA allocation in
same 4G region" from Apr 23, 2020, leads to the following static
checker warning:

	drivers/scsi/mpt3sas/mpt3sas_base.c:5256 _base_allocate_memory_pools()
	warn: 'ioc->hpr_lookup' double freed

	drivers/scsi/mpt3sas/mpt3sas_base.c:5256 _base_allocate_memory_pools()
	warn: 'ioc->internal_lookup' double freed

drivers/scsi/mpt3sas/mpt3sas_base.c
  5202                  rdpq_sz = reply_post_free_sz * ioc->reply_queue_count;
  5203          ret = base_alloc_rdpq_dma_pool(ioc, rdpq_sz);
  5204          if (ret == -EAGAIN) {
  5205                  /*
  5206                   * Free allocated bad RDPQ memory pools.
  5207                   * Change dma coherent mask to 32 bit and reallocate RDPQ
  5208                   */
  5209                  _base_release_memory_pools(ioc);
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
We added this free here which frees a whole ton of different stuff.

  5210                  ioc->use_32bit_dma = true;
  5211                  if (_base_config_dma_addressing(ioc, ioc->pdev) != 0) {
  5212                          ioc_err(ioc,
  5213                              "32 DMA mask failed %s\n", pci_name(ioc->pdev));
  5214                          return -ENODEV;
  5215                  }
  5216                  if (base_alloc_rdpq_dma_pool(ioc, rdpq_sz))
  5217                          return -ENOMEM;
  5218          } else if (ret == -ENOMEM)
  5219                  return -ENOMEM;
  5220          total_sz = rdpq_sz * (!ioc->rdpq_array_enable ? 1 :
  5221              DIV_ROUND_UP(ioc->reply_queue_count, RDPQ_MAX_INDEX_IN_ONE_CHUNK));
  5222          ioc->scsiio_depth = ioc->hba_queue_depth -
  5223              ioc->hi_priority_depth - ioc->internal_depth;
  5224  
  5225          /* set the scsi host can_queue depth
  5226           * with some internal commands that could be outstanding
  5227           */
  5228          ioc->shost->can_queue = ioc->scsiio_depth - INTERNAL_SCSIIO_CMDS_COUNT;
  5229          dinitprintk(ioc,
  5230                      ioc_info(ioc, "scsi host: can_queue depth (%d)\n",
  5231                               ioc->shost->can_queue));
  5232  
  5233          /* contiguous pool for request and chains, 16 byte align, one extra "
  5234           * "frame for smid=0
  5235           */
  5236          ioc->chain_depth = ioc->chains_needed_per_io * ioc->scsiio_depth;
  5237          sz = ((ioc->scsiio_depth + 1) * ioc->request_sz);
  5238  
  5239          /* hi-priority queue */
  5240          sz += (ioc->hi_priority_depth * ioc->request_sz);
  5241  
  5242          /* internal queue */
  5243          sz += (ioc->internal_depth * ioc->request_sz);
  5244  
  5245          ioc->request_dma_sz = sz;
  5246          ioc->request = dma_alloc_coherent(&ioc->pdev->dev, sz,
  5247                          &ioc->request_dma, GFP_KERNEL);
  5248          if (!ioc->request) {
  5249                  ioc_err(ioc, "request pool: dma_alloc_coherent failed: hba_depth(%d), chains_per_io(%d), frame_sz(%d), total(%d kB)\n",
  5250                          ioc->hba_queue_depth, ioc->chains_needed_per_io,
  5251                          ioc->request_sz, sz / 1024);
  5252                  if (ioc->scsiio_depth < MPT3SAS_SAS_QUEUE_DEPTH)
  5253                          goto out;
  5254                  retry_sz = 64;
  5255                  ioc->hba_queue_depth -= retry_sz;
  5256                  _base_release_memory_pools(ioc);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
And leads to some double frees here.

  5257                  goto retry_allocation;
  5258          }
  5259          memset(ioc->request, 0, sz);
  5260  
  5261          if (retry_sz)
  5262                  ioc_err(ioc, "request pool: dma_alloc_coherent succeed: hba_depth(%d), chains_per_io(%d), frame_sz(%d), total(%d kb)\n",
  5263                          ioc->hba_queue_depth, ioc->chains_needed_per_io,
  5264                          ioc->request_sz, sz / 1024);
  5265  

regards,
dan carpenter

Re: [bug report] scsi: mpt3sas: Handle RDPQ DMA allocation in same 4G region

[Dan Carpenter]

This also causes a bug in the callers.

    drivers/scsi/mpt3sas/mpt3sas_base.c:7428 mpt3sas_base_attach()
    warn: 'ioc->hpr_lookup' double freed

We free the pointers, then return an error and the caller frees the
pointers as well.

Smatch is very limited in the types of double frees it looks for.
It really requires someone to manually go through the free paths and
check it by hand because I'm sure there are other bugs there as well.

Please CC me on the Fix because I'm not subscribed to the linux-scsi
mailing list.

regards,
dan carpenter

Re: [bug report] scsi: mpt3sas: Handle RDPQ DMA allocation in same 4G region

[Suganath Prabu Subramani]

Hi Dan,

We will post the patch with fix as soon as possible.

Thanks,
Suganath


On Fri, May 1, 2020 at 3:25 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> This also causes a bug in the callers.
>
>     drivers/scsi/mpt3sas/mpt3sas_base.c:7428 mpt3sas_base_attach()
>     warn: 'ioc->hpr_lookup' double freed
>
> We free the pointers, then return an error and the caller frees the
> pointers as well.
>
> Smatch is very limited in the types of double frees it looks for.
> It really requires someone to manually go through the free paths and
> check it by hand because I'm sure there are other bugs there as well.
>
> Please CC me on the Fix because I'm not subscribed to the linux-scsi
> mailing list.
>
> regards,
> dan carpenter
>