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

From: John Garry
Date: Wed Sep 19 2018 - 04:40:01 EST

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

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()


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
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);

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");
- 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.


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