From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3AD44C43460 for ; Sat, 10 Apr 2021 03:17:44 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 98B31610C8 for ; Sat, 10 Apr 2021 03:17:43 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 98B31610C8 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=huawei.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id EB3526B006C; Fri, 9 Apr 2021 23:17:42 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id E63BE6B006E; Fri, 9 Apr 2021 23:17:42 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D046E6B0070; Fri, 9 Apr 2021 23:17:42 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0100.hostedemail.com [216.40.44.100]) by kanga.kvack.org (Postfix) with ESMTP id B73ED6B006C for ; Fri, 9 Apr 2021 23:17:42 -0400 (EDT) Received: from smtpin03.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 776D7ABF4 for ; Sat, 10 Apr 2021 03:17:42 +0000 (UTC) X-FDA: 78014997564.03.4BC7636 Received: from szxga07-in.huawei.com (szxga07-in.huawei.com [45.249.212.35]) by imf18.hostedemail.com (Postfix) with ESMTP id 97D982000249 for ; Sat, 10 Apr 2021 03:17:41 +0000 (UTC) Received: from DGGEMS403-HUB.china.huawei.com (unknown [172.30.72.60]) by szxga07-in.huawei.com (SkyGuard) with ESMTP id 4FHKr94ZV4z9xF3; Sat, 10 Apr 2021 11:15:21 +0800 (CST) Received: from [10.174.179.9] (10.174.179.9) by DGGEMS403-HUB.china.huawei.com (10.3.19.203) with Microsoft SMTP Server id 14.3.498.0; Sat, 10 Apr 2021 11:17:29 +0800 Subject: Re: [PATCH 2/5] swap: fix do_swap_page() race with swapoff To: Tim Chen , CC: , , , , , , , , , , , References: <20210408130820.48233-1-linmiaohe@huawei.com> <20210408130820.48233-3-linmiaohe@huawei.com> <7684b3de-2824-9b1f-f033-d4bc14f9e195@linux.intel.com> <50d34b02-c155-bad7-da1f-03807ad31275@huawei.com> <995a130b-f07a-4771-1fe3-477d2f3c1e8e@linux.intel.com> From: Miaohe Lin Message-ID: Date: Sat, 10 Apr 2021 11:17:29 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: <995a130b-f07a-4771-1fe3-477d2f3c1e8e@linux.intel.com> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.174.179.9] X-CFilter-Loop: Reflected X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: 97D982000249 X-Stat-Signature: m1ughf99hsyd7ghodnhynzxw66tqq8zp Received-SPF: none (huawei.com>: No applicable sender policy available) receiver=imf18; identity=mailfrom; envelope-from=""; helo=szxga07-in.huawei.com; client-ip=45.249.212.35 X-HE-DKIM-Result: none/none X-HE-Tag: 1618024661-159202 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 2021/4/10 1:17, Tim Chen wrote: > > > On 4/9/21 1:42 AM, Miaohe Lin wrote: >> On 2021/4/9 5:34, Tim Chen wrote: >>> >>> >>> On 4/8/21 6:08 AM, Miaohe Lin wrote: >>>> When I was investigating the swap code, I found the below possible race >>>> window: >>>> >>>> CPU 1 CPU 2 >>>> ----- ----- >>>> do_swap_page >>>> synchronous swap_readpage >>>> alloc_page_vma >>>> swapoff >>>> release swap_file, bdev, or ... >>> >> >> Many thanks for quick review and reply! >> >>> Perhaps I'm missing something. The release of swap_file, bdev etc >>> happens after we have cleared the SWP_VALID bit in si->flags in destroy_swap_extents >>> if I read the swapoff code correctly. >> Agree. Let's look this more close: >> CPU1 CPU2 >> ----- ----- >> swap_readpage >> if (data_race(sis->flags & SWP_FS_OPS)) { >> swapoff >> p->swap_file = NULL; >> struct file *swap_file = sis->swap_file; >> struct address_space *mapping = swap_file->f_mapping;[oops!] >> ... >> p->flags = 0; >> ... >> >> Does this make sense for you? > > p->swapfile = NULL happens after the > p->flags &= ~SWP_VALID, synchronize_rcu(), destroy_swap_extents() sequence in swapoff(). > > So I don't think the sequence you illustrated on CPU2 is in the right order. > That said, without get_swap_device/put_swap_device in swap_readpage, you could > potentially blow pass synchronize_rcu() on CPU2 and causes a problem. so I think > the problematic race looks something like the following: > > > CPU1 CPU2 > ----- ----- > swap_readpage > if (data_race(sis->flags & SWP_FS_OPS)) { > swapoff > p->flags = &= ~SWP_VALID; > .. > synchronize_rcu(); > .. > p->swap_file = NULL; > struct file *swap_file = sis->swap_file; > struct address_space *mapping = swap_file->f_mapping;[oops!] > ... > ... > Agree. This is also what I meant to illustrate. And you provide a better one. Many thanks! > By adding get_swap_device/put_swap_device, then the race is fixed. > > > CPU1 CPU2 > ----- ----- > swap_readpage > get_swap_device() > .. > if (data_race(sis->flags & SWP_FS_OPS)) { > swapoff > p->flags = &= ~SWP_VALID; > .. > struct file *swap_file = sis->swap_file; > struct address_space *mapping = swap_file->f_mapping;[valid value] > .. > put_swap_device() > synchronize_rcu(); > .. > p->swap_file = NULL; > > >> >>>> >>>> swap_readpage >>>> check sis->flags is ok >>>> access swap_file, bdev...[oops!] >>>> si->flags = 0 >>> >>> This happens after we clear the si->flags >>> synchronize_rcu() >>> release swap_file, bdev, in destroy_swap_extents() >>> >>> So I think if we have get_swap_device/put_swap_device in do_swap_page, >>> it should fix the race you've pointed out here. >>> Then synchronize_rcu() will wait till we have completed do_swap_page and >>> call put_swap_device. >> >> Right, get_swap_device/put_swap_device could fix this race. __But__ rcu_read_lock() >> in get_swap_device() could disable preempt and do_swap_page() may take a really long >> time because it involves I/O. It may not be acceptable to disable preempt for such a >> long time. :( > > I can see that it is not a good idea to hold rcu read lock for a long > time over slow file I/O operation, which will be the side effect of > introducing get/put_swap_device to swap_readpage. So using percpu_ref > will then be preferable for synchronization once we introduce > get/put_swap_device into swap_readpage. > The sis->bdev should also be protected by get/put_swap_device. It has the similar issue. And swap_slot_free_notify (called from callback end_swap_bio_read) would race with swapoff too. So I use get/put_swap_device to protect swap_readpage until file I/O operation is completed. Thanks again! > Tim > . >