Received: by 2002:ac0:a5a6:0:0:0:0:0 with SMTP id m35-v6csp465439imm; Wed, 19 Sep 2018 01:40:44 -0700 (PDT) X-Google-Smtp-Source: ANB0VdaGWvgH2h6He/gwwA6Q8ciqnLjIWvUlQ0dIXDt+SZaj24shludQW5e+FZnyLjftm5hO+Xxf X-Received: by 2002:a17:902:b902:: with SMTP id bf2-v6mr33595471plb.185.1537346444503; Wed, 19 Sep 2018 01:40:44 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1537346444; cv=none; d=google.com; s=arc-20160816; b=aMW8ZVCYAx0UfXHwFZB8uFVK5LeSVwJMfkezN3IuUar8ow8/dIhd1nparvea3D7OJZ 0JWYP5iKsBcAfpjlsOxYsFvNwIS1r9uumglmzAdw0YcgiIgDRIqH9z2jiGWuP1R5voJ9 /pzz6hhQYpbYIBbL7/1zhzs1HMlddwqzs+4qo0ckISiSnwQAimc8Q0T/Gw2z/JIcX1y0 AUm6kFwOj3O1UmwLmR27eBJZLJifZRURL9YgGUgOKCDBKRx6urX+M5bI0Umq3KYmwsiA zairGz3LYh6IqAJ5PygOHLg/DXZgPNsmkjuhuWvP1jrhKF8vsDDj9SPMcS+1r62uYED6 PjKw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:in-reply-to :mime-version:user-agent:date:message-id:from:cc:references:to :subject; bh=5cJua1TwZ5aRDahir1HHGZnlc9zcP8b6oHXGm/l1nBo=; b=hgnLFEcuVxfaDi+XntXt19OtWWw91CW8HUpoAs8suydwce9uLVZWFamo6DPRla8KJD WV98UoKbi1XrCW1JTTqJ8NUxsHYUdT1ehQ+562EgBQyV37eNLB8Ktq+yCAbgxNbjh4N3 kzCqgkC3o9zWC/Co1Z+FH7iPotllIhH4CcJ+9jcnuVlUoN1ShRTbjDnVkQ83wgpRxK2y sXNFMt1OdnVS8qOXT5EsdGyFDQV4MwvNvyTG7mPj8oyhiDf3gT5b2meWwgYhKS5Vd78y e2T+CPalrazW9nQhqemGlhiHlmXx5Lst0w7xfTef3sA7nlUrktSBJJJ6+rKoYl1JHDD9 2DAg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id x6-v6si19425973pgb.469.2018.09.19.01.40.28; Wed, 19 Sep 2018 01:40:44 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731039AbeISOQu (ORCPT + 99 others); Wed, 19 Sep 2018 10:16:50 -0400 Received: from szxga04-in.huawei.com ([45.249.212.190]:12618 "EHLO huawei.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726527AbeISOQu (ORCPT ); Wed, 19 Sep 2018 10:16:50 -0400 Received: from DGGEMS401-HUB.china.huawei.com (unknown [172.30.72.58]) by Forcepoint Email with ESMTP id 584EBB70C67F3; Wed, 19 Sep 2018 16:39:56 +0800 (CST) Received: from [127.0.0.1] (10.202.226.41) by DGGEMS401-HUB.china.huawei.com (10.3.19.201) with Microsoft SMTP Server id 14.3.399.0; Wed, 19 Sep 2018 16:39:47 +0800 Subject: Re: [PATCH 5/5] scsi: libsas: fix a race condition when smp task timeout To: Jason Yan , , References: <20180912082946.34814-1-yanaijie@huawei.com> <20180912082946.34814-6-yanaijie@huawei.com> <357ad1d1-1675-4479-c0c3-c872b5f71892@huawei.com> <5BA1B922.5010208@huawei.com> CC: , , , , , , , , , , Ewan Milne , Tomas Henzl , Linuxarm From: John Garry Message-ID: Date: Wed, 19 Sep 2018 09:39:38 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <5BA1B922.5010208@huawei.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.202.226.41] X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>> Signed-off-by: Jason Yan >>> CC: John Garry >>> CC: Johannes Thumshirn >>> CC: Ewan Milne >>> CC: Christoph Hellwig >>> CC: Tomas Henzl >>> CC: Dan Williams >>> CC: Hannes Reinecke >>> --- >>> 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 >> >> >> >> . >> > > > . >