linux-kernel - [PATCH] mm, swap: Remove unnecessary smp_rmb() in swap_type_to_swap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 13 May 2021 14:48:37 +0800
From:   Huang Ying <ying.huang@...el.com>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Huang Ying <ying.huang@...el.com>,
        Daniel Jordan <daniel.m.jordan@...cle.com>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Andrea Parri <andrea.parri@...rulasolutions.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Andi Kleen <ak@...ux.intel.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Omar Sandoval <osandov@...com>,
        Paul McKenney <paulmck@...nel.org>, Tejun Heo <tj@...nel.org>,
        Will Deacon <will.deacon@....com>,
        Miaohe Lin <linmiaohe@...wei.com>
Subject: [PATCH] mm, swap: Remove unnecessary smp_rmb() in swap_type_to_swap_info()

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" <ying.huang@...el.com>
Cc: Daniel Jordan <daniel.m.jordan@...cle.com>
Cc: Dan Carpenter <dan.carpenter@...cle.com>
Cc: Andrea Parri <andrea.parri@...rulasolutions.com>
Cc: Peter Zijlstra (Intel) <peterz@...radead.org>
Cc: Andi Kleen <ak@...ux.intel.com>
Cc: Dave Hansen <dave.hansen@...ux.intel.com>
Cc: Omar Sandoval <osandov@...com>
Cc: Paul McKenney <paulmck@...nel.org>
Cc: Tejun Heo <tj@...nel.org>
Cc: Will Deacon <will.deacon@....com>
Cc: Miaohe Lin <linmiaohe@...wei.com>
---
 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();
-		WRITE_ONCE(nr_swapfiles, nr_swapfiles + 1);
+		WRITE_ONCE(swap_info[type], p);
+		nr_swapfiles++;
 	} else {
 		defer = p;
 		p = swap_info[type];
-- 
2.30.2