From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751753AbbJYN3d (ORCPT ); Sun, 25 Oct 2015 09:29:33 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:39011 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750762AbbJYN3b (ORCPT ); Sun, 25 Oct 2015 09:29:31 -0400 Message-ID: <5c991223dc8eeefa92263c27b20c59b0.squirrel@www.codeaurora.org> In-Reply-To: References: <1441188795-4600-1-git-send-email-ygardi@codeaurora.org> <1441188795-4600-4-git-send-email-ygardi@codeaurora.org> Date: Sun, 25 Oct 2015 13:29:30 -0000 Subject: Re: [PATCH v3 03/15] scsi: ufs: verify command tag validity From: ygardi@codeaurora.org To: "Akinobu Mita" Cc: "Yaniv Gardi" , "Rob Herring" , "Jej B" , "Paul Bolle" , "Christoph Hellwig" , "LKML" , "linux-scsi@vger.kernel.org" , linux-arm-msm@vger.kernel.org, "Santosh Y" , linux-scsi-owner@vger.kernel.org, "Subhash Jadavani" , "Gilad Broner" , "Dolev Raviv" , "Vinayak Holikatti" , "James E.J. Bottomley" User-Agent: SquirrelMail/1.4.22-4.el6 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7BIT X-Priority: 3 (Normal) Importance: Normal Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > 2015-09-02 19:13 GMT+09:00 Yaniv Gardi : >> A race condition appear to exist between request completion when >> scsi_done() is called to end the request and set the tag back to >> -1 (at blk_queue_end_tag() scsi_end_request), and scsi layer error >> handling which aborts the command and reuses it to request sense >> data. Sending the request sense is done with tag which was set to -1 >> and so it is invalid. >> Assert command tag passed from scsi layer is valid. >> >> Signed-off-by: Gilad Broner >> Signed-off-by: Yaniv Gardi >> >> --- >> drivers/scsi/ufs/ufshcd.c | 24 ++++++++++++++++++++++-- >> 1 file changed, 22 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >> index 2d3ebca..8860a57 100644 >> --- a/drivers/scsi/ufs/ufshcd.c >> +++ b/drivers/scsi/ufs/ufshcd.c >> @@ -190,6 +190,10 @@ static int ufshcd_config_pwr_mode(struct ufs_hba >> *hba, >> struct ufs_pa_layer_attr *desired_pwr_mode); >> static int ufshcd_change_power_mode(struct ufs_hba *hba, >> struct ufs_pa_layer_attr *pwr_mode); >> +static inline bool ufshcd_valid_tag(struct ufs_hba *hba, int tag) >> +{ >> + return tag >= 0 && tag < hba->nutrs; >> +} >> >> static inline int ufshcd_enable_irq(struct ufs_hba *hba) >> { >> @@ -1310,6 +1314,12 @@ static int ufshcd_queuecommand(struct Scsi_Host >> *host, struct scsi_cmnd *cmd) >> hba = shost_priv(host); >> >> tag = cmd->request->tag; >> + if (!ufshcd_valid_tag(hba, tag)) { >> + dev_err(hba->dev, >> + "%s: invalid command tag %d: cmd=0x%p, >> cmd->request=0x%p", >> + __func__, tag, cmd, cmd->request); >> + BUG(); >> + } > > Is it better to avoid BUG() by WARN_ON() and return if possible? in this specific case, i think BUG() is the way to handle this scenario. It is very rare, and if invalid_tag is sent, the SW can not proceed, and it indicates something very wrong happened. either in the block layer that allocated the tag, or in the UFS that reported nutrs. So, if we actually hit this scenario, then recovering is not an option and i believe we need to BUG. hope it makes sense. > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >