Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1252887 > unrolled thread

Re: [PATCH v3 03/15] scsi: ufs: verify command tag validity

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 03/15] scsi: ufs: verify command tag validity Akinobu Mita <akinobu.mita@gmail.com> - 2015-10-21 16:50 +0200
    Re: [PATCH v3 03/15] scsi: ufs: verify command tag validity ygardi@codeaurora.org - 2015-10-25 14:30 +0100

#1252887 — Re: [PATCH v3 03/15] scsi: ufs: verify command tag validity

2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>:
> 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 <gbroner@codeaurora.org>
> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>
> ---
>  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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1255391

> 2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>:
>> 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 <gbroner@codeaurora.org>
>> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>>
>> ---
>>  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
>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web