Linux-mm Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v5 0/2] fix and improvement for file THP
@ 2019-11-06  6:09 Song Liu
  2019-11-06  6:09 ` [PATCH v5 1/2] mm,thp: recheck each page before collapsing " Song Liu
  2019-11-06  6:09 ` [PATCH v5 2/2] mm/thp: flush file for !is_shmem PageDirty() case in collapse_file() Song Liu
  0 siblings, 2 replies; 5+ messages in thread
From: Song Liu @ 2019-11-06  6:09 UTC (permalink / raw)
  To: linux-kernel, linux-mm, akpm
  Cc: matthew.wilcox, kernel-team, william.kucharski, kirill.shutemov,
	Song Liu

This set includes a fix and an improvement for file THP.

1/2 fixes pagecache corruption for file THP, where !PageUptodate page gets
collapsed into huge page. 1/2 is needed for 5.4.

2/2 improve collapse of file THP by flushing dirty page in collapse_file().
2/2 can wait until 5.5.

Thanks,
Song

Song Liu (2):
  mm,thp: recheck each page before collapsing file THP
  mm/thp: flush file for !is_shmem PageDirty() case in collapse_file()

 mm/khugepaged.c | 42 ++++++++++++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 10 deletions(-)

--
2.17.1


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

* [PATCH v5 1/2] mm,thp: recheck each page before collapsing file THP
  2019-11-06  6:09 [PATCH v5 0/2] fix and improvement for file THP Song Liu
@ 2019-11-06  6:09 ` " Song Liu
  2019-11-08  3:53   ` Andrew Morton
  2019-11-06  6:09 ` [PATCH v5 2/2] mm/thp: flush file for !is_shmem PageDirty() case in collapse_file() Song Liu
  1 sibling, 1 reply; 5+ messages in thread
From: Song Liu @ 2019-11-06  6:09 UTC (permalink / raw)
  To: linux-kernel, linux-mm, akpm
  Cc: matthew.wilcox, kernel-team, william.kucharski, kirill.shutemov,
	Song Liu, Johannes Weiner, Hugh Dickins

In collapse_file(), for !is_shmem case, current check cannot guarantee
the locked page is up-to-date.  Specifically, xas_unlock_irq() should
not be called before lock_page() and get_page(); and it is necessary to
recheck PageUptodate() after locking the page.

With this bug and CONFIG_READ_ONLY_THP_FOR_FS=y, madvise(HUGE)'ed .text
may contain corrupted data.  This is because khugepaged mistakenly
collapses some not up-to-date sub pages into a huge page, and assumes
the huge page is up-to-date.  This will NOT corrupt data in the disk,
because the page is read-only and never written back.  Fix this by
properly checking PageUptodate() after locking the page.  This check
replaces "VM_BUG_ON_PAGE(!PageUptodate(page), page);".

Also, move PageDirty() check after locking the page.  Current
khugepaged should not try to collapse dirty file THP, because it is
limited to read-only .text. The only case we hit a dirty page here is
when the page hasn't been written since write. Bail out and retry when
this happens.

syzbot reported bug on previous version of this patch.

Fixes: 99cb0dbd47a1 ("mm,thp: add read-only THP support for (non-shmem) FS")
Reported-by: syzbot+efb9e48b9fbdc49bb34a@syzkaller.appspotmail.com
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: William Kucharski <william.kucharski@oracle.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Song Liu <songliubraving@fb.com>

---

[songliubraving@fb.com: v4]
 Link: http://lkml.kernel.org/r/20191022191006.411277-1-songliubraving@fb.com
[songliubraving@fb.com: fix deadlock in collapse_file()]
 Link: http://lkml.kernel.org/r/20191028221414.3685035-1-songliubraving@fb.com
Link: http://lkml.kernel.org/r/20191018180345.4188310-1-songliubraving@fb.com
[songliubraving@fb.com: flush file for !is_shmem PageDirty() case in collapse_file()]
https://lkml.kernel.org/linux-mm/20191030200736.3455046-1-songliubraving@fb.com/

---
 mm/khugepaged.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 0a1b4b484ac5..40215795d641 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -1601,17 +1601,6 @@ static void collapse_file(struct mm_struct *mm,
 					result = SCAN_FAIL;
 					goto xa_unlocked;
 				}
-			} else if (!PageUptodate(page)) {
-				xas_unlock_irq(&xas);
-				wait_on_page_locked(page);
-				if (!trylock_page(page)) {
-					result = SCAN_PAGE_LOCK;
-					goto xa_unlocked;
-				}
-				get_page(page);
-			} else if (PageDirty(page)) {
-				result = SCAN_FAIL;
-				goto xa_locked;
 			} else if (trylock_page(page)) {
 				get_page(page);
 				xas_unlock_irq(&xas);
@@ -1626,7 +1615,12 @@ static void collapse_file(struct mm_struct *mm,
 		 * without racing with truncate.
 		 */
 		VM_BUG_ON_PAGE(!PageLocked(page), page);
-		VM_BUG_ON_PAGE(!PageUptodate(page), page);
+
+		/* make sure the page is up to date */
+		if (unlikely(!PageUptodate(page))) {
+			result = SCAN_FAIL;
+			goto out_unlock;
+		}

 		/*
 		 * If file was truncated then extended, or hole-punched, before
@@ -1642,6 +1636,16 @@ static void collapse_file(struct mm_struct *mm,
 			goto out_unlock;
 		}

+		if (!is_shmem && PageDirty(page)) {
+			/*
+			 * khugepaged only works on read-only fd, so this
+			 * page is dirty because it hasn't been flushed
+			 * since first write.
+			 */
+			result = SCAN_FAIL;
+			goto out_unlock;
+		}
+
 		if (isolate_lru_page(page)) {
 			result = SCAN_DEL_PAGE_LRU;
 			goto out_unlock;
--
2.17.1


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

* [PATCH v5 2/2] mm/thp: flush file for !is_shmem PageDirty() case in collapse_file()
  2019-11-06  6:09 [PATCH v5 0/2] fix and improvement for file THP Song Liu
  2019-11-06  6:09 ` [PATCH v5 1/2] mm,thp: recheck each page before collapsing " Song Liu
@ 2019-11-06  6:09 ` Song Liu
  1 sibling, 0 replies; 5+ messages in thread
From: Song Liu @ 2019-11-06  6:09 UTC (permalink / raw)
  To: linux-kernel, linux-mm, akpm
  Cc: matthew.wilcox, kernel-team, william.kucharski, kirill.shutemov,
	Song Liu, Hugh Dickins, Johannes Weiner, Matthew Wilcox

For non-shmem file THPs, khugepaged only collapses read only .text mapping
(VM_DENYWRITE). These pages should not be dirty except the case where the
file hasn't been flushed since first write.

Call filemap_flush() in collapse_file() to accelerate the write back in
such cases.

Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: William Kucharski <william.kucharski@oracle.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Song Liu <songliubraving@fb.com>

---

[songliubraving@fb.com: flush file for !is_shmem PageDirty() case in collapse_file()]
https://lkml.kernel.org/linux-mm/20191030200736.3455046-1-songliubraving@fb.com/

---
 mm/khugepaged.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 40215795d641..513cce897794 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -1601,6 +1601,24 @@ static void collapse_file(struct mm_struct *mm,
 					result = SCAN_FAIL;
 					goto xa_unlocked;
 				}
+			} else if (PageDirty(page)) {
+				/*
+				 * khugepaged only works on read-only fd,
+				 * so this page is dirty because it hasn't
+				 * been flushed since first write. There
+				 * won't be new dirty pages.
+				 *
+				 * Trigger async flush here and hope the
+				 * writeback is done when khugepaged
+				 * revisits this page.
+				 *
+				 * This is a one-off situation. We are not
+				 * forcing writeback in loop.
+				 */
+				xas_unlock_irq(&xas);
+				filemap_flush(mapping);
+				result = SCAN_FAIL;
+				goto xa_unlocked;
 			} else if (trylock_page(page)) {
 				get_page(page);
 				xas_unlock_irq(&xas);
--
2.17.1


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

* Re: [PATCH v5 1/2] mm,thp: recheck each page before collapsing file THP
  2019-11-06  6:09 ` [PATCH v5 1/2] mm,thp: recheck each page before collapsing " Song Liu
@ 2019-11-08  3:53   ` Andrew Morton
  2019-11-08  3:58     ` Song Liu
  0 siblings, 1 reply; 5+ messages in thread
From: Andrew Morton @ 2019-11-08  3:53 UTC (permalink / raw)
  To: Song Liu
  Cc: linux-kernel, linux-mm, matthew.wilcox, kernel-team,
	william.kucharski, kirill.shutemov, Johannes Weiner,
	Hugh Dickins

On Tue, 5 Nov 2019 22:09:29 -0800 Song Liu <songliubraving@fb.com> wrote:

> In collapse_file(), for !is_shmem case, current check cannot guarantee
> the locked page is up-to-date.  Specifically, xas_unlock_irq() should
> not be called before lock_page() and get_page(); and it is necessary to
> recheck PageUptodate() after locking the page.
> 
> With this bug and CONFIG_READ_ONLY_THP_FOR_FS=y, madvise(HUGE)'ed .text
> may contain corrupted data.  This is because khugepaged mistakenly
> collapses some not up-to-date sub pages into a huge page, and assumes
> the huge page is up-to-date.  This will NOT corrupt data in the disk,
> because the page is read-only and never written back.  Fix this by
> properly checking PageUptodate() after locking the page.  This check
> replaces "VM_BUG_ON_PAGE(!PageUptodate(page), page);".
> 
> Also, move PageDirty() check after locking the page.  Current
> khugepaged should not try to collapse dirty file THP, because it is
> limited to read-only .text. The only case we hit a dirty page here is
> when the page hasn't been written since write. Bail out and retry when
> this happens.

Incorrect data is pretty serious.  Should we backport this into -stable
kernels?

(I suspect I already asked this in response to earier versions, sorry ;))


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

* Re: [PATCH v5 1/2] mm,thp: recheck each page before collapsing file THP
  2019-11-08  3:53   ` Andrew Morton
@ 2019-11-08  3:58     ` Song Liu
  0 siblings, 0 replies; 5+ messages in thread
From: Song Liu @ 2019-11-08  3:58 UTC (permalink / raw)
  To: Andrew Morton
  Cc: open list, linux-mm, matthew.wilcox, Kernel Team,
	william.kucharski, kirill.shutemov, Johannes Weiner,
	Hugh Dickins



> On Nov 7, 2019, at 7:53 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> On Tue, 5 Nov 2019 22:09:29 -0800 Song Liu <songliubraving@fb.com> wrote:
> 
>> In collapse_file(), for !is_shmem case, current check cannot guarantee
>> the locked page is up-to-date.  Specifically, xas_unlock_irq() should
>> not be called before lock_page() and get_page(); and it is necessary to
>> recheck PageUptodate() after locking the page.
>> 
>> With this bug and CONFIG_READ_ONLY_THP_FOR_FS=y, madvise(HUGE)'ed .text
>> may contain corrupted data.  This is because khugepaged mistakenly
>> collapses some not up-to-date sub pages into a huge page, and assumes
>> the huge page is up-to-date.  This will NOT corrupt data in the disk,
>> because the page is read-only and never written back.  Fix this by
>> properly checking PageUptodate() after locking the page.  This check
>> replaces "VM_BUG_ON_PAGE(!PageUptodate(page), page);".
>> 
>> Also, move PageDirty() check after locking the page.  Current
>> khugepaged should not try to collapse dirty file THP, because it is
>> limited to read-only .text. The only case we hit a dirty page here is
>> when the page hasn't been written since write. Bail out and retry when
>> this happens.
> 
> Incorrect data is pretty serious.  Should we backport this into -stable
> kernels?
> 
> (I suspect I already asked this in response to earier versions, sorry ;))

This is new feature (and new bug :( ) in 5.4. So no need to back port.

Thanks,
Song

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

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-06  6:09 [PATCH v5 0/2] fix and improvement for file THP Song Liu
2019-11-06  6:09 ` [PATCH v5 1/2] mm,thp: recheck each page before collapsing " Song Liu
2019-11-08  3:53   ` Andrew Morton
2019-11-08  3:58     ` Song Liu
2019-11-06  6:09 ` [PATCH v5 2/2] mm/thp: flush file for !is_shmem PageDirty() case in collapse_file() Song Liu

Linux-mm Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mm/0 linux-mm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mm linux-mm/ https://lore.kernel.org/linux-mm \
		linux-mm@kvack.org
	public-inbox-index linux-mm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kvack.linux-mm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git