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
