From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752136AbdFVOZK (ORCPT ); Thu, 22 Jun 2017 10:25:10 -0400 Received: from szxga02-in.huawei.com ([45.249.212.188]:8817 "EHLO szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751095AbdFVOZJ (ORCPT ); Thu, 22 Jun 2017 10:25:09 -0400 Subject: Re: [PATCH] qla2xxx: Protect access to qpair members with qpair->qp_lock To: Johannes Thumshirn , "Martin K . Petersen" References: <20170622134325.26931-1-jthumshirn@suse.de> CC: Linux SCSI Mailinglist , "Linux Kernel Mailinglist" , , Himanshu Madhani From: John Garry Message-ID: Date: Thu, 22 Jun 2017 15:19:27 +0100 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <20170622134325.26931-1-jthumshirn@suse.de> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.203.181.162] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020202.594BD1F7.01D3,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 066042a8d2abf9f0ae8cf2af5d19582d Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 22/06/2017 14:43, Johannes Thumshirn wrote: > In qla2xx_start_scsi_mq() and qla2xx_dif_start_scsi_mq() we grab the > qpair->qp_lock but do access members of the qpair before having the lock. > Re-order the locking sequence to have all read and write access to qpair > members under the qpair->qp_lock. > > Signed-off-by: Johannes Thumshirn > --- > drivers/scsi/qla2xxx/qla_iocb.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c > index 8404f17f3c6c..425ca1646a9a 100644 > --- a/drivers/scsi/qla2xxx/qla_iocb.c > +++ b/drivers/scsi/qla2xxx/qla_iocb.c > @@ -1770,10 +1770,6 @@ qla2xxx_start_scsi_mq(srb_t *sp) > struct qla_hw_data *ha = vha->hw; > struct qla_qpair *qpair = sp->qpair; > > - /* Setup qpair pointers */ > - rsp = qpair->rsp; > - req = qpair->req; Can you check the call to qla2x00_marker() before the spinlock grab, which takes rsp and req as parameters? > - > /* So we know we haven't pci_map'ed anything yet */ > tot_dsds = 0; > > @@ -1788,6 +1784,10 @@ qla2xxx_start_scsi_mq(srb_t *sp) > /* Acquire qpair specific lock */ > spin_lock_irqsave(&qpair->qp_lock, flags); > > + /* Setup qpair pointers */ > + rsp = qpair->rsp; > + req = qpair->req; > + > /* Check for room in outstanding command list. */ > handle = req->current_outstanding_cmd; > for (index = 1; index < req->num_outstanding_cmds; index++) { > @@ -1924,24 +1924,33 @@ qla2xxx_dif_start_scsi_mq(srb_t *sp) > > #define QDSS_GOT_Q_SPACE BIT_0 > > + /* Acquire ring specific lock */ > + spin_lock_irqsave(&qpair->qp_lock, flags); > + > /* Check for host side state */ > if (!qpair->online) { > cmd->result = DID_NO_CONNECT << 16; > + spin_unlock_irqrestore(&qpair->qp_lock, flags); > return QLA_INTERFACE_ERROR; > } > > if (!qpair->difdix_supported && > scsi_get_prot_op(cmd) != SCSI_PROT_NORMAL) { > cmd->result = DID_NO_CONNECT << 16; > + spin_unlock_irqrestore(&qpair->qp_lock, flags); > return QLA_INTERFACE_ERROR; > } > > + spin_unlock_irqrestore(&qpair->qp_lock, flags); > + > /* Only process protection or >16 cdb in this routine */ > if (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL) { > if (cmd->cmd_len <= 16) > return qla2xxx_start_scsi_mq(sp); > } > > + spin_lock_irqsave(&qpair->qp_lock, flags); > + > /* Setup qpair pointers */ > rsp = qpair->rsp; > req = qpair->req; > @@ -1957,9 +1966,6 @@ qla2xxx_dif_start_scsi_mq(srb_t *sp) > vha->marker_needed = 0; > } > > - /* Acquire ring specific lock */ > - spin_lock_irqsave(&qpair->qp_lock, flags); > - > /* Check for room in outstanding command list. */ > handle = req->current_outstanding_cmd; > for (index = 1; index < req->num_outstanding_cmds; index++) { >