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=-23.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_IN_DEF_DKIM_WL autolearn=ham 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 495EAC433ED for ; Sat, 10 Apr 2021 20:02:54 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id ABC27610C8 for ; Sat, 10 Apr 2021 20:02:53 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org ABC27610C8 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 0D0FD8E0002; Sat, 10 Apr 2021 16:02:53 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 082108E0001; Sat, 10 Apr 2021 16:02:53 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E3D3E8E0002; Sat, 10 Apr 2021 16:02:52 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0169.hostedemail.com [216.40.44.169]) by kanga.kvack.org (Postfix) with ESMTP id C7A238E0001 for ; Sat, 10 Apr 2021 16:02:52 -0400 (EDT) Received: from smtpin08.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 719AF18069D48 for ; Sat, 10 Apr 2021 20:02:52 +0000 (UTC) X-FDA: 78017530584.08.6CC6916 Received: from mail-wm1-f53.google.com (mail-wm1-f53.google.com [209.85.128.53]) by imf23.hostedemail.com (Postfix) with ESMTP id BC2E4A00039C for ; Sat, 10 Apr 2021 20:02:50 +0000 (UTC) Received: by mail-wm1-f53.google.com with SMTP id n11-20020a05600c4f8bb029010e5cf86347so6616382wmq.1 for ; Sat, 10 Apr 2021 13:02:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=KQVc6ALPRczTIMmLPJFtRRr0DNOfBBNxvSVhyFFM4K4=; b=hr0iJ1WtsIlMpe2hr16+8h88/1pin2I46eCM682iC5gJjB19XCUjyOBBMy+cIQvPcb ok1gaC1AEfhOQ7r2myEaAwnmyPi9DbpyYGRKH+W/vigx48Cl7dT38alxbvtAEP+VM25o MaCagMU5eLfWaN3SY0KwFadIMXf1RUFNDjywVjv5kx6Jsaqrrsj7TESduWcOXOuv6jfl 6g/JmiFVRPrDG1Z3L9hncbNzSxv2zjrlnCqKPiVV5qbWfLfDm7ytS6z0w+JAQ0GFaJdk rkU9/m+W6Fy+CRFvcs6E20SdneoX2GfxQ1HHCRreZSRNdrJTRZ066f+jtcgrIuYLoW2O JovQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=KQVc6ALPRczTIMmLPJFtRRr0DNOfBBNxvSVhyFFM4K4=; b=TXDtj3DE6P4va25jWmtO26xmlDLVS1YIJ3FvMQhs63uiuQclfvysWUxwXG9l5y6lgU ve27I5uPPJmQ1Bo/BuOk6eSP7BZF4ou5wu7rjNSUy2kvUVoXQdzoNjUdUN1wokUA1CQY K4nIzzUVyW6tClItcSd17YuJIKZ7PLrtAINQHl6J4lu8CzjoxlZXHbOYkYjQ41mxJj8G /Lq+hqHJfqeSiQKYcfBabaEzkcJEaf3pr0rvI+ui8HfSHcLjcHLukewqKkkCAULnfdHf dAcdFvtxIjJ8aByozlcwW6KZXb330Aph5ioPGAUBN3oMH3LxgY2dgJ4WmCpx+u3hhokS AqxA== X-Gm-Message-State: AOAM533i6uwbLzjaRHO9t1CL/KTSET6nHxziZ/LD8M46gMi7/c5oINOj +/bG+NvVNoUVmdmTc+zuqUXRCN6ggkVm+YI5vIGTkA== X-Google-Smtp-Source: ABdhPJzPg6vFeaJ5urVg1wxZjP90byCsCiZUy7mnevv/n5dldzigX7Hqf5xNy7cKaguyCo4LjODdsuEOR6oGSzPKIB8= X-Received: by 2002:a1c:c918:: with SMTP id f24mr19844443wmb.12.1618084970674; Sat, 10 Apr 2021 13:02:50 -0700 (PDT) MIME-Version: 1.0 References: <20210405102008.25504-1-linmiaohe@huawei.com> <5ec0f24e-eac2-1e62-628b-757f8ce11dc2@huawei.com> <7e5ea991-f219-5b42-ee71-4ec4004ca550@huawei.com> In-Reply-To: <7e5ea991-f219-5b42-ee71-4ec4004ca550@huawei.com> From: Yu Zhao Date: Sat, 10 Apr 2021 14:02:39 -0600 Message-ID: Subject: Re: [PATCH] mm/frontswap: fix frontswap_register_ops() race with swapon and swapoff To: Miaohe Lin Cc: Andrew Morton , konrad.wilk@oracle.com, Dan Streetman , linux-kernel , Linux-MM Content-Type: text/plain; charset="UTF-8" X-Stat-Signature: cwcnt5pcsio95dpymk5tk58n1kwyfpmw X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: BC2E4A00039C Received-SPF: none (google.com>: No applicable sender policy available) receiver=imf23; identity=mailfrom; envelope-from=""; helo=mail-wm1-f53.google.com; client-ip=209.85.128.53 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1618084970-211296 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 Sat, Apr 10, 2021 at 5:01 AM Miaohe Lin wrote: > > On 2021/4/10 18:42, Yu Zhao wrote: > > On Sat, Apr 10, 2021 at 1:30 AM Miaohe Lin wrote: > >> > >> 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. > > > > Let me run a stress test and get back to you (within a day or so). > > That's very kind of you. Many thanks! I'm still getting the following crash. Probably I should try the other series you sent earlier too? BUG: unable to handle page fault for address: ffffaa7937d82000 RIP: 0010:scan_swap_map_slots+0x12b/0x7f0 Call Trace: get_swap_pages+0x278/0x590 get_swap_page+0x1ab/0x280 add_to_swap+0x7d/0x130 shrink_page_list+0xf84/0x25f0 reclaim_pages+0x313/0x430 madvise_cold_or_pageout_pte_range+0x95c/0xaa0 walk_p4d_range+0x43f/0x790 walk_pgd_range+0xf1/0x150 __walk_page_range+0x6f/0x1b0 walk_page_range+0xbe/0x1e do_madvise+0x271/0x720 > >>> 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) > >>> > >> > >> > > . > > > >