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,URIBL_BLOCKED, USER_AGENT_SANE_1 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 6D953C433ED for ; Thu, 13 May 2021 08:49:18 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id E1BDD60232 for ; Thu, 13 May 2021 08:49:17 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E1BDD60232 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 61D946B0036; Thu, 13 May 2021 04:49:17 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 5CDC76B006E; Thu, 13 May 2021 04:49:17 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 46E146B0070; Thu, 13 May 2021 04:49:17 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0061.hostedemail.com [216.40.44.61]) by kanga.kvack.org (Postfix) with ESMTP id 176F86B0036 for ; Thu, 13 May 2021 04:49:17 -0400 (EDT) Received: from smtpin16.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id A4340B9E0 for ; Thu, 13 May 2021 08:49:16 +0000 (UTC) X-FDA: 78135583512.16.4045CC6 Received: from szxga07-in.huawei.com (szxga07-in.huawei.com [45.249.212.35]) by imf09.hostedemail.com (Postfix) with ESMTP id 3E5796000130 for ; Thu, 13 May 2021 08:49:03 +0000 (UTC) Received: from DGGEMS404-HUB.china.huawei.com (unknown [172.30.72.60]) by szxga07-in.huawei.com (SkyGuard) with ESMTP id 4Fgld03Fr7zBtcQ; Thu, 13 May 2021 16:46:28 +0800 (CST) Received: from [10.174.176.232] (10.174.176.232) by DGGEMS404-HUB.china.huawei.com (10.3.19.204) with Microsoft SMTP Server id 14.3.498.0; Thu, 13 May 2021 16:49:07 +0800 Subject: Re: [PATCH] mm, swap: Remove unnecessary smp_rmb() in swap_type_to_swap_info() To: Huang Ying , Andrew Morton CC: , , Daniel Jordan , Dan Carpenter , Andrea Parri , Peter Zijlstra , Andi Kleen , Dave Hansen , Omar Sandoval , Paul McKenney , Tejun Heo , Will Deacon References: <20210513064837.3949064-1-ying.huang@intel.com> From: Miaohe Lin Message-ID: <176421ba-77d9-8422-21e0-b5e2951533bd@huawei.com> Date: Thu, 13 May 2021 16:49:07 +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: <20210513064837.3949064-1-ying.huang@intel.com> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.174.176.232] X-CFilter-Loop: Reflected X-Rspamd-Queue-Id: 3E5796000130 Authentication-Results: imf09.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=huawei.com; spf=pass (imf09.hostedemail.com: domain of linmiaohe@huawei.com designates 45.249.212.35 as permitted sender) smtp.mailfrom=linmiaohe@huawei.com X-Rspamd-Server: rspam04 X-Stat-Signature: 59y775es6szx8y7jzpozers6wkki3qrk Received-SPF: none (huawei.com>: No applicable sender policy available) receiver=imf09; identity=mailfrom; envelope-from=""; helo=szxga07-in.huawei.com; client-ip=45.249.212.35 X-HE-DKIM-Result: none/none X-HE-Tag: 1620895743-103286 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/5/13 14:48, Huang Ying wrote: > Before commit c10d38cc8d3e ("mm, swap: bounds check swap_info array > accesses to avoid NULL derefs"), the typical code to reference the > swap_info[] is as follows, > > type = swp_type(swp_entry); > if (type >= nr_swapfiles) > /* handle invalid swp_entry */; > p = swap_info[type]; > /* access fields of *p. OOPS! p may be NULL! */ > > Because the ordering isn't guaranteed, it's possible that "p" is read > before checking "type". And that may result in NULL pointer > dereference. > > So in commit c10d38cc8d3e, the code becomes, > > struct swap_info_struct *swap_type_to_swap_info(int type) > { > if (type >= READ_ONCE(nr_swapfiles)) > return NULL; > smp_rmb(); > return READ_ONCE(swap_info[type]); > } > > /* users */ > type = swp_type(swp_entry); > p = swap_type_to_swap_info(type); > if (!p) > /* handle invalid swp_entry */; > /* access fields of *p */ > > Because "p" is checked to be non-zero before dereference, smp_rmb() > isn't needed anymore. > > We still need to guarantee swap_info[type] is read before dereference. > That can be satisfied via the data dependency ordering of > READ_ONCE(swap_info[type]). The corresponding smp_wmb() is adjusted > in alloc_swap_info() too. > > And, we don't need to read "nr_swapfiles" too. Because if > "type >= nr_swapfiles", swap_info[type] will be NULL. We just need > to make sure we will not access out of the boundary of the array. > With that change, nr_swapfiles will only be accessed with swap_lock > held, except in swapcache_free_entries(). Where the absolute > correctness of the value isn't needed, as described in the comments. > > Signed-off-by: "Huang, Ying" > Cc: Daniel Jordan > Cc: Dan Carpenter > Cc: Andrea Parri > Cc: Peter Zijlstra (Intel) > Cc: Andi Kleen > Cc: Dave Hansen > Cc: Omar Sandoval > Cc: Paul McKenney > Cc: Tejun Heo > Cc: Will Deacon > Cc: Miaohe Lin > --- > mm/swapfile.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/mm/swapfile.c b/mm/swapfile.c > index 2aad85751991..4c1fb28bbe0e 100644 > --- a/mm/swapfile.c > +++ b/mm/swapfile.c > @@ -100,10 +100,14 @@ atomic_t nr_rotate_swap = ATOMIC_INIT(0); > > static struct swap_info_struct *swap_type_to_swap_info(int type) > { > - if (type >= READ_ONCE(nr_swapfiles)) > + if (type >= MAX_SWAPFILES) > return NULL; > > - smp_rmb(); /* Pairs with smp_wmb in alloc_swap_info. */ > + /* > + * The data dependency ordering from the READ_ONCE() pairs > + * with smp_wmb() in alloc_swap_info() to guarantee the > + * swap_info_struct fields are read after swap_info[type]. > + */ > return READ_ONCE(swap_info[type]); > } > > @@ -2884,14 +2888,10 @@ static struct swap_info_struct *alloc_swap_info(void) > } > if (type >= nr_swapfiles) { > p->type = type; > - WRITE_ONCE(swap_info[type], p); > - /* > - * Write swap_info[type] before nr_swapfiles, in case a > - * racing procfs swap_start() or swap_next() is reading them. > - * (We never shrink nr_swapfiles, we never free this entry.) > - */ > + /* Paired with READ_ONCE() in swap_type_to_swap_info() */ > smp_wmb(); Many thank for your patch. The patch looks fine to me. There is one question: There is no smp_rmb() paired with above smp_wmb(). What is this smp_wmb() used for ? Could you please have a explanation ? Thanks again! > - WRITE_ONCE(nr_swapfiles, nr_swapfiles + 1); > + WRITE_ONCE(swap_info[type], p); > + nr_swapfiles++; > } else { > defer = p; > p = swap_info[type]; >