* [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; 7+ 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] 7+ 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 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, 2 replies; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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 related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-11-15 0:11 UTC | newest] Thread overview: 7+ 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 2019-11-06 6:09 ` [PATCH v5 2/2] mm/thp: flush file for !is_shmem PageDirty() case in collapse_file() Song Liu
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).