From patchwork Thu May 20 07:33:01 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Huang, Ying" X-Patchwork-Id: 12269287 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=-16.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_GIT 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 AF67DC433B4 for ; Thu, 20 May 2021 07:33:34 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 414356135C for ; Thu, 20 May 2021 07:33:34 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 414356135C Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 4492A6B006C; Thu, 20 May 2021 03:33:33 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 3F92B6B006E; Thu, 20 May 2021 03:33:33 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 272FC6B0070; Thu, 20 May 2021 03:33:33 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0228.hostedemail.com [216.40.44.228]) by kanga.kvack.org (Postfix) with ESMTP id E66F36B006C for ; Thu, 20 May 2021 03:33:32 -0400 (EDT) Received: from smtpin36.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 7FD0FBBFE for ; Thu, 20 May 2021 07:33:32 +0000 (UTC) X-FDA: 78160794264.36.288A7EF Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by imf13.hostedemail.com (Postfix) with ESMTP id 9F04CE000129 for ; Thu, 20 May 2021 07:33:28 +0000 (UTC) IronPort-SDR: PJHKkD58DYa4dqc+mqYM4RjHwFAgcUih6wSS/4WIoMVvpOvoPO3j0zWQS3QOmPRM5KZ/L3A4EJ yxBDRMi8egsQ== X-IronPort-AV: E=McAfee;i="6200,9189,9989"; a="262389525" X-IronPort-AV: E=Sophos;i="5.82,313,1613462400"; d="scan'208";a="262389525" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 May 2021 00:33:27 -0700 IronPort-SDR: A2G2dULyX5u+hXjc1IsDi3pnqyRWa4YlPM1CfvPxBQNBuvpEpvJ+sDA7tnu/EP8a5WwR89GNw+ cZ3sgBD7Kr0A== X-IronPort-AV: E=Sophos;i="5.82,313,1613462400"; d="scan'208";a="473880275" Received: from yhuang6-desk1.sh.intel.com ([10.239.13.1]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 May 2021 00:33:20 -0700 From: Huang Ying To: Andrew Morton Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Huang Ying , Daniel Jordan , Dan Carpenter , Andrea Parri , Peter Zijlstra , Andi Kleen , Dave Hansen , Omar Sandoval , Paul McKenney , Tejun Heo , Will Deacon , Miaohe Lin Subject: [PATCH -V2] mm, swap: Remove unnecessary smp_rmb() in swap_type_to_swap_info() Date: Thu, 20 May 2021 15:33:01 +0800 Message-Id: <20210520073301.1676294-1-ying.huang@intel.com> X-Mailer: git-send-email 2.30.2 MIME-Version: 1.0 X-Rspamd-Queue-Id: 9F04CE000129 Authentication-Results: imf13.hostedemail.com; dkim=none; spf=none (imf13.hostedemail.com: domain of ying.huang@intel.com has no SPF policy when checking 134.134.136.31) smtp.mailfrom=ying.huang@intel.com; dmarc=fail reason="No valid SPF, No valid DKIM" header.from=intel.com (policy=none) X-Rspamd-Server: rspam03 X-Stat-Signature: inzbopxoim8zyogfzo3jb95myxwzwfu4 X-HE-Tag: 1621496008-63102 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: 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 swap_info[type] is read before "nr_swapfiles". And that may result in NULL pointer dereference. So after 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 */; /* dereference p */ Where the value of swap_info[type] (that is, "p") is checked to be non-zero before being dereferenced. So, the NULL deferencing becomes impossible even if "nr_swapfiles" is read after swap_info[type]. Therefore, the "smp_rmb()" becomes unnecessary. And, we don't even need to read "nr_swapfiles" here. Because the non-zero checking for "p" is sufficient. We just need to make sure we will not access out of the boundary of the array. With the 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. We still need to guarantee swap_info[type] is read before being dereferenced. That can be satisfied via the data dependency ordering enforced by READ_ONCE(swap_info[type]). This needs to be paired with proper write barriers. So smp_store_release() is used in alloc_swap_info() to guarantee the fields of *swap_info[type] is initialized before swap_info[type] itself being written. Note that the fields of *swap_info[type] is initialized to be 0 via kvzalloc() firstly. The assignment and deferencing of swap_info[type] is like rcu_assign_pointer() and rcu_dereference(). 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 v2: - Revise the patch description and comments per Peter's comments. --- mm/swapfile.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/mm/swapfile.c b/mm/swapfile.c index 2aad85751991..65dd979a0f94 100644 --- a/mm/swapfile.c +++ b/mm/swapfile.c @@ -100,11 +100,10 @@ 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. */ - return READ_ONCE(swap_info[type]); + return READ_ONCE(swap_info[type]); /* rcu_dereference() */ } static inline unsigned char swap_count(unsigned char ent) @@ -2884,14 +2883,12 @@ 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.) + * Publish the swap_info_struct after initializing it. + * Note that kvzalloc() above zeroes all its fields. */ - smp_wmb(); - WRITE_ONCE(nr_swapfiles, nr_swapfiles + 1); + smp_store_release(&swap_info[type], p); /* rcu_assign_pointer() */ + nr_swapfiles++; } else { defer = p; p = swap_info[type];