* [PATCH v3] mm,thp: recheck each page before collapsing file THP
@ 2019-10-18 18:03 Song Liu
2019-10-19 1:17 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Song Liu @ 2019-10-18 18:03 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(), after locking the page, it is necessary to recheck
that the page is up-to-date. Add PageUptodate() check for both shmem THP
and file THP.
Current khugepaged should not try to collapse dirty file THP, because it
is limited to read only text. Add a PageDirty check and warning for file
THP. This is added after page_mapping() check, because if the page is
truncated, it might be dirty.
Fixes: 99cb0dbd47a1 ("mm,thp: add read-only THP support for (non-shmem) FS")
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
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>
---
Changes v1 => v2:
Rearrange the checks per feedback from Johannes, Rik, and Kirill.
Changes v2 => v3:
Remove redudant checks before trylock_page(). (Johannes)
---
mm/khugepaged.c | 27 +++++++++++++++------------
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 0a1b4b484ac5..e20bb9d3482d 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);
+
+ /* double check 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,15 @@ static void collapse_file(struct mm_struct *mm,
goto out_unlock;
}
+ /*
+ * khugepaged should not try to collapse dirty pages for
+ * file THP. Show warning if this somehow happens.
+ */
+ if (WARN_ON_ONCE(!is_shmem && PageDirty(page))) {
+ 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 v3] mm,thp: recheck each page before collapsing file THP
2019-10-18 18:03 [PATCH v3] mm,thp: recheck each page before collapsing file THP Song Liu
@ 2019-10-19 1:17 ` Andrew Morton
2019-10-19 5:24 ` Song Liu
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2019-10-19 1:17 UTC (permalink / raw)
To: Song Liu
Cc: linux-kernel, linux-mm, matthew.wilcox, kernel-team,
william.kucharski, kirill.shutemov, Johannes Weiner,
Hugh Dickins
On Fri, 18 Oct 2019 11:03:45 -0700 Song Liu <songliubraving@fb.com> wrote:
> In collapse_file(), after locking the page, it is necessary to recheck
> that the page is up-to-date. Add PageUptodate() check for both shmem THP
> and file THP.
>
> Current khugepaged should not try to collapse dirty file THP, because it
> is limited to read only text. Add a PageDirty check and warning for file
> THP. This is added after page_mapping() check, because if the page is
> truncated, it might be dirty.
When fixing a bug, please always fully describe the end-user visible
effects of that bug. This is vital information for people who are
considering the fix for backporting.
I'm suspecting that you've found a race condition which can trigger a
VM_BUG_ON_PAGE(), which is rather serious. But that was just a wild
guess. Please don't make us wildly guess :(
The old code looked rather alarming:
} 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);
We don't have a ref on that page. After we've released the xarray lock
we have no business playing with *page at all, correct?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] mm,thp: recheck each page before collapsing file THP
2019-10-19 1:17 ` Andrew Morton
@ 2019-10-19 5:24 ` Song Liu
2019-10-21 18:03 ` Johannes Weiner
2019-11-06 4:58 ` Andrew Morton
0 siblings, 2 replies; 6+ messages in thread
From: Song Liu @ 2019-10-19 5:24 UTC (permalink / raw)
To: Andrew Morton
Cc: open list, linux-mm, matthew.wilcox, Kernel Team,
william.kucharski, kirill.shutemov, Johannes Weiner,
Hugh Dickins
> On Oct 18, 2019, at 6:17 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Fri, 18 Oct 2019 11:03:45 -0700 Song Liu <songliubraving@fb.com> wrote:
>
>> In collapse_file(), after locking the page, it is necessary to recheck
>> that the page is up-to-date. Add PageUptodate() check for both shmem THP
>> and file THP.
>>
>> Current khugepaged should not try to collapse dirty file THP, because it
>> is limited to read only text. Add a PageDirty check and warning for file
>> THP. This is added after page_mapping() check, because if the page is
>> truncated, it might be dirty.
>
> When fixing a bug, please always fully describe the end-user visible
> effects of that bug. This is vital information for people who are
> considering the fix for backporting.
The end user effect is, corruption in page cache. While grouping pages
into a huge page, the page cache mistakenly includes some pages that
are not uptodate, and considers the huge page is uptodate.
I am attaching updated commit log to the end.
>
> I'm suspecting that you've found a race condition which can trigger a
> VM_BUG_ON_PAGE(), which is rather serious. But that was just a wild
> guess. Please don't make us wildly guess :(
>
> The old code looked rather alarming:
>
> } 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);
>
> We don't have a ref on that page. After we've released the xarray lock
> we have no business playing with *page at all, correct?
Yeah, this piece is not just redundant, but also buggy. I am also
including some information about it.
Updated commit log:
============================= 8< =============================
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. Add a warning with the PageDirty() check as it should
not happen. This warning is added after page_mapping() check, because
if the page is truncated, it might be dirty.
Fixes: 99cb0dbd47a1 ("mm,thp: add read-only THP support for (non-shmem) FS")
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
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>
============================= 8< =============================
Thanks,
Song
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] mm,thp: recheck each page before collapsing file THP
2019-10-19 5:24 ` Song Liu
@ 2019-10-21 18:03 ` Johannes Weiner
2019-11-06 4:58 ` Andrew Morton
1 sibling, 0 replies; 6+ messages in thread
From: Johannes Weiner @ 2019-10-21 18:03 UTC (permalink / raw)
To: Song Liu
Cc: Andrew Morton, open list, linux-mm, matthew.wilcox, Kernel Team,
william.kucharski, kirill.shutemov, Hugh Dickins
On Sat, Oct 19, 2019 at 05:24:00AM +0000, Song Liu 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. Add a warning with the PageDirty() check as it should
> not happen. This warning is added after page_mapping() check, because
> if the page is truncated, it might be dirty.
>
> Fixes: 99cb0dbd47a1 ("mm,thp: add read-only THP support for (non-shmem) FS")
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> 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>
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] mm,thp: recheck each page before collapsing file THP
2019-10-19 5:24 ` Song Liu
2019-10-21 18:03 ` Johannes Weiner
@ 2019-11-06 4:58 ` Andrew Morton
2019-11-06 5:45 ` Song Liu
1 sibling, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2019-11-06 4:58 UTC (permalink / raw)
To: Song Liu
Cc: open list, linux-mm, matthew.wilcox, Kernel Team,
william.kucharski, kirill.shutemov, Johannes Weiner,
Hugh Dickins
On Sat, 19 Oct 2019 05:24:00 +0000 Song Liu <songliubraving@fb.com> wrote:
> > We don't have a ref on that page. After we've released the xarray lock
> > we have no business playing with *page at all, correct?
>
> Yeah, this piece is not just redundant, but also buggy. I am also
> including some information about it.
>
> Updated commit log:
>
> ============================= 8< =============================
>
> 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. Add a warning with the PageDirty() check as it should
> not happen. This warning is added after page_mapping() check, because
> if the page is truncated, it might be dirty.
I've lost the plot on this patch. I have the v3 patch plus these fixes:
http://lkml.kernel.org/r/20191028221414.3685035-1-songliubraving@fb.com
http://lkml.kernel.org/r/20191022191006.411277-1-songliubraving@fb.com
http://lkml.kernel.org/r/20191030200736.3455046-1-songliubraving@fb.com
and there's a v4 which I can't correlate with the above. And there has
been discussion about deferring some of the filemap_flush() changes
until later.
So I think it's best if we just start again. Can you please prepare
and send out a v5 (which might be a 2-patch series)?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] mm,thp: recheck each page before collapsing file THP
2019-11-06 4:58 ` Andrew Morton
@ 2019-11-06 5:45 ` Song Liu
0 siblings, 0 replies; 6+ messages in thread
From: Song Liu @ 2019-11-06 5:45 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 5, 2019, at 8:58 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Sat, 19 Oct 2019 05:24:00 +0000 Song Liu <songliubraving@fb.com> wrote:
>
>>> We don't have a ref on that page. After we've released the xarray lock
>>> we have no business playing with *page at all, correct?
>>
>> Yeah, this piece is not just redundant, but also buggy. I am also
>> including some information about it.
>>
>> Updated commit log:
>>
>> ============================= 8< =============================
>>
>> 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. Add a warning with the PageDirty() check as it should
>> not happen. This warning is added after page_mapping() check, because
>> if the page is truncated, it might be dirty.
>
> I've lost the plot on this patch. I have the v3 patch plus these fixes:
>
> http://lkml.kernel.org/r/20191028221414.3685035-1-songliubraving@fb.com
> http://lkml.kernel.org/r/20191022191006.411277-1-songliubraving@fb.com
> http://lkml.kernel.org/r/20191030200736.3455046-1-songliubraving@fb.com
>
> and there's a v4 which I can't correlate with the above. And there has
> been discussion about deferring some of the filemap_flush() changes
> until later.
>
> So I think it's best if we just start again. Can you please prepare
> and send out a v5 (which might be a 2-patch series)?
Sounds good. Sending v5 soon.
Thanks,
Song
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-11-06 5:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-18 18:03 [PATCH v3] mm,thp: recheck each page before collapsing file THP Song Liu
2019-10-19 1:17 ` Andrew Morton
2019-10-19 5:24 ` Song Liu
2019-10-21 18:03 ` Johannes Weiner
2019-11-06 4:58 ` Andrew Morton
2019-11-06 5:45 ` 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).