* [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
0 siblings, 1 reply; 6+ 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] 6+ 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-13 23:47 ` Song Liu
0 siblings, 2 replies; 6+ 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 related [flat|nested] 6+ 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
2019-11-13 23:47 ` Song Liu
1 sibling, 1 reply; 6+ 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] 6+ 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; 6+ 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] 6+ 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-13 23:47 ` Song Liu
2019-11-15 0:10 ` Andrew Morton
1 sibling, 1 reply; 6+ messages in thread
From: Song Liu @ 2019-11-13 23:47 UTC (permalink / raw)
To: open list, Linux MM, Andrew Morton
Cc: matthew.wilcox, Kernel Team, william.kucharski, kirill.shutemov,
Johannes Weiner, Hugh Dickins
> On Nov 5, 2019, at 10:09 PM, 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.
>
> 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>
I think we need this in 5.4 official, but I haven't seen it in Linus'
master branch.
Hi Andrew,
Could you please send patch/pull-request for it?
Thanks,
Song
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v5 1/2] mm,thp: recheck each page before collapsing file THP
2019-11-13 23:47 ` Song Liu
@ 2019-11-15 0:10 ` Andrew Morton
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Morton @ 2019-11-15 0:10 UTC (permalink / raw)
To: Song Liu
Cc: open list, Linux MM, matthew.wilcox, Kernel Team,
william.kucharski, kirill.shutemov, Johannes Weiner,
Hugh Dickins
On Wed, 13 Nov 2019 23:47:06 +0000 Song Liu <songliubraving@fb.com> wrote:
>
> > On Nov 5, 2019, at 10:09 PM, 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.
> >
> > 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>
>
> I think we need this in 5.4 official, but I haven't seen it in Linus'
> master branch.
>
> Hi Andrew,
>
> Could you please send patch/pull-request for it?
Yep, today or tomorrow...
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-11-15 0:11 UTC | newest]
Thread overview: 6+ 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-13 23:47 ` Song Liu
2019-11-15 0:10 ` Andrew Morton
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).