scsi: be2iscsi: Fix a theoretical leak in beiscsi_create_eqs()

The be_fill_queue() function can only fail when "eq_vaddress" is NULL and since it's non-NULL here that means the function call can't fail. But imagine if it could, then in that situation we would want to store the "paddr" so that dma memory can be released.

---
 drivers/scsi/be2iscsi/be_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 5c3513a4b450..202ba925c494 100644
@@ -3020,6 +3020,7 @@ static int beiscsi_create_eqs(struct beiscsi_hba *phba,
 			goto create_eq_error;
 		}
 
+		mem->dma = paddr;
 		mem->va = eq_vaddress;
 		ret = be_fill_queue(eq, phba->params.num_eq_entries,
 				    sizeof(struct be_eq_entry), eq_vaddress);

@@ -3029,7 +3030,6 @@ static int beiscsi_create_eqs(struct beiscsi_hba *phba,
 			goto create_eq_error;
 		}
 
-		mem->dma = paddr;
 		ret = beiscsi_cmd_eq_create(&phba->ctrl, eq,
 					    BEISCSI_EQ_DELAY_DEF);
 		if (ret) {

@@ -3086,6 +3086,7 @@ static int beiscsi_create_cqs(struct beiscsi_hba *phba,
 			goto create_cq_error;
 		}
 
+		mem->dma = paddr;
 		ret = be_fill_queue(cq, phba->params.num_cq_entries,
 				    sizeof(struct sol_cqe), cq_vaddress);
 		if (ret) {

@@ -3095,7 +3096,6 @@ static int beiscsi_create_cqs(struct beiscsi_hba *phba,
 			goto create_cq_error;
 		}
 
-		mem->dma = paddr;
 		ret = beiscsi_cmd_cq_create(&phba->ctrl, cq, eq, false,
 					    false, 0);
 		if (ret) {

2.28.0

Applied to 5.10/scsi-queue, thanks!

[1/1] scsi: be2iscsi: Fix a theoretical leak in beiscsi_create_eqs()

      https://git.kernel.org/mkp/scsi/c/38b2db564d9a

Martin K. Petersen	Oracle Linux Engineering

This came in here through the stable 5.4 tree with v5.4.74, and we have some users of ours report that it results in kernel oopses and delayed boot on their HP DL 380 Gen 9 (and other Gen 9, FWICT) servers:

Blacklisting the be2iscsi module or reverting this commit helps, I did not get around to look further into the mechanics at play and figured you would be faster at that, or that this info at least helps someone else when searching for the same symptoms.

cheers,
Thomas

Thanks for the report Thomas. I see the bug in my patch:

drivers/scsi/be2iscsi/be_main.c
3008 eq_for_mcc = 1;
3009 else
3010 eq_for_mcc = 0;
3011 for (i = 0; i < (phba->num_cpus + eq_for_mcc); i++) {

  3012                  eq = &phwi_context->be_eq[i].q;
  3013                  mem = &eq->dma_mem;
  3014                  phwi_context->be_eq[i].phba = phba;
  3015                  eq_vaddress = dma_alloc_coherent(&phba->pcidev->dev,
  3016                                                     num_eq_pages * PAGE_SIZE,
  3017                                                     &paddr, GFP_KERNEL);
  3018                  if (!eq_vaddress) {
  3019                          ret = -ENOMEM;
  3020                          goto create_eq_error;
  3021                  }
  3022  
  3023                  mem->dma = paddr;
                        ^^^^^^^^^^^^^^^^

I moved this assignment ahead of the call to be_fill_queue().

  3024                  mem->va = eq_vaddress;
  3025                  ret = be_fill_queue(eq, phba->params.num_eq_entries,
  3026                                      sizeof(struct be_eq_entry), eq_vaddress);
  3027                  if (ret) {
  3028                          beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT,
  3029                                      "BM_%d : be_fill_queue Failed for EQ\n");
  3030                          goto create_eq_error;
  3031                  }

drivers/scsi/be2iscsi/be_main.c
2978 static int be_fill_queue(struct be_queue_info *q,
2979 u16 len, u16 entry_size, void *vaddress)
2980 {
2981 struct be_dma_mem *mem = &q->dma_mem;
2982

  2983          memset(q, 0, sizeof(*q));
                ^^^^^^^^^^^^^^^^^^^^^^^

But the first thing that it does is it overwrites it with zeros.

  2984          q->len = len;
  2985          q->entry_size = entry_size;
  2986          mem->size = len * entry_size;
  2987          mem->va = vaddress;

It also overwrites the "mem->va = eq_vaddress;" assignment as well, but but it sets that back again here...

  2988          if (!mem->va)
  2989                  return -ENOMEM;
  2990          memset(mem->va, 0, mem->size);
  2991          return 0;
  2992  }

I will just revert my patch. This code is messy but it works so far as I can see.

regards,
dan carpenter

My patch caused kernel Oopses and delays in boot. Revert it.

The problem was that I moved the "mem->dma = paddr;" before the call to be_fill_queue(). But the first thing that the be_fill_queue() function does is memset the whole struct to zero which overwrites the assignment.

---
My original patch was basically a clean up patch and to try silence a static checker warning. I've already updated the static checker to not warn about impossible leaks and in this case we know that be_fill_queue() cannot fail.

I was tempted to delete the "mem->va = eq_vaddress;" assignment as a clean up but I didn't. :P

 drivers/scsi/be2iscsi/be_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 50e464224d47..90fcddb76f46 100644
@@ -3020,7 +3020,6 @@ static int beiscsi_create_eqs(struct beiscsi_hba *phba,
 			goto create_eq_error;
 		}
 
-		mem->dma = paddr;
 		mem->va = eq_vaddress;
 		ret = be_fill_queue(eq, phba->params.num_eq_entries,
 				    sizeof(struct be_eq_entry), eq_vaddress);

@@ -3030,6 +3029,7 @@ static int beiscsi_create_eqs(struct beiscsi_hba *phba,
 			goto create_eq_error;
 		}
 
+		mem->dma = paddr;
 		ret = beiscsi_cmd_eq_create(&phba->ctrl, eq,
 					    BEISCSI_EQ_DELAY_DEF);
 		if (ret) {

@@ -3086,7 +3086,6 @@ static int beiscsi_create_cqs(struct beiscsi_hba *phba,
 			goto create_cq_error;
 		}
 
-		mem->dma = paddr;
 		ret = be_fill_queue(cq, phba->params.num_cq_entries,
 				    sizeof(struct sol_cqe), cq_vaddress);
 		if (ret) {

@@ -3096,6 +3095,7 @@ static int beiscsi_create_cqs(struct beiscsi_hba *phba,
 			goto create_cq_error;
 		}
 
+		mem->dma = paddr;
 		ret = beiscsi_cmd_cq_create(&phba->ctrl, cq, eq, false,
 					    false, 0);
 		if (ret) {

2.29.2

Can someone please add:
Cc: stable <[email protected]>
to this so we know to pick it up quickly there?

thanks,

greg k-h

Added Cc: stable and applied to 5.10/scsi-fixes, thanks!

[1/1] scsi: be2iscsi: revert "Fix a theoretical leak in beiscsi_create_eqs()"

      https://git.kernel.org/mkp/scsi/c/eeaf06af6f87

Martin K. Petersen	Oracle Linux Engineering