[bug report] [SCSI] lpfc 8.3.24: Extend BSG infrastructure and add link diagnostics

[bug report] [SCSI] lpfc 8.3.24: Extend BSG infrastructure and add link diagnostics

[Dan Carpenter]

Hello James Smart,

The patch 7ad20aa9d39a: "[SCSI] lpfc 8.3.24: Extend BSG
infrastructure and add link diagnostics" from May 24, 2011, leads to
the following static checker warning:

	drivers/scsi/lpfc/lpfc_bsg.c:4989 lpfc_bsg_issue_mbox()
	warn: 'dmabuf' was already freed.

The problem is that lpfc_bsg_issue_mbox() call lpfc_bsg_handle_sli_cfg_ext()
which calls lpfc_bsg_handle_sli_cfg_ebuf() which is where the bug really
is, I think.

drivers/scsi/lpfc/lpfc_bsg.c
  4584  static int
  4585  lpfc_bsg_handle_sli_cfg_ebuf(struct lpfc_hba *phba, struct bsg_job *job,
  4586                               struct lpfc_dmabuf *dmabuf)
  4587  {
  4588          int rc;
  4589  
  4590          lpfc_printf_log(phba, KERN_INFO, LOG_LIBDFC,
  4591                          "2971 SLI_CONFIG buffer (type:x%x)\n",
  4592                          phba->mbox_ext_buf_ctx.mboxType);
  4593  
  4594          if (phba->mbox_ext_buf_ctx.mboxType == mbox_rd) {
  4595                  if (phba->mbox_ext_buf_ctx.state != LPFC_BSG_MBOX_DONE) {
  4596                          lpfc_printf_log(phba, KERN_ERR, LOG_LIBDFC,
  4597                                          "2972 SLI_CONFIG rd buffer state "
  4598                                          "mismatch:x%x\n",
  4599                                          phba->mbox_ext_buf_ctx.state);
  4600                          lpfc_bsg_mbox_ext_abort(phba);
  4601                          return -EPIPE;
  4602                  }
  4603                  rc = lpfc_bsg_read_ebuf_get(phba, job);
  4604                  if (rc == SLI_CONFIG_HANDLED)
  4605                          lpfc_bsg_dma_page_free(phba, dmabuf);

I think this path is correct.

  4606          } else { /* phba->mbox_ext_buf_ctx.mboxType == mbox_wr */
  4607                  if (phba->mbox_ext_buf_ctx.state != LPFC_BSG_MBOX_HOST) {
  4608                          lpfc_printf_log(phba, KERN_ERR, LOG_LIBDFC,
  4609                                          "2973 SLI_CONFIG wr buffer state "
  4610                                          "mismatch:x%x\n",
  4611                                          phba->mbox_ext_buf_ctx.state);
  4612                          lpfc_bsg_mbox_ext_abort(phba);
  4613                          return -EPIPE;
  4614                  }
  4615                  rc = lpfc_bsg_write_ebuf_set(phba, job, dmabuf);

But this path has two bugs.  If lpfc_bsg_write_ebuf_set() then it frees
three things but it should not free anything.  This leads to the double
free bug which Smatch is complaining about.  (Smatch only catches one of
the problems).  The second bug is that if lpfc_bsg_write_ebuf_set()
return SLI_CONFIG_HANDLED then this patch should call
lpfc_bsg_dma_page_free(phba, dmabuf);

  4616          }
  4617          return rc;
  4618  }

regards,
dan carpenter

Re: [bug report] [SCSI] lpfc 8.3.24: Extend BSG infrastructure and add link diagnostics

[Dan Carpenter]

On Wed, Jan 13, 2021 at 11:56:06AM +0300, Dan Carpenter wrote:
> Hello James Smart,
> 
> The patch 7ad20aa9d39a: "[SCSI] lpfc 8.3.24: Extend BSG
> infrastructure and add link diagnostics" from May 24, 2011, leads to
> the following static checker warning:
> 
> 	drivers/scsi/lpfc/lpfc_bsg.c:4989 lpfc_bsg_issue_mbox()
> 	warn: 'dmabuf' was already freed.
> 
> The problem is that lpfc_bsg_issue_mbox() call lpfc_bsg_handle_sli_cfg_ext()
> which calls lpfc_bsg_handle_sli_cfg_ebuf() which is where the bug really
> is, I think.
> 
> drivers/scsi/lpfc/lpfc_bsg.c
>   4584  static int
>   4585  lpfc_bsg_handle_sli_cfg_ebuf(struct lpfc_hba *phba, struct bsg_job *job,
>   4586                               struct lpfc_dmabuf *dmabuf)
>   4587  {
>   4588          int rc;
>   4589  
>   4590          lpfc_printf_log(phba, KERN_INFO, LOG_LIBDFC,
>   4591                          "2971 SLI_CONFIG buffer (type:x%x)\n",
>   4592                          phba->mbox_ext_buf_ctx.mboxType);
>   4593  
>   4594          if (phba->mbox_ext_buf_ctx.mboxType == mbox_rd) {
>   4595                  if (phba->mbox_ext_buf_ctx.state != LPFC_BSG_MBOX_DONE) {
>   4596                          lpfc_printf_log(phba, KERN_ERR, LOG_LIBDFC,
>   4597                                          "2972 SLI_CONFIG rd buffer state "
>   4598                                          "mismatch:x%x\n",
>   4599                                          phba->mbox_ext_buf_ctx.state);
>   4600                          lpfc_bsg_mbox_ext_abort(phba);
>   4601                          return -EPIPE;
>   4602                  }
>   4603                  rc = lpfc_bsg_read_ebuf_get(phba, job);
>   4604                  if (rc == SLI_CONFIG_HANDLED)
>   4605                          lpfc_bsg_dma_page_free(phba, dmabuf);
> 
> I think this path is correct.
> 
>   4606          } else { /* phba->mbox_ext_buf_ctx.mboxType == mbox_wr */
>   4607                  if (phba->mbox_ext_buf_ctx.state != LPFC_BSG_MBOX_HOST) {
>   4608                          lpfc_printf_log(phba, KERN_ERR, LOG_LIBDFC,
>   4609                                          "2973 SLI_CONFIG wr buffer state "
>   4610                                          "mismatch:x%x\n",
>   4611                                          phba->mbox_ext_buf_ctx.state);
>   4612                          lpfc_bsg_mbox_ext_abort(phba);
>   4613                          return -EPIPE;
>   4614                  }
>   4615                  rc = lpfc_bsg_write_ebuf_set(phba, job, dmabuf);
> 
> But this path has two bugs.  If lpfc_bsg_write_ebuf_set() then it frees
> three things but it should not free anything.  This leads to the double
> free bug which Smatch is complaining about.  (Smatch only catches one of
> the problems).

Actually the other two are local variables so freeing them was correct.
But I've added the mempool_free() to Smatch as a free function since it
wasn't tracking that before.

regards,
dan carpenter