[PATCH 1/2] qla2xxx: Fix a recently introduced memory leak

[PATCH 1/2] qla2xxx: Fix a recently introduced memory leak

[Bart Van Assche]

qla2x00_probe_one() allocates IRQs before it initializes rsp_q_map
so IRQs must be freed even if rsp_q_map allocation did not occur.
This was detected by kmemleak.

Fixes: 4fa183455988 ("scsi: qla2xxx: Utilize pci_alloc_irq_vectors/pci_free_irq_vectors calls")
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Michael Hernandez <michael.hernandez@cavium.com>
Cc: Himanshu Madhani <himanshu.madhani@cavium.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: <stable@vger.kernel.org>
---
 drivers/scsi/qla2xxx/qla_isr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
index dc88a09f9043..a94b0b6bd030 100644
--- a/drivers/scsi/qla2xxx/qla_isr.c
+++ b/drivers/scsi/qla2xxx/qla_isr.c
@@ -3242,7 +3242,7 @@ qla2x00_free_irqs(scsi_qla_host_t *vha)
 	 * from a probe failure context.
 	 */
 	if (!ha->rsp_q_map || !ha->rsp_q_map[0])
-		return;
+		goto free_irqs;
 	rsp = ha->rsp_q_map[0];
 
 	if (ha->flags.msix_enabled) {
@@ -3262,6 +3262,7 @@ qla2x00_free_irqs(scsi_qla_host_t *vha)
 		free_irq(pci_irq_vector(ha->pdev, 0), rsp);
 	}
 
+free_irqs:
 	pci_free_irq_vectors(ha->pdev);
 }
 
-- 
2.11.0

Re: [PATCH 1/2] qla2xxx: Fix a recently introduced memory leak

[Christoph Hellwig]

Thanks Bart,

this looks good to me.

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 1/2] qla2xxx: Fix a recently introduced memory leak

[Madhani, Himanshu]

On 1/23/17, 8:34 AM, "Bart Van Assche" <bart.vanassche@sandisk.com> wrote:

>qla2x00_probe_one() allocates IRQs before it initializes rsp_q_map
>so IRQs must be freed even if rsp_q_map allocation did not occur.
>This was detected by kmemleak.
>
>Fixes: 4fa183455988 ("scsi: qla2xxx: Utilize pci_alloc_irq_vectors/pci_free_irq_vectors calls")
>Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
>Cc: Michael Hernandez <michael.hernandez@cavium.com>
>Cc: Himanshu Madhani <himanshu.madhani@cavium.com>
>Cc: Christoph Hellwig <hch@lst.de>
>Cc: <stable@vger.kernel.org>
>---
> drivers/scsi/qla2xxx/qla_isr.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
>index dc88a09f9043..a94b0b6bd030 100644
>--- a/drivers/scsi/qla2xxx/qla_isr.c
>+++ b/drivers/scsi/qla2xxx/qla_isr.c
>@@ -3242,7 +3242,7 @@ qla2x00_free_irqs(scsi_qla_host_t *vha)
> 	 * from a probe failure context.
> 	 */
> 	if (!ha->rsp_q_map || !ha->rsp_q_map[0])
>-		return;
>+		goto free_irqs;
> 	rsp = ha->rsp_q_map[0];
> 
> 	if (ha->flags.msix_enabled) {
>@@ -3262,6 +3262,7 @@ qla2x00_free_irqs(scsi_qla_host_t *vha)
> 		free_irq(pci_irq_vector(ha->pdev, 0), rsp);
> 	}
> 
>+free_irqs:
> 	pci_free_irq_vectors(ha->pdev);
> }
> 
>-- 
>2.11.0

Thanks Bart. Looks good. 

Acked-By: Himanshu Madhani <himanshu.madhani@cavium.com>

Re: [PATCH 1/2] qla2xxx: Fix a recently introduced memory leak

[Johannes Thumshirn]

On Mon, Jan 23, 2017 at 08:34:45AM -0800, Bart Van Assche wrote:
> qla2x00_probe_one() allocates IRQs before it initializes rsp_q_map
> so IRQs must be freed even if rsp_q_map allocation did not occur.
> This was detected by kmemleak.
> 
> Fixes: 4fa183455988 ("scsi: qla2xxx: Utilize pci_alloc_irq_vectors/pci_free_irq_vectors calls")
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Michael Hernandez <michael.hernandez@cavium.com>
> Cc: Himanshu Madhani <himanshu.madhani@cavium.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: <stable@vger.kernel.org>
> ---

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: Felix Imend�rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N�rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 1/2] qla2xxx: Fix a recently introduced memory leak

[Martin K. Petersen]

>>>>> "Bart" == Bart Van Assche <bart.vanassche@sandisk.com> writes:

Bart> qla2x00_probe_one() allocates IRQs before it initializes rsp_q_map
Bart> so IRQs must be freed even if rsp_q_map allocation did not occur.
Bart> This was detected by kmemleak.

I queued this one yesterday but was waiting for a resolution on patch
2...

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH 2/2] qla2xxx: Avoid that issuing a LIP triggers a kernel crash

[Bart Van Assche]

Avoid that issuing a LIP as follows:

  find /sys -name 'issue_lip'|while read f; do echo 1 > $f; done

triggers the following:

BUG: unable to handle kernel NULL pointer dereference at (null)
Call Trace:
 qla2x00_abort_all_cmds+0xed/0x140 [qla2xxx]
 qla2x00_abort_isp_cleanup+0x1e3/0x280 [qla2xxx]
 qla2x00_abort_isp+0xef/0x690 [qla2xxx]
 qla2x00_do_dpc+0x36c/0x880 [qla2xxx]
 kthread+0x10c/0x140

Fixes: 1535aa75a3d8 ("qla2xxx: fix invalid DMA access after command aborts in PCI device remove")
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Naresh Bannoth <nbannoth@in.ibm.com>
Cc: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>
Cc: Himanshu Madhani <himanshu.madhani@cavium.com>
Cc: <stable@vger.kernel.org>
---
 drivers/scsi/qla2xxx/qla_os.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index 0a000ecf0881..ae9c5a7b239a 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -1600,6 +1600,7 @@ qla2x00_abort_all_cmds(scsi_qla_host_t *vha, int res)
 	srb_t *sp;
 	struct qla_hw_data *ha = vha->hw;
 	struct req_que *req;
+	struct scsi_cmnd *scmd;
 
 	qlt_host_reset_handler(ha);
 
@@ -1613,6 +1614,8 @@ qla2x00_abort_all_cmds(scsi_qla_host_t *vha, int res)
 		for (cnt = 1; cnt < req->num_outstanding_cmds; cnt++) {
 			sp = req->outstanding_cmds[cnt];
 			if (sp) {
+				scmd = GET_CMD_SP(sp);
+
 				/* Don't abort commands in adapter during EEH
 				 * recovery as it's not accessible/responding.
 				 */
@@ -1624,7 +1627,8 @@ qla2x00_abort_all_cmds(scsi_qla_host_t *vha, int res)
 					 */
 					sp_get(sp);
 					spin_unlock_irqrestore(&ha->hardware_lock, flags);
-					qla2xxx_eh_abort(GET_CMD_SP(sp));
+					if (scmd)
+						qla2xxx_eh_abort(scmd);
 					spin_lock_irqsave(&ha->hardware_lock, flags);
 				}
 				req->outstanding_cmds[cnt] = NULL;
-- 
2.11.0

Re: [PATCH 2/2] qla2xxx: Avoid that issuing a LIP triggers a kernel crash

[Madhani, Himanshu]

On 1/23/17, 8:34 AM, "Bart Van Assche" <bart.vanassche@sandisk.com> wrote:

>Avoid that issuing a LIP as follows:
>
>  find /sys -name 'issue_lip'|while read f; do echo 1 > $f; done
>
>triggers the following:
>
>BUG: unable to handle kernel NULL pointer dereference at (null)
>Call Trace:
> qla2x00_abort_all_cmds+0xed/0x140 [qla2xxx]
> qla2x00_abort_isp_cleanup+0x1e3/0x280 [qla2xxx]
> qla2x00_abort_isp+0xef/0x690 [qla2xxx]
> qla2x00_do_dpc+0x36c/0x880 [qla2xxx]
> kthread+0x10c/0x140
>
>Fixes: 1535aa75a3d8 ("qla2xxx: fix invalid DMA access after command aborts in PCI device remove")
>Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
>Cc: Naresh Bannoth <nbannoth@in.ibm.com>
>Cc: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>
>Cc: Himanshu Madhani <himanshu.madhani@cavium.com>
>Cc: <stable@vger.kernel.org>
>---
> drivers/scsi/qla2xxx/qla_os.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
>index 0a000ecf0881..ae9c5a7b239a 100644
>--- a/drivers/scsi/qla2xxx/qla_os.c
>+++ b/drivers/scsi/qla2xxx/qla_os.c
>@@ -1600,6 +1600,7 @@ qla2x00_abort_all_cmds(scsi_qla_host_t *vha, int res)
> 	srb_t *sp;
> 	struct qla_hw_data *ha = vha->hw;
> 	struct req_que *req;
>+	struct scsi_cmnd *scmd;
> 
> 	qlt_host_reset_handler(ha);
> 
>@@ -1613,6 +1614,8 @@ qla2x00_abort_all_cmds(scsi_qla_host_t *vha, int res)
> 		for (cnt = 1; cnt < req->num_outstanding_cmds; cnt++) {
> 			sp = req->outstanding_cmds[cnt];
> 			if (sp) {
>+				scmd = GET_CMD_SP(sp);
>+
> 				/* Don't abort commands in adapter during EEH
> 				 * recovery as it's not accessible/responding.
> 				 */
>@@ -1624,7 +1627,8 @@ qla2x00_abort_all_cmds(scsi_qla_host_t *vha, int res)
> 					 */
> 					sp_get(sp);
> 					spin_unlock_irqrestore(&ha->hardware_lock, flags);
>-					qla2xxx_eh_abort(GET_CMD_SP(sp));
>+					if (scmd)
>+						qla2xxx_eh_abort(scmd);
> 					spin_lock_irqsave(&ha->hardware_lock, flags);
> 				}
> 				req->outstanding_cmds[cnt] = NULL;
>-- 
>2.11.0
>

Looks Good. 

Acked-by: Himanshu Madhani <himanshu.madhani@cavium.com>

Re: [PATCH 2/2] qla2xxx: Avoid that issuing a LIP triggers a kernel crash

[Johannes Thumshirn]

On Mon, Jan 23, 2017 at 08:34:46AM -0800, Bart Van Assche wrote:
> Avoid that issuing a LIP as follows:
> 
>   find /sys -name 'issue_lip'|while read f; do echo 1 > $f; done
> 
> triggers the following:
> 
> BUG: unable to handle kernel NULL pointer dereference at (null)
> Call Trace:
>  qla2x00_abort_all_cmds+0xed/0x140 [qla2xxx]
>  qla2x00_abort_isp_cleanup+0x1e3/0x280 [qla2xxx]
>  qla2x00_abort_isp+0xef/0x690 [qla2xxx]
>  qla2x00_do_dpc+0x36c/0x880 [qla2xxx]
>  kthread+0x10c/0x140
> 
> Fixes: 1535aa75a3d8 ("qla2xxx: fix invalid DMA access after command aborts in PCI device remove")
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Naresh Bannoth <nbannoth@in.ibm.com>
> Cc: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>
> Cc: Himanshu Madhani <himanshu.madhani@cavium.com>
> Cc: <stable@vger.kernel.org>
> ---

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: Felix Imend�rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N�rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 2/2] qla2xxx: Avoid that issuing a LIP triggers a kernel crash

[Mauricio Faria de Oliveira]

Hi Bart,

First of all, sorry for the new bug; I didn't realize the pointer could
be NULL at this scenario.

On 01/23/2017 02:34 PM, Bart Van Assche wrote:
> @@ -1624,7 +1627,8 @@ qla2x00_abort_all_cmds(scsi_qla_host_t *vha, int res)
>  					 */
>  					sp_get(sp);
>  					spin_unlock_irqrestore(&ha->hardware_lock, flags);
> -					qla2xxx_eh_abort(GET_CMD_SP(sp));
> +					if (scmd)
> +						qla2xxx_eh_abort(scmd);
>  					spin_lock_irqsave(&ha->hardware_lock, flags);
>  				}

Now, this chunk has a problem with reference counting (and unnecessary
spin-locking), which we can avoid by simply moving up this NULL check.

The call to sp_get() increments the sp->ref_count, but if you skip the
call to qla2xxx_eh_abort() you don't get the decrement from the call to
sp->done() at abort handling from ISR, e.g., qla24xx_abort_iocb_entry().
[or if the command completed successfully between issue/complete abort,
at the completion from ISR, e.g., qla2x00_process_completed_request().]

The sp->done() call just below this chunk was supposed to drop the
initial reference [set at qla2xxx_queuecommand()] at a time we did
not call qla2xxx_eh_abort() yet... but now that we __may__ call it
(and get that sp->done() call from the ISR abort handling), we need
to only increment it if we're going to drop it.

That should be resolved with this slight change to your patch
(which also helps w/ the spin-locking).  What do you/others think?

diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index 0a000ecf0881..a17cb63b3fd5 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -1600,6 +1600,7 @@ uint32_t qla2x00_isp_reg_stat(struct qla_hw_data *ha)
         srb_t *sp;
         struct qla_hw_data *ha = vha->hw;
         struct req_que *req;
+       struct scsi_cmnd *scmd;

         qlt_host_reset_handler(ha);

@@ -1613,10 +1614,12 @@ uint32_t qla2x00_isp_reg_stat(struct qla_hw_data 
*ha)
                 for (cnt = 1; cnt < req->num_outstanding_cmds; cnt++) {
                         sp = req->outstanding_cmds[cnt];
                         if (sp) {
+                               scmd = GET_CMD_SP(sp);
+
                                 /* Don't abort commands in adapter 
during EEH
                                  * recovery as it's not 
accessible/responding.
                                  */
-                               if (!ha->flags.eeh_busy) {
+                               if (scmd && !ha->flags.eeh_busy) {
                                         /* Get a reference to the sp 
and drop the lock.
                                          * The reference ensures this 
sp->done() call
                                          * - and not the call in 
qla2xxx_eh_abort() -
@@ -1624,7 +1627,7 @@ uint32_t qla2x00_isp_reg_stat(struct qla_hw_data *ha)
                                          */
                                         sp_get(sp);
 
spin_unlock_irqrestore(&ha->hardware_lock, flags);
-                                       qla2xxx_eh_abort(GET_CMD_SP(sp));
+                                       qla2xxx_eh_abort(scmd);
 
spin_lock_irqsave(&ha->hardware_lock, flags);
                                 }
                                 req->outstanding_cmds[cnt] = NULL;


Signed-off-by: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>


-- 
Mauricio Faria de Oliveira
IBM Linux Technology Center

Re: [PATCH 2/2] qla2xxx: Avoid that issuing a LIP triggers a kernel crash

[Madhani, Himanshu]

On 1/24/17, 6:59 AM, "Mauricio Faria de Oliveira" <mauricfo@linux.vnet.ibm.com> wrote:

>Hi Bart,
>
>First of all, sorry for the new bug; I didn't realize the pointer could
>be NULL at this scenario.
>
>On 01/23/2017 02:34 PM, Bart Van Assche wrote:
>> @@ -1624,7 +1627,8 @@ qla2x00_abort_all_cmds(scsi_qla_host_t *vha, int res)
>>  					 */
>>  					sp_get(sp);
>>  					spin_unlock_irqrestore(&ha->hardware_lock, flags);
>> -					qla2xxx_eh_abort(GET_CMD_SP(sp));
>> +					if (scmd)
>> +						qla2xxx_eh_abort(scmd);
>>  					spin_lock_irqsave(&ha->hardware_lock, flags);
>>  				}
>
>Now, this chunk has a problem with reference counting (and unnecessary
>spin-locking), which we can avoid by simply moving up this NULL check.
>
>The call to sp_get() increments the sp->ref_count, but if you skip the
>call to qla2xxx_eh_abort() you don't get the decrement from the call to
>sp->done() at abort handling from ISR, e.g., qla24xx_abort_iocb_entry().
>[or if the command completed successfully between issue/complete abort,
>at the completion from ISR, e.g., qla2x00_process_completed_request().]
>
>The sp->done() call just below this chunk was supposed to drop the
>initial reference [set at qla2xxx_queuecommand()] at a time we did
>not call qla2xxx_eh_abort() yet... but now that we __may__ call it
>(and get that sp->done() call from the ISR abort handling), we need
>to only increment it if we're going to drop it.
>
>That should be resolved with this slight change to your patch
>(which also helps w/ the spin-locking).  What do you/others think?
>
>diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
>index 0a000ecf0881..a17cb63b3fd5 100644
>--- a/drivers/scsi/qla2xxx/qla_os.c
>+++ b/drivers/scsi/qla2xxx/qla_os.c
>@@ -1600,6 +1600,7 @@ uint32_t qla2x00_isp_reg_stat(struct qla_hw_data *ha)
>         srb_t *sp;
>         struct qla_hw_data *ha = vha->hw;
>         struct req_que *req;
>+       struct scsi_cmnd *scmd;
>
>         qlt_host_reset_handler(ha);
>
>@@ -1613,10 +1614,12 @@ uint32_t qla2x00_isp_reg_stat(struct qla_hw_data 
>*ha)
>                 for (cnt = 1; cnt < req->num_outstanding_cmds; cnt++) {
>                         sp = req->outstanding_cmds[cnt];
>                         if (sp) {
>+                               scmd = GET_CMD_SP(sp);
>+
>                                 /* Don't abort commands in adapter 
>during EEH
>                                  * recovery as it's not 
>accessible/responding.
>                                  */
>-                               if (!ha->flags.eeh_busy) {
>+                               if (scmd && !ha->flags.eeh_busy) {
>                                         /* Get a reference to the sp 
>and drop the lock.
>                                          * The reference ensures this 
>sp->done() call
>                                          * - and not the call in 
>qla2xxx_eh_abort() -
>@@ -1624,7 +1627,7 @@ uint32_t qla2x00_isp_reg_stat(struct qla_hw_data *ha)
>                                          */
>                                         sp_get(sp);
> 
>spin_unlock_irqrestore(&ha->hardware_lock, flags);
>-                                       qla2xxx_eh_abort(GET_CMD_SP(sp));
>+                                       qla2xxx_eh_abort(scmd);
> 
>spin_lock_irqsave(&ha->hardware_lock, flags);
>                                 }
>                                 req->outstanding_cmds[cnt] = NULL;
>
>
>Signed-off-by: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>
>
>
>-- 
>Mauricio Faria de Oliveira
>IBM Linux Technology Center

This is more appropriate fix. Looks good.

Re: [PATCH 2/2] qla2xxx: Avoid that issuing a LIP triggers a kernel crash

[Martin K. Petersen]

>>>>> "Mauricio" == Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com> writes:

Hi Mauricio,

Mauricio> First of all, sorry for the new bug; I didn't realize the
Mauricio> pointer could be NULL at this scenario.

Please do a proper patch submission for this fix.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 2/2] qla2xxx: Avoid that issuing a LIP triggers a kernel crash

[Mauricio Faria de Oliveira]

Hi Martin,

On 01/25/2017 09:29 PM, Martin K. Petersen wrote:
> Please do a proper patch submission for this fix.

Okay, I submitted a v2 patch w/ the suggested change.

However, the original patch has been submitted by Bart,
so I believe credit is due, but not sure how to handle
this case.

Thus, please feel free to change the sign-off line as
appropriate here.

Thanks,

-- 
Mauricio Faria de Oliveira
IBM Linux Technology Center