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=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable 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 12305C433B4 for ; Sat, 10 Apr 2021 07:30:57 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id A118C61165 for ; Sat, 10 Apr 2021 07:30:56 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A118C61165 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 13BA06B006E; Sat, 10 Apr 2021 03:30:56 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 1132C6B0070; Sat, 10 Apr 2021 03:30:56 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id F1C896B0071; Sat, 10 Apr 2021 03:30:55 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0008.hostedemail.com [216.40.44.8]) by kanga.kvack.org (Postfix) with ESMTP id D340A6B006E for ; Sat, 10 Apr 2021 03:30:55 -0400 (EDT) Received: from smtpin16.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 93F9B1845F4A9 for ; Sat, 10 Apr 2021 07:30:55 +0000 (UTC) X-FDA: 78015635670.16.6B9B926 Received: from szxga05-in.huawei.com (szxga05-in.huawei.com [45.249.212.191]) by imf30.hostedemail.com (Postfix) with ESMTP id 8CDB7E000105 for ; Sat, 10 Apr 2021 07:30:46 +0000 (UTC) Received: from DGGEMS405-HUB.china.huawei.com (unknown [172.30.72.59]) by szxga05-in.huawei.com (SkyGuard) with ESMTP id 4FHRRf6LDSzNvPf; Sat, 10 Apr 2021 15:27:58 +0800 (CST) Received: from [10.174.179.9] (10.174.179.9) by DGGEMS405-HUB.china.huawei.com (10.3.19.205) with Microsoft SMTP Server id 14.3.498.0; Sat, 10 Apr 2021 15:30:46 +0800 Subject: Re: [PATCH] mm/frontswap: fix frontswap_register_ops() race with swapon and swapoff To: , CC: , , References: <20210405102008.25504-1-linmiaohe@huawei.com> From: Miaohe Lin Message-ID: <5ec0f24e-eac2-1e62-628b-757f8ce11dc2@huawei.com> Date: Sat, 10 Apr 2021 15:30:46 +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: <20210405102008.25504-1-linmiaohe@huawei.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-Stat-Signature: jqgf69hgdfb5t9f7p6nnz14965495fu7 X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 8CDB7E000105 Received-SPF: none (huawei.com>: No applicable sender policy available) receiver=imf30; identity=mailfrom; envelope-from=""; helo=szxga05-in.huawei.com; client-ip=45.249.212.191 X-HE-DKIM-Result: none/none X-HE-Tag: 1618039846-536891 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: Hi: On 2021/4/5 18:20, Miaohe Lin wrote: > frontswap_register_ops can race with swapon. Consider the following scene: Any comment or suggestion? Or is this race window too theoretical to fix? Thanks. > > CPU1 CPU2 > ---- ---- > frontswap_register_ops > fill bitmap a > ops->init > sys_swapon > enable_swap_info > frontswap_init without new ops > add ops to frontswap_ops list > check if swap_active_head changed > add to swap_active_head > > So the frontswap_ops init is missed on the new swap device. Consider the > another scene: > CPU1 CPU2 > ---- ---- > frontswap_register_ops > fill bitmap a > ops->init > add ops to frontswap_ops list > sys_swapon > enable_swap_info > frontswap_init with new ops > add to swap_active_head > check if swap_active_head changed > ops->init for new swap device [twice!] > > The frontswap_ops init will be called two times on the new swap device this > time. frontswap_register_ops can also race with swapoff. Consider the > following scene: > > CPU1 CPU2 > ---- ---- > sys_swapoff > removed from swap_active_head > frontswap_register_ops > fill bitmap a > ops->init without swap device > add ops to frontswap_ops list > invalidate_area with new ops > check if swap_active_head changed > > We could call invalidate_area on a swap device under swapoff with frontswap > is uninitialized yet. Fix all these by using swapon_mutex to guard against > race with swapon and add swap_info_get_if_under_swapoff() to collect swap > devices under swapoff. > > Fixes: d1dc6f1bcf1e ("frontswap: allow multiple backends") > Signed-off-by: Miaohe Lin > --- > include/linux/swapfile.h | 2 ++ > mm/frontswap.c | 40 +++++++++++++++++----------------------- > mm/swapfile.c | 13 ++++++++++++- > 3 files changed, 31 insertions(+), 24 deletions(-) > > diff --git a/include/linux/swapfile.h b/include/linux/swapfile.h > index e06febf62978..7ae15d917828 100644 > --- a/include/linux/swapfile.h > +++ b/include/linux/swapfile.h > @@ -9,8 +9,10 @@ > extern spinlock_t swap_lock; > extern struct plist_head swap_active_head; > extern struct swap_info_struct *swap_info[]; > +extern struct mutex swapon_mutex; > extern int try_to_unuse(unsigned int, bool, unsigned long); > extern unsigned long generic_max_swapfile_size(void); > extern unsigned long max_swapfile_size(void); > +extern struct swap_info_struct *swap_info_get_if_under_swapoff(int type); > > #endif /* _LINUX_SWAPFILE_H */ > diff --git a/mm/frontswap.c b/mm/frontswap.c > index 130e301c5ac0..c16bfc7550b5 100644 > --- a/mm/frontswap.c > +++ b/mm/frontswap.c > @@ -123,12 +123,26 @@ void frontswap_register_ops(struct frontswap_ops *ops) > > bitmap_zero(a, MAX_SWAPFILES); > bitmap_zero(b, MAX_SWAPFILES); > - > + mutex_lock(&swapon_mutex); > spin_lock(&swap_lock); > plist_for_each_entry(si, &swap_active_head, list) { > if (!WARN_ON(!si->frontswap_map)) > set_bit(si->type, a); > } > + /* > + * There might be some swap devices under swapoff, i.e. they are > + * removed from swap_active_head but frontswap_invalidate_area() > + * is not called yet due to swapon_mutex is held here. We must > + * collect these swap devices and call ops->init on them or they > + * might invalidate frontswap area while frontswap is uninitialized. > + */ > + for_each_clear_bit(i, a, MAX_SWAPFILES) { > + si = swap_info_get_if_under_swapoff(i); > + if (!si || !si->frontswap_map) > + continue; > + set_bit(si->type, b); > + } > + bitmap_or(a, a, b, MAX_SWAPFILES); > spin_unlock(&swap_lock); > > /* the new ops needs to know the currently active swap devices */ > @@ -144,29 +158,9 @@ void frontswap_register_ops(struct frontswap_ops *ops) > ops->next = frontswap_ops; > } while (cmpxchg(&frontswap_ops, ops->next, ops) != ops->next); > > - static_branch_inc(&frontswap_enabled_key); > - > - spin_lock(&swap_lock); > - plist_for_each_entry(si, &swap_active_head, list) { > - if (si->frontswap_map) > - set_bit(si->type, b); > - } > - spin_unlock(&swap_lock); > + mutex_unlock(&swapon_mutex); > > - /* > - * On the very unlikely chance that a swap device was added or > - * removed between setting the "a" list bits and the ops init > - * calls, we re-check and do init or invalidate for any changed > - * bits. > - */ > - if (unlikely(!bitmap_equal(a, b, MAX_SWAPFILES))) { > - for (i = 0; i < MAX_SWAPFILES; i++) { > - if (!test_bit(i, a) && test_bit(i, b)) > - ops->init(i); > - else if (test_bit(i, a) && !test_bit(i, b)) > - ops->invalidate_area(i); > - } > - } > + static_branch_inc(&frontswap_enabled_key); > } > EXPORT_SYMBOL(frontswap_register_ops); > > diff --git a/mm/swapfile.c b/mm/swapfile.c > index 149e77454e3c..ee736533717f 100644 > --- a/mm/swapfile.c > +++ b/mm/swapfile.c > @@ -89,7 +89,7 @@ static DEFINE_SPINLOCK(swap_avail_lock); > > struct swap_info_struct *swap_info[MAX_SWAPFILES]; > > -static DEFINE_MUTEX(swapon_mutex); > +DEFINE_MUTEX(swapon_mutex); > > static DECLARE_WAIT_QUEUE_HEAD(proc_poll_wait); > /* Activity counter to indicate that a swapon or swapoff has occurred */ > @@ -2958,6 +2958,17 @@ __weak unsigned long max_swapfile_size(void) > return generic_max_swapfile_size(); > } > > +struct swap_info_struct *swap_info_get_if_under_swapoff(int type) > +{ > + struct swap_info_struct *si = swap_type_to_swap_info(type); > + > + if (!si || !si->swap_map) > + return NULL; > + if ((si->flags & SWP_USED) && !(si->flags & SWP_WRITEOK)) > + return si; > + return NULL; > +} > + > static unsigned long read_swap_header(struct swap_info_struct *p, > union swap_header *swap_header, > struct inode *inode) >