Re: [PATCH 5/5] scsi: libsas: fix a race condition when smp task timeout

[John Garry]

On 19/09/2018 03:49, Jason Yan wrote:
> On 2018/9/17 17:47, John Garry wrote:
> > On 12/09/2018 09:29, Jason Yan wrote:
> > > When the lldd is processing the complete sas task in interrupt and set
> > > the task stat as SAS_TASK_STATE_DONE, the smp timeout timer is able to
> > > be triggered at the same time. And smp_task_timedout() will complete the
> > > task wheter the SAS_TASK_STATE_DONE is set or not. Then the sas task may
> > > freed before lldd end the interrupt process. Thus a use-after-free will
> > > happen.
> > >
> > > Fix this by calling the complete() only when SAS_TASK_STATE_DONE is not
> > > set. And remove the check of the return value of the del_timer().
> >
> > Hi Jason,
> >
> > Please mention that once the LLDD sets DONE, it must call task->done(),
> > which will call smp_task_done()->complete()
>
> OK
>
> > > Reported-by: chenxiang <chenxiang66@xxxxxxxxxxxxx>
> > > Signed-off-by: Jason Yan <yanaijie@xxxxxxxxxx>
> > > CC: John Garry <john.garry@xxxxxxxxxx>
> > > CC: Johannes Thumshirn <jthumshirn@xxxxxxx>
> > > CC: Ewan Milne <emilne@xxxxxxxxxx>
> > > CC: Christoph Hellwig <hch@xxxxxx>
> > > CC: Tomas Henzl <thenzl@xxxxxxxxxx>
> > > CC: Dan Williams <dan.j.williams@xxxxxxxxx>
> > > CC: Hannes Reinecke <hare@xxxxxxxx>
> > > ---
> > >  drivers/scsi/libsas/sas_expander.c | 9 ++++-----
> > >  1 file changed, 4 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/scsi/libsas/sas_expander.c
> > > b/drivers/scsi/libsas/sas_expander.c
> > > index 52222940d398..0d1f72752ca2 100644
> > > --- a/drivers/scsi/libsas/sas_expander.c
> > > +++ b/drivers/scsi/libsas/sas_expander.c
> > > @@ -48,17 +48,16 @@ static void smp_task_timedout(struct timer_list *t)
> > >      unsigned long flags;
> > >
> > >      spin_lock_irqsave(&task->task_state_lock, flags);
> > > -    if (!(task->task_state_flags & SAS_TASK_STATE_DONE))
> > > +    if (!(task->task_state_flags & SAS_TASK_STATE_DONE)) {
> > >          task->task_state_flags |= SAS_TASK_STATE_ABORTED;
> > > +        complete(&task->slow_task->completion);
> >
> > Nit: for consistency with any other time we use this lock, can we call
> > complete() outside the lock? Maybe just use a flag variable for this.
>
> Is it necessary to add a variable just for consistency with other places?

Actually other places don't only check/set bits in the flag, so it's ok

> > > +    }
> > >      spin_unlock_irqrestore(&task->task_state_lock, flags);
> > > -
> > > -    complete(&task->slow_task->completion);
> > >  }
> > >
> > >  static void smp_task_done(struct sas_task *task)
> > >  {
> > > -    if (!del_timer(&task->slow_task->timer))
> > > -        return;
> > > +    del_timer(&task->slow_task->timer);
> > >      complete(&task->slow_task->completion);
> > >  }
> >
> > Do we also need this change or similar:
> > static int smp_execute_task_sg(struct domain_device *dev,
> >      if ((task->task_state_flags & SAS_TASK_STATE_ABORTED)) {
> >          SAS_DPRINTK("smp task timed out or aborted\n");
> >          i->dft->lldd_abort_task(task);
> > -        if (!(task->task_state_flags & SAS_TASK_STATE_DONE)) {
> > -            SAS_DPRINTK("SMP task aborted and not done\n");
> > -            break;
> > -        }
> > +        break;
> >
> > To me, the ABORTED and DONE states are mutually exclusive.
>
> This changes the logic a bit.

Does it really? According to above change, if state ABORTED set then 
should not have DONE set also.

Anyway, according to my analysis, this suggestion in affect only removes 
the print, which holds true.

Thanks,
John

To be safe, maybe we shall do this with
> another patch after some tests.



> > >
> >
> > Thanks,
> > John
> >
> >
> >
> > .
>
>
> .