Skip to contentSkip to conversation

FAILED: patch "[PATCH] dax: Don't access a freed inode" failed to apply to 4.19-stable tree

PATCH [email protected] 4 participants 6 messages Dec 11, 2018 – Dec 22, 2018

Subsystems (4)
lore
Share & tools
lore

[email protected]

author maintainer
#0 lore raw

The patch below does not apply to the 4.19-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

Quoted 1 line

From 55e56f06ed71d9441f3abd5b1d3c1a870812b3fe Mon Sep 17 00:00:00 2001

From: Matthew Wilcox <[email protected]>
Date: Tue, 27 Nov 2018 13:16:34 -0800
Subject: [PATCH] dax: Don't access a freed inode

After we drop the i_pages lock, the inode can be freed at any time. The get_unlocked_entry() code has no choice but to reacquire the lock, so it can't be used here. Create a new wait_entry_unlocked() which takes care not to acquire the lock or dereference the address_space in any way.

Fixes c2a7d2a11552 ("filesystem-dax: Introduce dax_lock_mapping_entry()")
fs/dax.c +29 -3
index e69fc231833b..3f592dc18d67 100644
@@ -232,6 +232,34 @@ static void *get_unlocked_entry(struct xa_state *xas)
 	}
 }
 
+/*
+ * The only thing keeping the address space around is the i_pages lock
+ * (it's cycled in clear_inode() after removing the entries from i_pages)
+ * After we call xas_unlock_irq(), we cannot touch xas->xa.
+ */
+static void wait_entry_unlocked(struct xa_state *xas, void *entry)
+{
+	struct wait_exceptional_entry_queue ewait;
+	wait_queue_head_t *wq;
+
+	init_wait(&ewait.wait);
+	ewait.wait.func = wake_exceptional_entry_func;
+
+	wq = dax_entry_waitqueue(xas, entry, &ewait.key);
+	prepare_to_wait_exclusive(wq, &ewait.wait, TASK_UNINTERRUPTIBLE);
+	xas_unlock_irq(xas);
+	schedule();
+	finish_wait(wq, &ewait.wait);
+
+	/*
+	 * Entry lock waits are exclusive. Wake up the next waiter since
+	 * we aren't sure we will acquire the entry lock and thus wake
+	 * the next waiter up on unlock.
+	 */
+	if (waitqueue_active(wq))
+		__wake_up(wq, TASK_NORMAL, 1, &ewait.key);
+}
+
 static void put_unlocked_entry(struct xa_state *xas, void *entry)
 {
 	/* If we were the only waiter woken, wake the next one */
@@ -389,9 +417,7 @@ bool dax_lock_mapping_entry(struct page *page)
 		entry = xas_load(&xas);
 		if (dax_is_locked(entry)) {
 			rcu_read_unlock();
-			entry = get_unlocked_entry(&xas);
-			xas_unlock_irq(&xas);
-			put_unlocked_entry(&xas, entry);
+			wait_entry_unlocked(&xas, entry);
 			rcu_read_lock();
 			continue;
 		}
reviewer Reviewed-by Jan Kara
#1 lore raw
Quoted 5 lines · On Tue, Dec 11, 2018 at 03:00:09PM +0100, [email protected] wrote:

The patch below does not apply to the 4.19-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

I have only compile-tested this backport. Dan, do you want to run it through some tests before Greg applies it?

Quoted 1 line

From e01d37913b5577acaa2e8e35200081eae2795087 Mon Sep 17 00:00:00 2001

From: Matthew Wilcox <[email protected]>
Date: Tue, 11 Dec 2018 10:16:45 -0500
Subject: [PATCH 2/2] dax: Don't access a freed inode

commit 55e56f06ed71d9441f3abd5b1d3c1a870812b3fe upstream.

After we drop the i_pages lock, the inode can be freed at any time. The get_unlocked_entry() code has no choice but to reacquire the lock, so it can't be used here. Create a new wait_entry_unlocked() which takes care not to acquire the lock or dereference the address_space in any way.

Fixes c2a7d2a11552 ("filesystem-dax: Introduce dax_lock_mapping_entry()")
---
 fs/dax.c | 69 ++++++++++++++++++++++++++------------------------------
 1 file changed, 32 insertions(+), 37 deletions(-)
fs/dax.c +32 -37
index 3a2682a6c832..415605fafaeb 100644
@@ -229,8 +229,8 @@ static void put_unlocked_mapping_entry(struct address_space *mapping,
  *
  * Must be called with the i_pages lock held.
  */
-static void *__get_unlocked_mapping_entry(struct address_space *mapping,
-		pgoff_t index, void ***slotp, bool (*wait_fn)(void))
+static void *get_unlocked_mapping_entry(struct address_space *mapping,
+		pgoff_t index, void ***slotp)
 {
 	void *entry, **slot;
 	struct wait_exceptional_entry_queue ewait;
@@ -240,8 +240,6 @@ static void *__get_unlocked_mapping_entry(struct address_space *mapping,
 	ewait.wait.func = wake_exceptional_entry_func;
 
 	for (;;) {
-		bool revalidate;
-
 		entry = __radix_tree_lookup(&mapping->i_pages, index, NULL,
 					  &slot);
 		if (!entry ||
@@ -256,30 +254,39 @@ static void *__get_unlocked_mapping_entry(struct address_space *mapping,
 		prepare_to_wait_exclusive(wq, &ewait.wait,
 					  TASK_UNINTERRUPTIBLE);
 		xa_unlock_irq(&mapping->i_pages);
-		revalidate = wait_fn();
+		schedule();
 		finish_wait(wq, &ewait.wait);
 		xa_lock_irq(&mapping->i_pages);
-		if (revalidate) {
-			put_unlocked_mapping_entry(mapping, index, entry);
-			return ERR_PTR(-EAGAIN);
-		}
 	}
 }
 
-static bool entry_wait(void)
+/*
+ * The only thing keeping the address space around is the i_pages lock
+ * (it's cycled in clear_inode() after removing the entries from i_pages)
+ * After we call xas_unlock_irq(), we cannot touch xas->xa.
+ */
+static void wait_entry_unlocked(struct address_space *mapping, pgoff_t index,
+		void ***slotp, void *entry)
 {
+	struct wait_exceptional_entry_queue ewait;
+	wait_queue_head_t *wq;
+
+	init_wait(&ewait.wait);
+	ewait.wait.func = wake_exceptional_entry_func;
+
+	wq = dax_entry_waitqueue(mapping, index, entry, &ewait.key);
+	prepare_to_wait_exclusive(wq, &ewait.wait, TASK_UNINTERRUPTIBLE);
+	xa_unlock_irq(&mapping->i_pages);
 	schedule();
+	finish_wait(wq, &ewait.wait);
+
 	/*
-	 * Never return an ERR_PTR() from
-	 * __get_unlocked_mapping_entry(), just keep looping.
+	 * Entry lock waits are exclusive. Wake up the next waiter since
+	 * we aren't sure we will acquire the entry lock and thus wake
+	 * the next waiter up on unlock.
 	 */
-	return false;
-}
-
-static void *get_unlocked_mapping_entry(struct address_space *mapping,
-		pgoff_t index, void ***slotp)
-{
-	return __get_unlocked_mapping_entry(mapping, index, slotp, entry_wait);
+	if (waitqueue_active(wq))
+		__wake_up(wq, TASK_NORMAL, 1, &ewait.key);
 }
 
 static void unlock_mapping_entry(struct address_space *mapping, pgoff_t index)
@@ -398,19 +405,6 @@ static struct page *dax_busy_page(void *entry)
 	return NULL;
 }
 
-static bool entry_wait_revalidate(void)
-{
-	rcu_read_unlock();
-	schedule();
-	rcu_read_lock();
-
-	/*
-	 * Tell __get_unlocked_mapping_entry() to take a break, we need
-	 * to revalidate page->mapping after dropping locks
-	 */
-	return true;
-}
-
 bool dax_lock_mapping_entry(struct page *page)
 {
 	pgoff_t index;
@@ -446,14 +440,15 @@ bool dax_lock_mapping_entry(struct page *page)
 		}
 		index = page->index;
 
-		entry = __get_unlocked_mapping_entry(mapping, index, &slot,
-				entry_wait_revalidate);
+		entry = __radix_tree_lookup(&mapping->i_pages, index,
+						NULL, &slot);
 		if (!entry) {
 			xa_unlock_irq(&mapping->i_pages);
 			break;
-		} else if (IS_ERR(entry)) {
-			xa_unlock_irq(&mapping->i_pages);
-			WARN_ON_ONCE(PTR_ERR(entry) != -EAGAIN);
+		} else if (slot_locked(mapping, slot)) {
+			rcu_read_unlock();
+			wait_entry_unlocked(mapping, index, &slot, entry);
+			rcu_read_lock();
 			continue;
 		}
 		lock_slot(mapping, slot);
Signature · 1 line
2.19.1
author maintainer
#2 lore raw
Quoted 9 lines · Matthew Wilcox
Quoted 5 lines · On Tue, Dec 11, 2018 at 03:00:09PM +0100, [email protected] wrote:

The patch below does not apply to the 4.19-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

I have only compile-tested this backport. Dan, do you want to run it through some tests before Greg applies it?

Thanks for the backport, I'll wait to hear from Dan before queueing this up.

greg k-h
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm

Tested-by
#3 lore raw
Quoted 9 lines · Greg KH
Quoted 7 lines · Matthew Wilcox
Quoted 5 lines · On Tue, Dec 11, 2018 at 03:00:09PM +0100, [email protected] wrote:

The patch below does not apply to the 4.19-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

Quoted 5 lines
Quoted 2 lines

I have only compile-tested this backport. Dan, do you want to run it through some tests before Greg applies it?

Thanks for the backport, I'll wait to hear from Dan before queueing this up.

However, on the upstream version of this fix Linus pointed out an additional fixup. Once that goes upstream [1] I'll circle back to provide a fixed up version of this backport to keep -stable and mainline more aligned.

[1]: https://marc.info/?l=linux-fsdevel&m=154542184202064&w=2
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm

maintainer
#4 lore raw
Quoted 16 lines · Dan Williams
Quoted 9 lines · Greg KH
Quoted 7 lines · Matthew Wilcox
Quoted 5 lines · On Tue, Dec 11, 2018 at 03:00:09PM +0100, [email protected] wrote:

The patch below does not apply to the 4.19-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

Quoted 5 lines
Quoted 2 lines

I have only compile-tested this backport. Dan, do you want to run it through some tests before Greg applies it?

Thanks for the backport, I'll wait to hear from Dan before queueing this up.

Quoted 8 lines

However, on the upstream version of this fix Linus pointed out an additional fixup. Once that goes upstream [1] I'll circle back to provide a fixed up version of this backport to keep -stable and mainline more aligned.

[1]: https://marc.info/?l=linux-fsdevel&m=154542184202064&w=2

Okay, I guess it makes sense to wait for the fixup before we pull this backport in?

--
Thanks,
Sasha
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm

#5 lore raw
Quoted 25 lines · Sasha Levin
Quoted 23 lines · Dan Williams
Quoted 9 lines · Greg KH
Quoted 7 lines · Matthew Wilcox
Quoted 5 lines · On Tue, Dec 11, 2018 at 03:00:09PM +0100, [email protected] wrote:

The patch below does not apply to the 4.19-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

Quoted 5 lines
Quoted 2 lines

I have only compile-tested this backport. Dan, do you want to run it through some tests before Greg applies it?

Thanks for the backport, I'll wait to hear from Dan before queueing this up.

However, on the upstream version of this fix Linus pointed out an additional fixup. Once that goes upstream [1] I'll circle back to provide a fixed up version of this backport to keep -stable and mainline more aligned.

Quoted 4 lines
Quoted 1 line

[1]: https://marc.info/?l=linux-fsdevel&m=154542184202064&w=2

Okay, I guess it makes sense to wait for the fixup before we pull this backport in?

Yes, I'll send a replacement once mainline has that change.