mm, swap: Potential NULL dereference in get_swap_page_of_type()

Smatch complains that the NULL checks on "si" aren't consistent. This seems like a real bug because we have not ensured that the type is valid and so "si" can be NULL.

Fixes ec8acf20afb8 ("swap: add per-partition lock for swapfile")

Signed-off-by Dan Carpenter <[email protected]>

---
 mm/swapfile.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

mm/swapfile.c +0 -0

index f0edf7244256..21e92c757205 100644

mm/swapfile.c +2 -1

 	si = swap_info[type];
 	spin_lock(&si->lock);
-	if (si && (si->flags & SWP_WRITEOK)) {
+	if (si->flags & SWP_WRITEOK) {
 		atomic_long_dec(&nr_swap_pages);
 		/* This is called for allocating swap entry, not cache */
 		offset = scan_swap_map(si, 1);

@@ -1061,6 +1064,7 @@ swp_entry_t get_swap_page_of_type(int type)
 		atomic_long_inc(&nr_swap_pages);
 	}
 	spin_unlock(&si->lock);
+fail:
 	return (swp_entry_t) {0};
 }
 

Signature · 1 line

2.17.1

Quoted 15 lines · Dan Carpenter

Smatch complains that the NULL checks on "si" aren't consistent. This seems like a real bug because we have not ensured that the type is valid and so "si" can be NULL.

Fixes ec8acf20afb8 ("swap: add per-partition lock for swapfile")

Signed-off-by Dan Carpenter <[email protected]>

---
 mm/swapfile.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

mm/swapfile.c +0 -0

index f0edf7244256..21e92c757205 100644

mm/swapfile.c swp_entry_t get_swap_page_of_type(int type) · line 1053

Quoted 6 lines

mm/swapfile.c +3 -0

@@ -1048,9 +1048,12 @@ swp_entry_t get_swap_page_of_type(int type)
 	struct swap_info_struct *si;
 	pgoff_t offset;
 
+	if (type >= nr_swapfiles)
+		goto fail;
+

As long as we're worrying about NULL, I think there should be an smp_rmb here to ensure swap_info[type] isn't NULL in case of an (admittedly unlikely) racing swapon that increments nr_swapfiles. See smp_wmb in alloc_swap_info and the matching smp_rmb's in the file. And READ_ONCE's on either side of the barrier per LKMM.

I'm adding Andrea (randomly selected from the many LKMM folks to avoid spamming all) who can correct me if I'm wrong about any of this.

Quoted 19 lines

mm/swapfile.c +2 -1

 	si = swap_info[type];
 	spin_lock(&si->lock);
-	if (si && (si->flags & SWP_WRITEOK)) {
+	if (si->flags & SWP_WRITEOK) {
 		atomic_long_dec(&nr_swap_pages);
 		/* This is called for allocating swap entry, not cache */
 		offset = scan_swap_map(si, 1);

@@ -1061,6 +1064,7 @@ swp_entry_t get_swap_page_of_type(int type)
 		atomic_long_inc(&nr_swap_pages);
 	}
 	spin_unlock(&si->lock);
+fail:
 	return (swp_entry_t) {0};
 }
 

Signature · 1 line

2.17.1

Quoted 18 lines · Dan Carpenter

Smatch complains that the NULL checks on "si" aren't consistent. This seems like a real bug because we have not ensured that the type is valid and so "si" can be NULL.

Fixes ec8acf20afb8 ("swap: add per-partition lock for swapfile")

Signed-off-by Dan Carpenter <[email protected]>

---
 mm/swapfile.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

mm/swapfile.c +0 -0

index f0edf7244256..21e92c757205 100644
@@ -1048,9 +1048,12 @@ swp_entry_t get_swap_page_of_type(int type)
 	struct swap_info_struct *si;
 	pgoff_t offset;
 

mm/swapfile.c swp_entry_t get_swap_page_of_type(int type) · line 1053

Quoted 3 lines

mm/swapfile.c +3 -0

+	if (type >= nr_swapfiles)
+		goto fail;
+

As long as we're worrying about NULL, I think there should be an smp_rmb here to ensure swap_info[type] isn't NULL in case of an (admittedly unlikely) racing swapon that increments nr_swapfiles. See smp_wmb in alloc_swap_info and the matching smp_rmb's in the file. And READ_ONCE's on either side of the barrier per LKMM.

I'm adding Andrea (randomly selected from the many LKMM folks to avoid spamming all) who can correct me if I'm wrong about any of this.

Quoted 18 lines

mm/swapfile.c +2 -1

 	si = swap_info[type];
 	spin_lock(&si->lock);
-	if (si && (si->flags & SWP_WRITEOK)) {
+	if (si->flags & SWP_WRITEOK) {
 		atomic_long_dec(&nr_swap_pages);
 		/* This is called for allocating swap entry, not cache */
 		offset = scan_swap_map(si, 1);

@@ -1061,6 +1064,7 @@ swp_entry_t get_swap_page_of_type(int type)
 		atomic_long_inc(&nr_swap_pages);
 	}
 	spin_unlock(&si->lock);
+fail:
 	return (swp_entry_t) {0};
 }
 

Signature · 1 line

2.17.1

Quoted 19 lines · Daniel Jordan

mm/swapfile.c swp_entry_t get_swap_page_of_type(int type) · line 1053

Quoted 11 lines · Dan Carpenter

mm/swapfile.c +3 -0

index f0edf7244256..21e92c757205 100644
@@ -1048,9 +1048,12 @@ swp_entry_t get_swap_page_of_type(int type)
 	struct swap_info_struct *si;
 	pgoff_t offset;
 
+	if (type >= nr_swapfiles)
+		goto fail;
+

As long as we're worrying about NULL, I think there should be an smp_rmb here to ensure swap_info[type] isn't NULL in case of an (admittedly unlikely) racing swapon that increments nr_swapfiles. See smp_wmb in alloc_swap_info and the matching smp_rmb's in the file. And READ_ONCE's on either side of the barrier per LKMM.

Quoted 2 lines

I'm adding Andrea (randomly selected from the many LKMM folks to avoid spamming all) who can correct me if I'm wrong about any of this.

This is to confirm that your analysis seems correct to me: the barriers should guarantee that get_swap_page_of_type() will observe the store to swap_info[type] performed by alloc_swap_info() (or a "co"-later store), provided get_swap_page_of_type() observes the increment of nr_swapfiles performed by the (same instance of) alloc_swap_info().

One clarification about the READ_ONCE() matter: the LKMM cannot handle plain or unmarked (shared memory) accesses in their generality at the moment (patches providing support for these accesses are in the making, but they will take some time); IAC, I'm confident to anticipate that, for the particular pattern in question (aka, MP), marking the accesses to nr_swapfiles will be "LKMM-sane" (one way to achieve this would be to convert nr_swapfiles to an atomic_t type...).

I take the liberty of adding other LKMM folks (so that they can blame me for "the spam"! ;-) ): I've learnt from experience that four or more eyes are better than two when it comes to discuss these matters... ;-)

Quoted 1 line

Quoted 1 line

mm/swapfile.c +0 -0

 	si = swap_info[type];

I'm really terribly ignorant when it comes to things like this... To me it looked like the barrier in alloc_swap_info() was enough but when so many smarter people disagree then I must be wrong. I'd like to help, but I sort of feel unqualified.

Could someone else take care of it?

Quoted 20 lines · Andrea Parri

Quoted 19 lines · Daniel Jordan

mm/swapfile.c swp_entry_t get_swap_page_of_type(int type) · line 1053

Quoted 11 lines · Dan Carpenter

mm/swapfile.c +3 -0

index f0edf7244256..21e92c757205 100644
@@ -1048,9 +1048,12 @@ swp_entry_t get_swap_page_of_type(int type)
 	struct swap_info_struct *si;
 	pgoff_t offset;
 
+	if (type >= nr_swapfiles)
+		goto fail;
+

As long as we're worrying about NULL, I think there should be an smp_rmb here to ensure swap_info[type] isn't NULL in case of an (admittedly unlikely) racing swapon that increments nr_swapfiles. See smp_wmb in alloc_swap_info and the matching smp_rmb's in the file. And READ_ONCE's on either side of the barrier per LKMM.

Quoted 8 lines

Quoted 2 lines

I'm adding Andrea (randomly selected from the many LKMM folks to avoid spamming all) who can correct me if I'm wrong about any of this.

This is to confirm that your analysis seems correct to me: the barriers should guarantee that get_swap_page_of_type() will observe the store to swap_info[type] performed by alloc_swap_info() (or a "co"-later store), provided get_swap_page_of_type() observes the increment of nr_swapfiles performed by the (same instance of) alloc_swap_info().

That's good to hear, thanks for looking into it.

Quoted 7 lines

One clarification about the READ_ONCE() matter: the LKMM cannot handle plain or unmarked (shared memory) accesses in their generality at the moment (patches providing support for these accesses are in the making, but they will take some time); IAC, I'm confident to anticipate that, for the particular pattern in question (aka, MP), marking the accesses to nr_swapfiles will be "LKMM-sane" (one way to achieve this would be to convert nr_swapfiles to an atomic_t type...).

I guess you mean we could either use READ_ONCE or make nr_swapfiles atomic, they're different ways of achieving the same thing.

Quoted 6 lines

swap_info[type] performed by alloc_swap_info() (or a "co"-later store),

I take the liberty of adding other LKMM folks (so that they can blame me for "the spam"! ;-) ): I've learnt from experience that four or more eyes are better than two when it comes to discuss these matters... ;-)

Ok, it's fine with me as long as they blame you :)

Quoted 1 line

Quoted 1 line

Quoted 1 line

mm/swapfile.c +0 -0

 	si = swap_info[type];

Dan Carpenter reports a potential NULL dereference in get_swap_page_of_type:

Smatch complains that the NULL checks on "si" aren't consistent. This seems like a real bug because we have not ensured that the type is valid and so "si" can be NULL.

Add the missing check for NULL, taking care to use a read barrier to ensure CPU1 observes CPU0's updates in the correct order:

CPU0 CPU1
alloc_swap_info() if (type >= nr_swapfiles)
swap_info[type] = p /* handle invalid entry */
smp_wmb() smp_rmb()
++nr_swapfiles p = swap_info[type]

Without smp_rmb, CPU1 might observe CPU0's write to nr_swapfiles before CPU0's write to swap_info[type] and read NULL from swap_info[type].

Ying Huang noticed that other places don't order these reads properly. Introduce swap_type_to_swap_info to encourage correct usage.

Use READ_ONCE and WRITE_ONCE to follow the Linux Kernel Memory Model (see tools/memory-model/Documentation/explanation.txt).

This ordering need not be enforced in places where swap_lock is held (e.g. si_swapinfo) because swap_lock serializes updates to nr_swapfiles and the swap_info array.

This is a theoretical problem, no actual reports of it exist.

Fixes ec8acf20afb8 ("swap: add per-partition lock for swapfile")

Reported-by Dan Carpenter <[email protected]>

Signed-off-by Daniel Jordan <[email protected]>

Cc Alan Stern <[email protected]>

Cc Andi Kleen <[email protected]>

Cc Andrea Parri <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Dan Carpenter <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Huang Ying <[email protected]>

Cc Omar Sandoval <[email protected]>

Cc Paul McKenney <[email protected]>

Cc Peter Zijlstra (Intel) <[email protected]>

Cc Shaohua Li <[email protected]>

Cc Stephen Rothwell <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Will Deacon <[email protected]>

---

I'd appreciate it if someone more familiar with memory barriers could check this over. Thanks.

Probably no need for stable, this is all theoretical.

Against linux-mmotm tag v5.0-rc1-mmotm-2019-01-09-13-40

 mm/swapfile.c | 43 +++++++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 16 deletions(-)

mm/swapfile.c +27 -16

index f0edf7244256..dad52fc67045 100644
@@ -99,6 +99,15 @@ static atomic_t proc_poll_event = ATOMIC_INIT(0);
 
 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))
+		return NULL;
+
+	smp_rmb();	/* Pairs with smp_wmb in alloc_swap_info. */
+	return READ_ONCE(swap_info[type]);
+}
+
 static inline unsigned char swap_count(unsigned char ent)
 {
 	return ent & ~SWAP_HAS_CACHE;	/* may include COUNT_CONTINUED flag */

@@ -1045,12 +1054,14 @@ int get_swap_pages(int n_goal, swp_entry_t swp_entries[], int entry_size)
 /* The only caller of this function is now suspend routine */
 swp_entry_t get_swap_page_of_type(int type)
 {
-	struct swap_info_struct *si;
+	struct swap_info_struct *si = swap_type_to_swap_info(type);
 	pgoff_t offset;
 
-	si = swap_info[type];
+	if (!si)
+		goto fail;
+
 	spin_lock(&si->lock);
-	if (si && (si->flags & SWP_WRITEOK)) {
+	if (si->flags & SWP_WRITEOK) {
 		atomic_long_dec(&nr_swap_pages);
 		/* This is called for allocating swap entry, not cache */
 		offset = scan_swap_map(si, 1);

@@ -1061,6 +1072,7 @@ swp_entry_t get_swap_page_of_type(int type)
 		atomic_long_inc(&nr_swap_pages);
 	}
 	spin_unlock(&si->lock);
+fail:
 	return (swp_entry_t) {0};
 }
 

@@ -1072,9 +1084,9 @@ static struct swap_info_struct *__swap_info_get(swp_entry_t entry)
 	if (!entry.val)
 		goto out;
 	type = swp_type(entry);
-	if (type >= nr_swapfiles)
+	p = swap_type_to_swap_info(type);
+	if (!p)
 		goto bad_nofile;
-	p = swap_info[type];
 	if (!(p->flags & SWP_USED))
 		goto bad_device;
 	offset = swp_offset(entry);

@@ -1212,9 +1224,9 @@ struct swap_info_struct *get_swap_device(swp_entry_t entry)
 	if (!entry.val)
 		goto out;
 	type = swp_type(entry);
-	if (type >= nr_swapfiles)
+	si = swap_type_to_swap_info(type);
+	if (!si)
 		goto bad_nofile;
-	si = swap_info[type];
 
 	preempt_disable();
 	if (!(si->flags & SWP_VALID))

@@ -1765,10 +1777,9 @@ int swap_type_of(dev_t device, sector_t offset, struct block_device **bdev_p)
 sector_t swapdev_block(int type, pgoff_t offset)
 {
 	struct block_device *bdev;
+	struct swap_info_struct *si = swap_type_to_swap_info(type);
 
-	if ((unsigned int)type >= nr_swapfiles)
-		return 0;
-	if (!(swap_info[type]->flags & SWP_WRITEOK))
+	if (!si || !(si->flags & SWP_WRITEOK))
 		return 0;
 	return map_swap_entry(swp_entry(type, offset), &bdev);
 }

@@ -2799,9 +2810,9 @@ static void *swap_start(struct seq_file *swap, loff_t *pos)
 	if (!l)
 		return SEQ_START_TOKEN;
 
-	for (type = 0; type < nr_swapfiles; type++) {
+	for (type = 0; type < READ_ONCE(nr_swapfiles); type++) {
 		smp_rmb();	/* read nr_swapfiles before swap_info[type] */
-		si = swap_info[type];
+		si = READ_ONCE(swap_info[type]);
 		if (!(si->flags & SWP_USED) || !si->swap_map)
 			continue;
 		if (!--l)

@@ -2821,9 +2832,9 @@ static void *swap_next(struct seq_file *swap, void *v, loff_t *pos)
 	else
 		type = si->type + 1;
 
-	for (; type < nr_swapfiles; type++) {
+	for (; type < READ_ONCE(nr_swapfiles); type++) {
 		smp_rmb();	/* read nr_swapfiles before swap_info[type] */
-		si = swap_info[type];
+		si = READ_ONCE(swap_info[type]);
 		if (!(si->flags & SWP_USED) || !si->swap_map)
 			continue;
 		++*pos;

@@ -2930,14 +2941,14 @@ static struct swap_info_struct *alloc_swap_info(void)
 	}
 	if (type >= nr_swapfiles) {
 		p->type = type;
-		swap_info[type] = p;
+		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.)
 		 */
 		smp_wmb();
-		nr_swapfiles++;
+		WRITE_ONCE(nr_swapfiles, nr_swapfiles + 1);
 	} else {
 		kvfree(p);
 		p = swap_info[type];

Signature · 1 line

2.20.0

Dan Carpenter reports a potential NULL dereference in get_swap_page_of_type:

Smatch complains that the NULL checks on "si" aren't consistent. This seems like a real bug because we have not ensured that the type is valid and so "si" can be NULL.

Add the missing check for NULL, taking care to use a read barrier to ensure CPU1 observes CPU0's updates in the correct order:

CPU0 CPU1
alloc_swap_info() if (type >= nr_swapfiles)
swap_info[type] = p /* handle invalid entry */
smp_wmb() smp_rmb()
++nr_swapfiles p = swap_info[type]

Without smp_rmb, CPU1 might observe CPU0's write to nr_swapfiles before CPU0's write to swap_info[type] and read NULL from swap_info[type].

Ying Huang noticed that other places don't order these reads properly. Introduce swap_type_to_swap_info to encourage correct usage.

Use READ_ONCE and WRITE_ONCE to follow the Linux Kernel Memory Model (see tools/memory-model/Documentation/explanation.txt).

This ordering need not be enforced in places where swap_lock is held (e.g. si_swapinfo) because swap_lock serializes updates to nr_swapfiles and the swap_info array.

This is a theoretical problem, no actual reports of it exist.

Dan Carpenter reports a potential NULL dereference in get_swap_page_of_type:

Smatch complains that the NULL checks on "si" aren't consistent. This seems like a real bug because we have not ensured that the type is valid and so "si" can be NULL.

Add the missing check for NULL, taking care to use a read barrier to ensure CPU1 observes CPU0's updates in the correct order:

CPU0 CPU1
alloc_swap_info() if (type >= nr_swapfiles)
swap_info[type] = p /* handle invalid entry */
smp_wmb() smp_rmb()
++nr_swapfiles p = swap_info[type]

Without smp_rmb, CPU1 might observe CPU0's write to nr_swapfiles before CPU0's write to swap_info[type] and read NULL from swap_info[type].

Ying Huang noticed that other places don't order these reads properly. Introduce swap_type_to_swap_info to encourage correct usage.

Use READ_ONCE and WRITE_ONCE to follow the Linux Kernel Memory Model (see tools/memory-model/Documentation/explanation.txt).

This ordering need not be enforced in places where swap_lock is held (e.g. si_swapinfo) because swap_lock serializes updates to nr_swapfiles and the swap_info array.

This is a theoretical problem, no actual reports of it exist.

Fixes ec8acf20afb8 ("swap: add per-partition lock for swapfile")

Reported-by Dan Carpenter <[email protected]>

Signed-off-by Daniel Jordan <[email protected]>

Cc Alan Stern <[email protected]>

Cc Andi Kleen <[email protected]>

Cc Andrea Parri <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Dan Carpenter <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Huang Ying <[email protected]>

Cc Omar Sandoval <[email protected]>

Cc Paul McKenney <[email protected]>

Cc Peter Zijlstra (Intel) <[email protected]>

Cc Shaohua Li <[email protected]>

Cc Stephen Rothwell <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Will Deacon <[email protected]>

---

I'd appreciate it if someone more familiar with memory barriers could check this over. Thanks.

Probably no need for stable, this is all theoretical.

Dan Carpenter reports a potential NULL dereference in get_swap_page_of_type:

Smatch complains that the NULL checks on "si" aren't consistent. This seems like a real bug because we have not ensured that the type is valid and so "si" can be NULL.

Add the missing check for NULL, taking care to use a read barrier to ensure CPU1 observes CPU0's updates in the correct order:

CPU0 CPU1
alloc_swap_info() if (type >= nr_swapfiles)
swap_info[type] = p /* handle invalid entry */
smp_wmb() smp_rmb()
++nr_swapfiles p = swap_info[type]

Without smp_rmb, CPU1 might observe CPU0's write to nr_swapfiles before CPU0's write to swap_info[type] and read NULL from swap_info[type].

Ying Huang noticed that other places don't order these reads properly. Introduce swap_type_to_swap_info to encourage correct usage.

Use READ_ONCE and WRITE_ONCE to follow the Linux Kernel Memory Model (see tools/memory-model/Documentation/explanation.txt).

This ordering need not be enforced in places where swap_lock is held (e.g. si_swapinfo) because swap_lock serializes updates to nr_swapfiles and the swap_info array.

This is a theoretical problem, no actual reports of it exist.

Fixes ec8acf20afb8 ("swap: add per-partition lock for swapfile")

Reported-by Dan Carpenter <[email protected]>

Signed-off-by Daniel Jordan <[email protected]>

mm/swapfile.c +10 -2

+static struct swap_info_struct *swap_type_to_swap_info(int type)
+{
+	if (type >= READ_ONCE(nr_swapfiles))
+		return NULL;
+
+	smp_rmb();	/* Pairs with smp_wmb in alloc_swap_info. */
+	return READ_ONCE(swap_info[type]);
+}

@@ -2799,9 +2810,9 @@ static void *swap_start(struct seq_file *swap, loff_t *pos)
 	if (!l)
 		return SEQ_START_TOKEN;
 
-	for (type = 0; type < nr_swapfiles; type++) {
+	for (type = 0; type < READ_ONCE(nr_swapfiles); type++) {
 		smp_rmb();	/* read nr_swapfiles before swap_info[type] */
-		si = swap_info[type];
+		si = READ_ONCE(swap_info[type]);
 		if (!(si->flags & SWP_USED) || !si->swap_map)
 			continue;
 		if (!--l)

@@ -2821,9 +2832,9 @@ static void *swap_next(struct seq_file *swap, void *v, loff_t *pos)
 	else

mm/swapfile.c +1 -1

@@ -2930,14 +2941,14 @@ static struct swap_info_struct *alloc_swap_info(void)
 	}
 	if (type >= nr_swapfiles) {
 		p->type = type;
-		swap_info[type] = p;
+		WRITE_ONCE(swap_info[type], p);
 		/*

LGTM, but like most people I'm afraid to ack it ;)

mm-swap-fix-race-between-swapoff-and-some-swap-operations.patch is very stuck so can you please redo this against mainline?

Quoted 1 line · Daniel Jordan

Probably no need for stable, this is all theoretical.

The NULL dereference part is not theoretical. It require CAP_SYS_ADMIN so it's not a huge deal, but you could trigger it from snapshot_ioctl() with SNAPSHOT_ALLOC_SWAP_PAGE.

On Mon, Jan 14, 2019 at 07:23:05PM -0500, Daniel Jordan wrote:

A few comments below, but:

Acked-by Peter Zijlstra (Intel) <[email protected]>

Quoted 14 lines

mm/swapfile.c +2 -2

@@ -2799,9 +2810,9 @@ static void *swap_start(struct seq_file *swap, loff_t *pos)
 	if (!l)
 		return SEQ_START_TOKEN;
 
-	for (type = 0; type < nr_swapfiles; type++) {
+	for (type = 0; type < READ_ONCE(nr_swapfiles); type++) {
 		smp_rmb();	/* read nr_swapfiles before swap_info[type] */
-		si = swap_info[type];
+		si = READ_ONCE(swap_info[type]);
 		if (!(si->flags & SWP_USED) || !si->swap_map)
 			continue;
 		if (!--l)

@@ -2821,9 +2832,9 @@ static void *swap_next(struct seq_file *swap, void *v, loff_t *pos)
 	else

mm/swapfile.c static void *swap_next(struct seq_file *swap, void *v, loff_t *pos) · line 2840

Quoted 10 lines

mm/swapfile.c +2 -2

 		type = si->type + 1;
 
-	for (; type < nr_swapfiles; type++) {
+	for (; type < READ_ONCE(nr_swapfiles); type++) {
 		smp_rmb();	/* read nr_swapfiles before swap_info[type] */
-		si = swap_info[type];
+		si = READ_ONCE(swap_info[type]);
 		if (!(si->flags & SWP_USED) || !si->swap_map)
 			continue;
 		++*pos;

You could write those like:

	for (; (si = swap_type_to_swap_info(type)); type++)

Quoted 7 lines

mm/swapfile.c +1 -1

@@ -2930,14 +2941,14 @@ static struct swap_info_struct *alloc_swap_info(void)
 	}
 	if (type >= nr_swapfiles) {
 		p->type = type;
-		swap_info[type] = p;
+		WRITE_ONCE(swap_info[type], p);
 		/*

mm/swapfile.c static struct swap_info_struct *alloc_swap_info(void) · line 2954

Quoted 10 lines

mm/swapfile.c +1 -1

 		 * 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.)
 		 */
 		smp_wmb();
-		nr_swapfiles++;
+		WRITE_ONCE(nr_swapfiles, nr_swapfiles + 1);
 	} else {
 		kvfree(p);
 		p = swap_info[type];

It is also possible to write this with smp_load_acquire() / smp_store_release(). ARM64/RISC-V might benefit from that, OTOH ARM won't like that much.

Dunno what would be better.

mm-swap-fix-race-between-swapoff-and-some-swap-operations.patch is very stuck so can you please redo this against mainline?

Quoted 2 lines · Andrew Morton

mm-swap-fix-race-between-swapoff-and-some-swap-operations.patch is very stuck so can you please redo this against mainline?

Allow me to be off topic, this patch has been in mm tree for quite some time, what can I do to help this be merged upstream?

Quoted 6 lines · Ying

Quoted 2 lines · Andrew Morton

mm-swap-fix-race-between-swapoff-and-some-swap-operations.patch is very stuck so can you please redo this against mainline?

Allow me to be off topic, this patch has been in mm tree for quite some time, what can I do to help this be merged upstream?

I have no evidence that it has been reviewed, for a start. I've asked Hugh to look at it.

Quoted 6 lines · Ying

Quoted 2 lines · Andrew Morton

mm-swap-fix-race-between-swapoff-and-some-swap-operations.patch is very stuck so can you please redo this against mainline?

Allow me to be off topic, this patch has been in mm tree for quite some time, what can I do to help this be merged upstream?

I have no evidence that it has been reviewed, for a start. I've asked Hugh to look at it.

Quoted 7 lines · Andrew Morton

Quoted 3 lines · Ying

Quoted 2 lines · Andrew Morton

mm-swap-fix-race-between-swapoff-and-some-swap-operations.patch is very stuck so can you please redo this against mainline?

I have no evidence that it has been reviewed, for a start. I've asked Hugh to look at it.

I tried at the weekend. Usual story: I don't like it at all, the ever-increasing complexity there, but certainly understand the need for that fix, and have not managed to think up anything better - and now I need to switch away, sorry.

The multiple dynamically allocated and freed swapper address spaces have indeed broken what used to make it safe. If those imaginary address spaces did not have to be virtually contiguous, I'd say cache them and reuse them, instead of freeing. But I don't see how to do that as it stands.

find_get_page(swapper_address_space(entry), swp_offset(entry)) has become an unsafe construct, where it used to be safe against corrupted page tables. Maybe we don't care so much about crashing on corrupted page tables nowadays (I haven't heard recent complaints), and I think Huang is correct that lookup_swap_cache() and __read_swap_cache_async() happen to be the only instances that need to be guarded against swapoff (the others are working with page table locked).

The array of arrays of swapper spaces is all just to get a separate lock for separate extents of the swapfile: I wonder whether Matthew has anything in mind for that in XArray (I think Peter once got it working in radix-tree, but the overhead not so good).

Quoted 7 lines · Andrew Morton

Quoted 6 lines · Ying

Quoted 2 lines · Andrew Morton

mm-swap-fix-race-between-swapoff-and-some-swap-operations.patch is very stuck so can you please redo this against mainline?

Allow me to be off topic, this patch has been in mm tree for quite some time, what can I do to help this be merged upstream?

Wow, yes, it's about a year old.

Quoted 3 lines

I have no evidence that it has been reviewed, for a start. I've asked Hugh to look at it.

I tried at the weekend. Usual story: I don't like it at all, the ever-increasing complexity there, but certainly understand the need for that fix, and have not managed to think up anything better - and now I need to switch away, sorry.

The multiple dynamically allocated and freed swapper address spaces have indeed broken what used to make it safe. If those imaginary address spaces did not have to be virtually contiguous, I'd say cache them and reuse them, instead of freeing. But I don't see how to do that as it stands.

find_get_page(swapper_address_space(entry), swp_offset(entry)) has become an unsafe construct, where it used to be safe against corrupted page tables. Maybe we don't care so much about crashing on corrupted page tables nowadays (I haven't heard recent complaints), and I think Huang is correct that lookup_swap_cache() and __read_swap_cache_async() happen to be the only instances that need to be guarded against swapoff (the others are working with page table locked).

The array of arrays of swapper spaces is all just to get a separate lock for separate extents of the swapfile: I wonder whether Matthew has anything in mind for that in XArray (I think Peter once got it working in radix-tree, but the overhead not so good).

(I was originally horrified by the stop_machine() added in swapon and swapoff, but perhaps I'm remembering a distant past of really stopping the machine: stop_machine() today looked reasonable, something to avoid generally like lru_add_drain_all(), but not as shameful as I thought.)

Thanks a lot for your review and comments!

It appears that you have no strong objection for this patch?

Could I have your "Acked-by"?

Quoted 4 lines · Ying

Thanks a lot for your review and comments!

It appears that you have no strong objection for this patch?

That much is correct.

Quoted 1 line

Could I have your "Acked-by"?

Sorry to be so begrudging, but I have to save my Acks for when I feel more confident in my opinion. Here I don't think I can get beyond

Not-Nacked-by Hugh Dickins <[email protected]>

I imagine Daniel would ask for some barriers in there: maybe you can get a more generous response from him when he looks over the result.

Warmly but meanly,
Hugh

Quoted 7 lines · Andrew Morton

Quoted 6 lines · Ying

Quoted 2 lines · Andrew Morton

mm-swap-fix-race-between-swapoff-and-some-swap-operations.patch is very stuck so can you please redo this against mainline?

Allow me to be off topic, this patch has been in mm tree for quite some time, what can I do to help this be merged upstream?

Wow, yes, it's about a year old.

Quoted 3 lines

I have no evidence that it has been reviewed, for a start. I've asked Hugh to look at it.

I tried at the weekend. Usual story: I don't like it at all, the ever-increasing complexity there, but certainly understand the need for that fix, and have not managed to think up anything better - and now I need to switch away, sorry.

(I was originally horrified by the stop_machine() added in swapon and swapoff, but perhaps I'm remembering a distant past of really stopping the machine: stop_machine() today looked reasonable, something to avoid generally like lru_add_drain_all(), but not as shameful as I thought.)

Hugh

Hi Huang, Ying,

Quoted 8 lines · Hugh Dickins

Quoted 7 lines · Andrew Morton

Quoted 6 lines · Ying

Quoted 2 lines · Andrew Morton

mm-swap-fix-race-between-swapoff-and-some-swap-operations.patch is very stuck so can you please redo this against mainline?

Allow me to be off topic, this patch has been in mm tree for quite some time, what can I do to help this be merged upstream?

[...]

Quoted 11 lines

Wow, yes, it's about a year old.

Quoted 3 lines

I have no evidence that it has been reviewed, for a start. I've asked Hugh to look at it.

I tried at the weekend. Usual story: I don't like it at all, the ever-increasing complexity there, but certainly understand the need for that fix, and have not managed to think up anything better - and now I need to switch away, sorry.

FWIW, I do agree with Hugh about "the need for that fix": AFAIU, that (mainline) code is naively buggy _and_ "this patch":

http://lkml.kernel.org/r/[email protected]

"redone on top of mainline" seems both correct and appropriate to me.

Quoted 5 lines

(I was originally horrified by the stop_machine() added in swapon and swapoff, but perhaps I'm remembering a distant past of really stopping the machine: stop_machine() today looked reasonable, something to avoid generally like lru_add_drain_all(), but not as shameful as I thought.)

AFAIC_find_on_LKML, we have three different fixes (at least!): resp.,

1. refcount(-based),
2. RCU,
3. stop_machine();

(3) appears to be the less documented/relied-upon/tested among these; I'm not aware of definitive reasons forcing us to reject (1) and (2).

Andrea

Quoted 2 lines

Hugh