Hi,
I updated the patchset based on previous discussion [1],
I fixed wrong patch separation, remaining race issue, and build failure.
If you still find some issue in this series, please let me know.
Thanks,
Naoya Horiguchi
[1] https://lore.kernel.org/linux-mm/[email protected]/T/#u
---
Summary:
Naoya Horiguchi (2):
mm,hwpoison: fix race with compound page allocation
mm,hwpoison: make get_hwpoison_page call get_any_page()
mm/memory-failure.c | 122 +++++++++++++++++++++++++++++++---------------------
1 file changed, 72 insertions(+), 50 deletions(-)
From: Naoya Horiguchi <[email protected]>
When hugetlb page fault (under overcommitting situation) and
memory_failure() race, VM_BUG_ON_PAGE() is triggered by the following race:
CPU0: CPU1:
gather_surplus_pages()
page = alloc_surplus_huge_page()
memory_failure_hugetlb()
get_hwpoison_page(page)
__get_hwpoison_page(page)
get_page_unless_zero(page)
zero = put_page_testzero(page)
VM_BUG_ON_PAGE(!zero, page)
enqueue_huge_page(h, page)
put_page(page)
__get_hwpoison_page() only checks page refcount before taking additional
one for memory error handling, which is wrong because there's a time
window where compound pages have non-zero refcount during initialization.
So makes __get_hwpoison_page() check page status a bit more for a few
types of compound pages. PageSlab() check is added because otherwise
"non anonymous thp" path is wrongly chosen.
Fixes: ead07f6a867b ("mm/memory-failure: introduce get_hwpoison_page() for consistent refcount handling")
Signed-off-by: Naoya Horiguchi <[email protected]>
Reported-by: Muchun Song <[email protected]>
Cc: [email protected] # 5.12+
---
ChangeLog v4:
- all hugetlb related check in hugetlb_lock,
- fix build error with #ifdef CONFIG_HUGETLB_PAGE
---
mm/memory-failure.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git v5.12/mm/memory-failure.c v5.12_patched/mm/memory-failure.c
index a3659619d293..761f982b6d7b 100644
--- v5.12/mm/memory-failure.c
+++ v5.12_patched/mm/memory-failure.c
@@ -1094,6 +1094,16 @@ static int page_action(struct page_state *ps, struct page *p,
static int __get_hwpoison_page(struct page *page)
{
struct page *head = compound_head(page);
+ int ret = 0;
+
+#ifdef CONFIG_HUGETLB_PAGE
+ spin_lock(&hugetlb_lock);
+ if (PageHuge(head) && (HPageFreed(head) || HPageMigratable(head)))
+ ret = get_page_unless_zero(head);
+ spin_unlock(&hugetlb_lock);
+ if (ret > 0)
+ return ret;
+#endif
if (!PageHuge(head) && PageTransHuge(head)) {
/*
--
2.25.1
On Mon, May 17, 2021 at 01:54:00PM +0900, Naoya Horiguchi wrote:
> From: Naoya Horiguchi <[email protected]>
>
> When hugetlb page fault (under overcommitting situation) and
> memory_failure() race, VM_BUG_ON_PAGE() is triggered by the following race:
>
> CPU0: CPU1:
>
> gather_surplus_pages()
> page = alloc_surplus_huge_page()
> memory_failure_hugetlb()
> get_hwpoison_page(page)
> __get_hwpoison_page(page)
> get_page_unless_zero(page)
> zero = put_page_testzero(page)
> VM_BUG_ON_PAGE(!zero, page)
> enqueue_huge_page(h, page)
> put_page(page)
>
> __get_hwpoison_page() only checks page refcount before taking additional
^^ the? ^^ an
> one for memory error handling, which is wrong because there's a time
> window where compound pages have non-zero refcount during initialization.
>
> So makes __get_hwpoison_page() check page status a bit more for a few
^^ make
> types of compound pages. PageSlab() check is added because otherwise
> "non anonymous thp" path is wrongly chosen.
This is no longer true with this patch, is it? What happened here?
> static int __get_hwpoison_page(struct page *page)
> {
> struct page *head = compound_head(page);
> + int ret = 0;
> +
> +#ifdef CONFIG_HUGETLB_PAGE
> + spin_lock(&hugetlb_lock);
> + if (PageHuge(head) && (HPageFreed(head) || HPageMigratable(head)))
> + ret = get_page_unless_zero(head);
> + spin_unlock(&hugetlb_lock);
> + if (ret > 0)
> + return ret;
> +#endif
I am kind of fine with this, but I wonder whether it makes sense to hide this
details into helper (with an empty stub for non-hugetlb pages)?
> if (!PageHuge(head) && PageTransHuge(head)) {
This !PageHuge could go?
--
Oscar Salvador
SUSE L3
On Mon, May 17, 2021 at 12:12:50PM +0200, Oscar Salvador wrote:
> On Mon, May 17, 2021 at 01:54:00PM +0900, Naoya Horiguchi wrote:
> > From: Naoya Horiguchi <[email protected]>
> >
> > When hugetlb page fault (under overcommitting situation) and
> > memory_failure() race, VM_BUG_ON_PAGE() is triggered by the following race:
> >
> > CPU0: CPU1:
> >
> > gather_surplus_pages()
> > page = alloc_surplus_huge_page()
> > memory_failure_hugetlb()
> > get_hwpoison_page(page)
> > __get_hwpoison_page(page)
> > get_page_unless_zero(page)
> > zero = put_page_testzero(page)
> > VM_BUG_ON_PAGE(!zero, page)
> > enqueue_huge_page(h, page)
> > put_page(page)
> >
> > __get_hwpoison_page() only checks page refcount before taking additional
> ^^ the? ^^ an
> > one for memory error handling, which is wrong because there's a time
> > window where compound pages have non-zero refcount during initialization.
> >
> > So makes __get_hwpoison_page() check page status a bit more for a few
> ^^ make
> > types of compound pages. PageSlab() check is added because otherwise
> > "non anonymous thp" path is wrongly chosen.
>
> This is no longer true with this patch, is it? What happened here?
Sorry, we need remove this. I dropped the changes for PageSlab
because I'm still not sure about what to do (I'd like to do it
in a separate work).
>
> > static int __get_hwpoison_page(struct page *page)
> > {
> > struct page *head = compound_head(page);
> > + int ret = 0;
> > +
> > +#ifdef CONFIG_HUGETLB_PAGE
> > + spin_lock(&hugetlb_lock);
> > + if (PageHuge(head) && (HPageFreed(head) || HPageMigratable(head)))
> > + ret = get_page_unless_zero(head);
> > + spin_unlock(&hugetlb_lock);
> > + if (ret > 0)
> > + return ret;
> > +#endif
>
> I am kind of fine with this, but I wonder whether it makes sense to hide this
> details into helper (with an empty stub for non-hugetlb pages)?
OK, I will do this.
>
> > if (!PageHuge(head) && PageTransHuge(head)) {
> This !PageHuge could go?
I'll update this, too.
Thanks,
Naoya Horiguchi
On 5/16/21 9:54 PM, Naoya Horiguchi wrote:
> From: Naoya Horiguchi <[email protected]>
>
> When hugetlb page fault (under overcommitting situation) and
> memory_failure() race, VM_BUG_ON_PAGE() is triggered by the following race:
>
> CPU0: CPU1:
>
> gather_surplus_pages()
> page = alloc_surplus_huge_page()
> memory_failure_hugetlb()
> get_hwpoison_page(page)
> __get_hwpoison_page(page)
> get_page_unless_zero(page)
> zero = put_page_testzero(page)
> VM_BUG_ON_PAGE(!zero, page)
> enqueue_huge_page(h, page)
> put_page(page)
>
> __get_hwpoison_page() only checks page refcount before taking additional
> one for memory error handling, which is wrong because there's a time
> window where compound pages have non-zero refcount during initialization.
>
> So makes __get_hwpoison_page() check page status a bit more for a few
> types of compound pages. PageSlab() check is added because otherwise
> "non anonymous thp" path is wrongly chosen.
>
> Fixes: ead07f6a867b ("mm/memory-failure: introduce get_hwpoison_page() for consistent refcount handling")
> Signed-off-by: Naoya Horiguchi <[email protected]>
> Reported-by: Muchun Song <[email protected]>
> Cc: [email protected] # 5.12+
> ---
> ChangeLog v4:
> - all hugetlb related check in hugetlb_lock,
> - fix build error with #ifdef CONFIG_HUGETLB_PAGE
> ---
> mm/memory-failure.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git v5.12/mm/memory-failure.c v5.12_patched/mm/memory-failure.c
> index a3659619d293..761f982b6d7b 100644
> --- v5.12/mm/memory-failure.c
> +++ v5.12_patched/mm/memory-failure.c
> @@ -1094,6 +1094,16 @@ static int page_action(struct page_state *ps, struct page *p,
> static int __get_hwpoison_page(struct page *page)
> {
> struct page *head = compound_head(page);
> + int ret = 0;
> +
> +#ifdef CONFIG_HUGETLB_PAGE
> + spin_lock(&hugetlb_lock);
> + if (PageHuge(head) && (HPageFreed(head) || HPageMigratable(head)))
> + ret = get_page_unless_zero(head);
> + spin_unlock(&hugetlb_lock);
> + if (ret > 0)
> + return ret;
> +#endif
I must be missing something.
The above code makes sure the page is not in one of these transitive
hugetlb states as mentioned in the commit message. It only attempts
to take a reference on the page if it is not in one of these states.
However, if it is in such a transitive state (!HPageFreed(head) &&
!HPageMigratable(head)) we will fall through and execute the code:
if (get_page_unless_zero(head)) {
if (head == compound_head(page))
return 1;
So, it seems like we will always do a get_page_unless_zero() for
PageHuge() pages?
Also, due to commit db71ef79b59b ("hugetlb: make free_huge_page irq
safe") you need to make sure interrupts are disabled when taking
hugetlb_lock.
--
Mike Kravetz
On Mon, May 17, 2021 at 01:11:25PM -0700, Mike Kravetz wrote:
> On 5/16/21 9:54 PM, Naoya Horiguchi wrote:
> > From: Naoya Horiguchi <[email protected]>
> >
> > When hugetlb page fault (under overcommitting situation) and
> > memory_failure() race, VM_BUG_ON_PAGE() is triggered by the following race:
> >
> > CPU0: CPU1:
> >
> > gather_surplus_pages()
> > page = alloc_surplus_huge_page()
> > memory_failure_hugetlb()
> > get_hwpoison_page(page)
> > __get_hwpoison_page(page)
> > get_page_unless_zero(page)
> > zero = put_page_testzero(page)
> > VM_BUG_ON_PAGE(!zero, page)
> > enqueue_huge_page(h, page)
> > put_page(page)
> >
> > __get_hwpoison_page() only checks page refcount before taking additional
> > one for memory error handling, which is wrong because there's a time
> > window where compound pages have non-zero refcount during initialization.
> >
> > So makes __get_hwpoison_page() check page status a bit more for a few
> > types of compound pages. PageSlab() check is added because otherwise
> > "non anonymous thp" path is wrongly chosen.
> >
> > Fixes: ead07f6a867b ("mm/memory-failure: introduce get_hwpoison_page() for consistent refcount handling")
> > Signed-off-by: Naoya Horiguchi <[email protected]>
> > Reported-by: Muchun Song <[email protected]>
> > Cc: [email protected] # 5.12+
> > ---
> > ChangeLog v4:
> > - all hugetlb related check in hugetlb_lock,
> > - fix build error with #ifdef CONFIG_HUGETLB_PAGE
> > ---
> > mm/memory-failure.c | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git v5.12/mm/memory-failure.c v5.12_patched/mm/memory-failure.c
> > index a3659619d293..761f982b6d7b 100644
> > --- v5.12/mm/memory-failure.c
> > +++ v5.12_patched/mm/memory-failure.c
> > @@ -1094,6 +1094,16 @@ static int page_action(struct page_state *ps, struct page *p,
> > static int __get_hwpoison_page(struct page *page)
> > {
> > struct page *head = compound_head(page);
> > + int ret = 0;
> > +
> > +#ifdef CONFIG_HUGETLB_PAGE
> > + spin_lock(&hugetlb_lock);
> > + if (PageHuge(head) && (HPageFreed(head) || HPageMigratable(head)))
> > + ret = get_page_unless_zero(head);
> > + spin_unlock(&hugetlb_lock);
> > + if (ret > 0)
> > + return ret;
> > +#endif
>
> I must be missing something.
>
> The above code makes sure the page is not in one of these transitive
> hugetlb states as mentioned in the commit message. It only attempts
> to take a reference on the page if it is not in one of these states.
>
> However, if it is in such a transitive state (!HPageFreed(head) &&
> !HPageMigratable(head)) we will fall through and execute the code:
>
> if (get_page_unless_zero(head)) {
> if (head == compound_head(page))
> return 1;
>
> So, it seems like we will always do a get_page_unless_zero() for
> PageHuge() pages?
Right, no need to fall through in such a case.
>
> Also, due to commit db71ef79b59b ("hugetlb: make free_huge_page irq
> safe") you need to make sure interrupts are disabled when taking
> hugetlb_lock.
Thanks, I'll rebase to latest mainline and comply with new semantics.
- Naoya