Date:   Thu, 20 May 2021 15:33:01 +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 -V2] 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
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" <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>

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];
-- 
2.30.2

Powered by blists - more mailing lists