Re: [PATCH 03/15] mpt3sas: Add sanity checks for scsi tracker before accessing it.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 2018-04-02 at 11:53 +0530, Sreekanth Reddy wrote:
> On Fri, Mar 30, 2018 at 5:29 PM, Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
> > On Fri, Mar 30, 2018 at 03:07:12PM +0530, Chaitra P B wrote:
> > > Check scsi tracker for NULL before accessing it.
> > > And in some places there are possibilities for getting valid st
> > > but still other fields are not set.
> > 
> > Please explain how that could ever happen.  You should never see
> > a scsi_cmnd without the device pointer.
> 
> Chris,
> 
> Here is one example, During Host reset operation time driver will
> flush out all the outstanding IO's to the SML in below function,
> 
> static void
> _scsih_flush_running_cmds(struct MPT3SAS_ADAPTER *ioc)
> {
>         struct scsi_cmnd *scmd;
>         struct scsiio_tracker *st;
>         u16 smid;
>         int count = 0;
> 
> [SR] Here driver is looping starting from smid one to HBA queue depth.
>         for (smid = 1; smid <= ioc->scsiio_depth; smid++) {
> 
> [SR] Some times it is possible that scsi_cmnd might have created at
> SML but it might not be issued to the driver as host reset operation
> is going on, So here we will get non-NULL scmd.
>                 scmd = mpt3sas_scsih_scsi_lookup_get(ioc, smid);
>                 if (!scmd)
>                         continue;
>                 count++;
>                 _scsih_set_satl_pending(scmd, false);
> [SR] Here we are trying to get the scsi tracker 'st' for the above
> scmd (which is not received by the driver) and so fields of this 'st'
> are uninitialized.
>                 st = scsi_cmd_priv(scmd);
> [SR] And here we are trying to clear the scsi tracker 'st' which is
> not yet all initialized by the driver, in other terms we are trying to
> flush out the scmd command which is not yet all received by the
> driver.
>                 mpt3sas_base_clear_st(ioc, st);
>                 scsi_dma_unmap(scmd);
>                 if (ioc->pci_error_recovery || ioc->remove_host)
>                         scmd->result = DID_NO_CONNECT << 16;
>                 else
>                         scmd->result = DID_RESET << 16;
>                 scmd->scsi_done(scmd);
>         }
>         dtmprintk(ioc, pr_info(MPT3SAS_FMT "completing %d cmds\n",
>             ioc->name, count));
> }

Hello Sreekanth,

>From mpt3sas_scsih_scsi_lookup_get():

			st = scsi_cmd_priv(scmd);
			if (st->cb_idx == 0xFF)
				scmd = NULL;

>From mpt3sas_base_get_smid(), mpt3sas_base_get_smid_scsiio() and
mpt3sas_base_get_smid_hpr():

	request->cb_idx = cb_idx;

Can _scsih_flush_running_cmds() be executed concurrently with scsih_qcmd()?
In other words, can it happen that mpt3sas_scsih_scsi_lookup_get() sees that
st->cb_idx == 0xff just before scsih_qcmd() assigns a value to .cb_idx? Can
this cause _scsih_flush_running_cmds() to skip commands that it shouldn't
skip?

Can it happen that _scsih_flush_running_cmds() calls .scsi_done() for a SCSI
command just after scsih_qcmd() transferred control for that command to the
firmware? Can that cause .scsi_done() to be called twice for the same command?

Thanks,

Bart.






[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]