linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] z3fold: fix retry mechanism in page reclaim
@ 2019-09-08 13:29 Vitaly Wool
  2019-09-08 13:56 ` Maciej S. Szmigiero
  0 siblings, 1 reply; 3+ messages in thread
From: Vitaly Wool @ 2019-09-08 13:29 UTC (permalink / raw)
  To: Linux-MM, Andrew Morton, linux-kernel
  Cc: Agustín DallʼAlba, Dan Streetman, Vlastimil Babka,
	markus.linnala

z3fold_page_reclaim()'s retry mechanism is broken: on a second
iteration it will have zhdr from the first one so that zhdr
is no longer in line with struct page. That leads to crashes when
the system is stressed.

Fix that by moving zhdr assignment up.

While at it, protect against using already freed handles by using
own local slots structure in z3fold_page_reclaim().

Reported-by: Markus Linnala <markus.linnala@gmail.com>
Reported-by: Chris Murphy <bugzilla@colorremedies.com>
Reported-by: Agustin Dall'Alba <agustin@dallalba.com.ar>
Signed-off-by: Vitaly Wool <vitalywool@gmail.com>
---
 mm/z3fold.c | 49 ++++++++++++++++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 15 deletions(-)

diff --git a/mm/z3fold.c b/mm/z3fold.c
index 75b7962439ff..6397725b5ec6 100644
--- a/mm/z3fold.c
+++ b/mm/z3fold.c
@@ -372,9 +372,10 @@ static inline int __idx(struct z3fold_header *zhdr, enum buddy bud)
  * Encodes the handle of a particular buddy within a z3fold page
  * Pool lock should be held as this function accesses first_num
  */
-static unsigned long encode_handle(struct z3fold_header *zhdr, enum buddy bud)
+static unsigned long __encode_handle(struct z3fold_header *zhdr,
+				struct z3fold_buddy_slots *slots,
+				enum buddy bud)
 {
-	struct z3fold_buddy_slots *slots;
 	unsigned long h = (unsigned long)zhdr;
 	int idx = 0;
 
@@ -391,11 +392,15 @@ static unsigned long encode_handle(struct z3fold_header *zhdr, enum buddy bud)
 	if (bud == LAST)
 		h |= (zhdr->last_chunks << BUDDY_SHIFT);
 
-	slots = zhdr->slots;
 	slots->slot[idx] = h;
 	return (unsigned long)&slots->slot[idx];
 }
 
+static unsigned long encode_handle(struct z3fold_header *zhdr, enum buddy bud)
+{
+	return __encode_handle(zhdr, zhdr->slots, bud);
+}
+
 /* Returns the z3fold page where a given handle is stored */
 static inline struct z3fold_header *handle_to_z3fold_header(unsigned long h)
 {
@@ -630,6 +635,7 @@ static void do_compact_page(struct z3fold_header *zhdr, bool locked)
 	}
 
 	if (unlikely(PageIsolated(page) ||
+		     test_bit(PAGE_CLAIMED, &page->private) ||
 		     test_bit(PAGE_STALE, &page->private))) {
 		z3fold_page_unlock(zhdr);
 		return;
@@ -1132,6 +1138,7 @@ static int z3fold_reclaim_page(struct z3fold_pool *pool, unsigned int retries)
 	struct z3fold_header *zhdr = NULL;
 	struct page *page = NULL;
 	struct list_head *pos;
+	struct z3fold_buddy_slots slots;
 	unsigned long first_handle = 0, middle_handle = 0, last_handle = 0;
 
 	spin_lock(&pool->lock);
@@ -1150,16 +1157,22 @@ static int z3fold_reclaim_page(struct z3fold_pool *pool, unsigned int retries)
 			/* this bit could have been set by free, in which case
 			 * we pass over to the next page in the pool.
 			 */
-			if (test_and_set_bit(PAGE_CLAIMED, &page->private))
+			if (test_and_set_bit(PAGE_CLAIMED, &page->private)) {
+				page = NULL;
 				continue;
+			}
 
-			if (unlikely(PageIsolated(page)))
+			if (unlikely(PageIsolated(page))) {
+				clear_bit(PAGE_CLAIMED, &page->private);
+				page = NULL;
 				continue;
+			}
+			zhdr = page_address(page);
 			if (test_bit(PAGE_HEADLESS, &page->private))
 				break;
 
-			zhdr = page_address(page);
 			if (!z3fold_page_trylock(zhdr)) {
+				clear_bit(PAGE_CLAIMED, &page->private);
 				zhdr = NULL;
 				continue; /* can't evict at this point */
 			}
@@ -1177,26 +1190,30 @@ static int z3fold_reclaim_page(struct z3fold_pool *pool, unsigned int retries)
 
 		if (!test_bit(PAGE_HEADLESS, &page->private)) {
 			/*
-			 * We need encode the handles before unlocking, since
-			 * we can race with free that will set
-			 * (first|last)_chunks to 0
+			 * We need encode the handles before unlocking, and
+			 * use our local slots structure because z3fold_free
+			 * can zero out zhdr->slots and we can't do much
+			 * about that
 			 */
 			first_handle = 0;
 			last_handle = 0;
 			middle_handle = 0;
 			if (zhdr->first_chunks)
-				first_handle = encode_handle(zhdr, FIRST);
+				first_handle = __encode_handle(zhdr, &slots,
+								FIRST);
 			if (zhdr->middle_chunks)
-				middle_handle = encode_handle(zhdr, MIDDLE);
+				middle_handle = __encode_handle(zhdr, &slots,
+								MIDDLE);
 			if (zhdr->last_chunks)
-				last_handle = encode_handle(zhdr, LAST);
+				last_handle = __encode_handle(zhdr, &slots,
+								LAST);
 			/*
 			 * it's safe to unlock here because we hold a
 			 * reference to this page
 			 */
 			z3fold_page_unlock(zhdr);
 		} else {
-			first_handle = encode_handle(zhdr, HEADLESS);
+			first_handle = __encode_handle(zhdr, &slots, HEADLESS);
 			last_handle = middle_handle = 0;
 		}
 
@@ -1226,9 +1243,9 @@ static int z3fold_reclaim_page(struct z3fold_pool *pool, unsigned int retries)
 			spin_lock(&pool->lock);
 			list_add(&page->lru, &pool->lru);
 			spin_unlock(&pool->lock);
+			clear_bit(PAGE_CLAIMED, &page->private);
 		} else {
 			z3fold_page_lock(zhdr);
-			clear_bit(PAGE_CLAIMED, &page->private);
 			if (kref_put(&zhdr->refcount,
 					release_z3fold_page_locked)) {
 				atomic64_dec(&pool->pages_nr);
@@ -1243,6 +1260,7 @@ static int z3fold_reclaim_page(struct z3fold_pool *pool, unsigned int retries)
 			list_add(&page->lru, &pool->lru);
 			spin_unlock(&pool->lock);
 			z3fold_page_unlock(zhdr);
+			clear_bit(PAGE_CLAIMED, &page->private);
 		}
 
 		/* We started off locked to we need to lock the pool back */
@@ -1369,7 +1387,8 @@ static bool z3fold_page_isolate(struct page *page, isolate_mode_t mode)
 	VM_BUG_ON_PAGE(!PageMovable(page), page);
 	VM_BUG_ON_PAGE(PageIsolated(page), page);
 
-	if (test_bit(PAGE_HEADLESS, &page->private))
+	if (test_bit(PAGE_HEADLESS, &page->private) ||
+	    test_bit(PAGE_CLAIMED, &page->private))
 		return false;
 
 	zhdr = page_address(page);
-- 
2.20.1

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] z3fold: fix retry mechanism in page reclaim
  2019-09-08 13:29 [PATCH] z3fold: fix retry mechanism in page reclaim Vitaly Wool
@ 2019-09-08 13:56 ` Maciej S. Szmigiero
  2019-09-08 20:44   ` Vitaly Wool
  0 siblings, 1 reply; 3+ messages in thread
From: Maciej S. Szmigiero @ 2019-09-08 13:56 UTC (permalink / raw)
  To: Vitaly Wool
  Cc: Linux-MM, Andrew Morton, linux-kernel,
	Agustín DallʼAlba, Dan Streetman, Vlastimil Babka,
	markus.linnala

On 08.09.2019 15:29, Vitaly Wool wrote:
> z3fold_page_reclaim()'s retry mechanism is broken: on a second
> iteration it will have zhdr from the first one so that zhdr
> is no longer in line with struct page. That leads to crashes when
> the system is stressed.
> 
> Fix that by moving zhdr assignment up.
> 
> While at it, protect against using already freed handles by using
> own local slots structure in z3fold_page_reclaim().
> 
> Reported-by: Markus Linnala <markus.linnala@gmail.com>
> Reported-by: Chris Murphy <bugzilla@colorremedies.com>
> Reported-by: Agustin Dall'Alba <agustin@dallalba.com.ar>
> Signed-off-by: Vitaly Wool <vitalywool@gmail.com>
> ---

Shouldn't this be CC'ed to stable@ ?

Maciej

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] z3fold: fix retry mechanism in page reclaim
  2019-09-08 13:56 ` Maciej S. Szmigiero
@ 2019-09-08 20:44   ` Vitaly Wool
  0 siblings, 0 replies; 3+ messages in thread
From: Vitaly Wool @ 2019-09-08 20:44 UTC (permalink / raw)
  To: Maciej S. Szmigiero
  Cc: Linux-MM, Andrew Morton, LKML, Agustín DallʼAlba,
	Dan Streetman, Vlastimil Babka, Markus Linnala

On Sun, Sep 8, 2019 at 4:56 PM Maciej S. Szmigiero
<mail@maciej.szmigiero.name> wrote:
>
> On 08.09.2019 15:29, Vitaly Wool wrote:
> > z3fold_page_reclaim()'s retry mechanism is broken: on a second
> > iteration it will have zhdr from the first one so that zhdr
> > is no longer in line with struct page. That leads to crashes when
> > the system is stressed.
> >
> > Fix that by moving zhdr assignment up.
> >
> > While at it, protect against using already freed handles by using
> > own local slots structure in z3fold_page_reclaim().
> >
> > Reported-by: Markus Linnala <markus.linnala@gmail.com>
> > Reported-by: Chris Murphy <bugzilla@colorremedies.com>
> > Reported-by: Agustin Dall'Alba <agustin@dallalba.com.ar>
> > Signed-off-by: Vitaly Wool <vitalywool@gmail.com>
> > ---
>
> Shouldn't this be CC'ed to stable@ ?

I guess :)

Thanks,
   Vitaly

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2019-09-08 20:44 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-08 13:29 [PATCH] z3fold: fix retry mechanism in page reclaim Vitaly Wool
2019-09-08 13:56 ` Maciej S. Szmigiero
2019-09-08 20:44   ` Vitaly Wool

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).