From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:60551 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750795AbdAXPAO (ORCPT ); Tue, 24 Jan 2017 10:00:14 -0500 Received: from pps.filterd (m0098409.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v0OEnSKC028468 for ; Tue, 24 Jan 2017 10:00:13 -0500 Received: from e24smtp04.br.ibm.com (e24smtp04.br.ibm.com [32.104.18.25]) by mx0a-001b2d01.pphosted.com with ESMTP id 2860crebnr-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Tue, 24 Jan 2017 10:00:13 -0500 Received: from localhost by e24smtp04.br.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 24 Jan 2017 13:00:09 -0200 Subject: Re: [PATCH 2/2] qla2xxx: Avoid that issuing a LIP triggers a kernel crash To: Bart Van Assche , "Martin K . Petersen" References: <20170123163446.9227-1-bart.vanassche@sandisk.com> <20170123163446.9227-3-bart.vanassche@sandisk.com> Cc: linux-scsi@vger.kernel.org, Naresh Bannoth , Himanshu Madhani , stable@vger.kernel.org From: Mauricio Faria de Oliveira Date: Tue, 24 Jan 2017 12:59:57 -0200 MIME-Version: 1.0 In-Reply-To: <20170123163446.9227-3-bart.vanassche@sandisk.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Message-Id: <91a8f843-c969-72c4-1add-8e44ea2d9a8a@linux.vnet.ibm.com> Sender: stable-owner@vger.kernel.org List-ID: 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 -- Mauricio Faria de Oliveira IBM Linux Technology Center