Hi everyone,
This series contains a few fixup patches to fix incorrect update of page
refcnt, fix possible use-after-free issue and so on. More details can be
found in the respective changelogs.
Thanks!
Miaohe Lin (6):
mm, hwpoison: fix page refcnt leaking in try_memory_failure_hugetlb()
mm, hwpoison: fix page refcnt leaking in unpoison_memory()
mm, hwpoison: fix extra put_page() in soft_offline_page()
mm, hwpoison: fix possible use-after-free in mf_dax_kill_procs()
mm, hwpoison: kill procs if unmap fails
mm, hwpoison: avoid trying to unpoison reserved page
mm/memory-failure.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
--
2.23.0
For reserved pages, HWPoison flag will be set without increasing the page
refcnt. So we shouldn't even try to unpoison these pages and thus decrease
the page refcnt unexpectly. Add a PageReserved() check to filter this case
out and remove the below unneeded zero page (zero page is reserved) check.
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/memory-failure.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 5f9615a86296..c831c41bb092 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -2355,7 +2355,7 @@ int unpoison_memory(unsigned long pfn)
goto unlock_mutex;
}
- if (PageSlab(page) || PageTable(page))
+ if (PageSlab(page) || PageTable(page) || PageReserved(page))
goto unlock_mutex;
ret = get_hwpoison_page(p, MF_UNPOISON);
@@ -2386,7 +2386,7 @@ int unpoison_memory(unsigned long pfn)
freeit = !!TestClearPageHWPoison(p);
put_page(page);
- if (freeit && !(pfn == my_zero_pfn(0) && page_count(p) == 1)) {
+ if (freeit) {
put_page(page);
ret = 0;
}
--
2.23.0
After kill_procs(), tk will be freed without being removed from the to_kill
list. In the next iteration, the freed list entry in the to_kill list will
be accessed, thus leading to use-after-free issue. Fix it by reinitializing
the to_kill list after unmap_and_kill().
Fixes: c36e20249571 ("mm: introduce mf_dax_kill_procs() for fsdax case")
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/memory-failure.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 7023c3d81273..a2f4e8b00a26 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1658,6 +1658,8 @@ int mf_dax_kill_procs(struct address_space *mapping, pgoff_t index,
collect_procs_fsdax(page, mapping, index, &to_kill);
unmap_and_kill(&to_kill, page_to_pfn(page), mapping,
index, mf_flags);
+ /* Reinitialize to_kill list for later resuing. */
+ INIT_LIST_HEAD(&to_kill);
unlock:
dax_unlock_mapping_entry(mapping, index, cookie);
}
--
2.23.0
When hwpoison_filter() refuses to soft offline a page, the page refcnt
incremented previously by MF_COUNT_INCREASED would have been consumed
via get_hwpoison_page() if ret <= 0. So the put_ref_page() here will
put the extra one. Remove it to fix the issue.
Fixes: 9113eaf331bf ("mm/memory-failure.c: add hwpoison_filter for soft offline")
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/memory-failure.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 0c5ad7505b99..7023c3d81273 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -2591,8 +2591,6 @@ int soft_offline_page(unsigned long pfn, int flags)
if (hwpoison_filter(page)) {
if (ret > 0)
put_page(page);
- else
- put_ref_page(ref_page);
mutex_unlock(&mf_mutex);
return -EOPNOTSUPP;
--
2.23.0
On 2022/8/18 21:00, Miaohe Lin wrote:
> Hi everyone,
> This series contains a few fixup patches to fix incorrect update of page
> refcnt, fix possible use-after-free issue and so on. More details can be
> found in the respective changelogs.
> Thanks!
Sorry for the wrong patch series cover name. It should be memory-failure instead of hugetlb.
When free_raw_hwp_pages() fails its work, the refcnt of the hugetlb page
would have been incremented if ret > 0. Using put_page() to fix refcnt
leaking in this case.
Fixes: debb6b9c3fdd ("mm, hwpoison: make unpoison aware of raw error info in hwpoisoned hugepage")
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/memory-failure.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 22840cd5fe59..0c5ad7505b99 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -2378,6 +2378,7 @@ int unpoison_memory(unsigned long pfn)
count = free_raw_hwp_pages(page, false);
if (count == 0) {
ret = -EBUSY;
+ put_page(page);
goto unlock_mutex;
}
}
--
2.23.0
If try_to_unmap() fails, the hwpoisoned page still resides in the address
space of some processes. We should kill these processes or the hwpoisoned
page might be consumed later. collect_procs() is always called to collect
relevant processes now so they can be killed later if unmap fails.
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/memory-failure.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index a2f4e8b00a26..5f9615a86296 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1396,7 +1396,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
struct address_space *mapping;
LIST_HEAD(tokill);
bool unmap_success;
- int kill = 1, forcekill;
+ int forcekill;
bool mlocked = PageMlocked(hpage);
/*
@@ -1437,7 +1437,6 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
if (page_mkclean(hpage)) {
SetPageDirty(hpage);
} else {
- kill = 0;
ttu |= TTU_IGNORE_HWPOISON;
pr_info("%#lx: corrupted page was clean: dropped without side effects\n",
pfn);
@@ -1452,8 +1451,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
* Error handling: We ignore errors here because
* there's nothing that can be done.
*/
- if (kill)
- collect_procs(hpage, &tokill, flags & MF_ACTION_REQUIRED);
+ collect_procs(hpage, &tokill, flags & MF_ACTION_REQUIRED);
if (PageHuge(hpage) && !PageAnon(hpage)) {
/*
@@ -1495,7 +1493,8 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
* use a more force-full uncatchable kill to prevent
* any accesses to the poisoned memory.
*/
- forcekill = PageDirty(hpage) || (flags & MF_MUST_KILL);
+ forcekill = PageDirty(hpage) || (flags & MF_MUST_KILL) ||
+ !unmap_success;
kill_procs(&tokill, forcekill, !unmap_success, pfn, flags);
return unmap_success;
--
2.23.0
On Thu, Aug 18, 2022 at 09:00:12PM +0800, Miaohe Lin wrote:
> When free_raw_hwp_pages() fails its work, the refcnt of the hugetlb page
> would have been incremented if ret > 0. Using put_page() to fix refcnt
> leaking in this case.
>
> Fixes: debb6b9c3fdd ("mm, hwpoison: make unpoison aware of raw error info in hwpoisoned hugepage")
> Signed-off-by: Miaohe Lin <[email protected]>
Acked-by: Naoya Horiguchi <[email protected]>
> ---
> mm/memory-failure.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 22840cd5fe59..0c5ad7505b99 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -2378,6 +2378,7 @@ int unpoison_memory(unsigned long pfn)
> count = free_raw_hwp_pages(page, false);
> if (count == 0) {
> ret = -EBUSY;
> + put_page(page);
> goto unlock_mutex;
> }
> }
> --
> 2.23.0
On Thu, Aug 18, 2022 at 09:00:15PM +0800, Miaohe Lin wrote:
> If try_to_unmap() fails, the hwpoisoned page still resides in the address
> space of some processes. We should kill these processes or the hwpoisoned
> page might be consumed later. collect_procs() is always called to collect
> relevant processes now so they can be killed later if unmap fails.
>
> Signed-off-by: Miaohe Lin <[email protected]>
> ---
> mm/memory-failure.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index a2f4e8b00a26..5f9615a86296 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1396,7 +1396,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
> struct address_space *mapping;
> LIST_HEAD(tokill);
> bool unmap_success;
> - int kill = 1, forcekill;
> + int forcekill;
> bool mlocked = PageMlocked(hpage);
>
> /*
> @@ -1437,7 +1437,6 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
> if (page_mkclean(hpage)) {
> SetPageDirty(hpage);
> } else {
> - kill = 0;
> ttu |= TTU_IGNORE_HWPOISON;
> pr_info("%#lx: corrupted page was clean: dropped without side effects\n",
> pfn);
> @@ -1452,8 +1451,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
> * Error handling: We ignore errors here because
> * there's nothing that can be done.
This above comment might be deprecated now (I'm not sure what this really mean),
so could you drop or update this?
Anyway, the patch looks good to me.
Acked-by: Naoya Horiguchi <[email protected]>
> */
> - if (kill)
> - collect_procs(hpage, &tokill, flags & MF_ACTION_REQUIRED);
> + collect_procs(hpage, &tokill, flags & MF_ACTION_REQUIRED);
>
> if (PageHuge(hpage) && !PageAnon(hpage)) {
> /*
> @@ -1495,7 +1493,8 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
> * use a more force-full uncatchable kill to prevent
> * any accesses to the poisoned memory.
> */
> - forcekill = PageDirty(hpage) || (flags & MF_MUST_KILL);
> + forcekill = PageDirty(hpage) || (flags & MF_MUST_KILL) ||
> + !unmap_success;
> kill_procs(&tokill, forcekill, !unmap_success, pfn, flags);
>
> return unmap_success;
> --
> 2.23.0
On Thu, Aug 18, 2022 at 09:00:13PM +0800, Miaohe Lin wrote:
> When hwpoison_filter() refuses to soft offline a page, the page refcnt
> incremented previously by MF_COUNT_INCREASED would have been consumed
> via get_hwpoison_page() if ret <= 0. So the put_ref_page() here will
> put the extra one. Remove it to fix the issue.
>
> Fixes: 9113eaf331bf ("mm/memory-failure.c: add hwpoison_filter for soft offline")
> Signed-off-by: Miaohe Lin <[email protected]>
Acked-by: Naoya Horiguchi <[email protected]>
> ---
> mm/memory-failure.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 0c5ad7505b99..7023c3d81273 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -2591,8 +2591,6 @@ int soft_offline_page(unsigned long pfn, int flags)
> if (hwpoison_filter(page)) {
> if (ret > 0)
> put_page(page);
> - else
> - put_ref_page(ref_page);
>
> mutex_unlock(&mf_mutex);
> return -EOPNOTSUPP;
> --
> 2.23.0
On Thu, Aug 18, 2022 at 09:00:14PM +0800, Miaohe Lin wrote:
> After kill_procs(), tk will be freed without being removed from the to_kill
> list. In the next iteration, the freed list entry in the to_kill list will
> be accessed, thus leading to use-after-free issue.
kill_procs() runs over the to_kill list and frees all listed items in each
iteration. So just after returning from unmap_and_kill(), to_kill->next and
to_kill->prev still point to the addresses of struct to_kill which was the
first or last item (already freed!). This is bad-manered, but
collect_procs_fsdax() in the next iteration calls list_add_tail() and
overwrites the dangling pointers with newly allocated item. So this problem
should not be so critical? Anyway, I agree with fixing this fragile code.
> Fix it by reinitializing
> the to_kill list after unmap_and_kill().
>
> Fixes: c36e20249571 ("mm: introduce mf_dax_kill_procs() for fsdax case")
> Signed-off-by: Miaohe Lin <[email protected]>
> ---
> mm/memory-failure.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 7023c3d81273..a2f4e8b00a26 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1658,6 +1658,8 @@ int mf_dax_kill_procs(struct address_space *mapping, pgoff_t index,
> collect_procs_fsdax(page, mapping, index, &to_kill);
> unmap_and_kill(&to_kill, page_to_pfn(page), mapping,
> index, mf_flags);
> + /* Reinitialize to_kill list for later resuing. */
s/resuing/reusing/ ?
> + INIT_LIST_HEAD(&to_kill);
How about adding list_del() in kill_procs()? Other callers now use
to_kill only once, but fixing generally looks tidier to me.
Thanks,
Naoya Horiguchi
On Thu, Aug 18, 2022 at 09:00:16PM +0800, Miaohe Lin wrote:
> For reserved pages, HWPoison flag will be set without increasing the page
> refcnt. So we shouldn't even try to unpoison these pages and thus decrease
> the page refcnt unexpectly. Add a PageReserved() check to filter this case
> out and remove the below unneeded zero page (zero page is reserved) check.
>
> Signed-off-by: Miaohe Lin <[email protected]>
Looks good to me, thank you.
Acked-by: Naoya Horiguchi <[email protected]>
> ---
> mm/memory-failure.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 5f9615a86296..c831c41bb092 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -2355,7 +2355,7 @@ int unpoison_memory(unsigned long pfn)
> goto unlock_mutex;
> }
>
> - if (PageSlab(page) || PageTable(page))
> + if (PageSlab(page) || PageTable(page) || PageReserved(page))
> goto unlock_mutex;
>
> ret = get_hwpoison_page(p, MF_UNPOISON);
> @@ -2386,7 +2386,7 @@ int unpoison_memory(unsigned long pfn)
> freeit = !!TestClearPageHWPoison(p);
>
> put_page(page);
> - if (freeit && !(pfn == my_zero_pfn(0) && page_count(p) == 1)) {
> + if (freeit) {
> put_page(page);
> ret = 0;
> }
> --
> 2.23.0
On 2022/8/19 13:24, HORIGUCHI NAOYA(堀口 直也) wrote:
> On Thu, Aug 18, 2022 at 09:00:15PM +0800, Miaohe Lin wrote:
>> If try_to_unmap() fails, the hwpoisoned page still resides in the address
>> space of some processes. We should kill these processes or the hwpoisoned
>> page might be consumed later. collect_procs() is always called to collect
>> relevant processes now so they can be killed later if unmap fails.
>>
>> Signed-off-by: Miaohe Lin <[email protected]>
>> ---
>> mm/memory-failure.c | 9 ++++-----
>> 1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index a2f4e8b00a26..5f9615a86296 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -1396,7 +1396,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
>> struct address_space *mapping;
>> LIST_HEAD(tokill);
>> bool unmap_success;
>> - int kill = 1, forcekill;
>> + int forcekill;
>> bool mlocked = PageMlocked(hpage);
>>
>> /*
>> @@ -1437,7 +1437,6 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
>> if (page_mkclean(hpage)) {
>> SetPageDirty(hpage);
>> } else {
>> - kill = 0;
>> ttu |= TTU_IGNORE_HWPOISON;
>> pr_info("%#lx: corrupted page was clean: dropped without side effects\n",
>> pfn);
>> @@ -1452,8 +1451,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
>> * Error handling: We ignore errors here because
>> * there's nothing that can be done.
>
> This above comment might be deprecated now (I'm not sure what this really mean),
> so could you drop or update this?
Do you mean remove the below comment? In fact, this doesn't make much sense for me.
* Error handling: We ignore errors here because
* there's nothing that can be done.
>
> Anyway, the patch looks good to me.
>
> Acked-by: Naoya Horiguchi <[email protected]>
Many thanks for review.
Thanks,
Miaohe Lin
On 2022/8/19 13:23, HORIGUCHI NAOYA(堀口 直也) wrote:
> On Thu, Aug 18, 2022 at 09:00:14PM +0800, Miaohe Lin wrote:
>> After kill_procs(), tk will be freed without being removed from the to_kill
>> list. In the next iteration, the freed list entry in the to_kill list will
>> be accessed, thus leading to use-after-free issue.
>
> kill_procs() runs over the to_kill list and frees all listed items in each
> iteration. So just after returning from unmap_and_kill(), to_kill->next and
> to_kill->prev still point to the addresses of struct to_kill which was the
> first or last item (already freed!). This is bad-manered, but
> collect_procs_fsdax() in the next iteration calls list_add_tail() and
> overwrites the dangling pointers with newly allocated item. So this problem
list_add_tail will do WRITE_ONCE(prev->next, new) where prev is already freed!
Or am I miss something?
> should not be so critical? Anyway, I agree with fixing this fragile code.
>
>> Fix it by reinitializing
>> the to_kill list after unmap_and_kill().
>>
>> Fixes: c36e20249571 ("mm: introduce mf_dax_kill_procs() for fsdax case")
>> Signed-off-by: Miaohe Lin <[email protected]>
>
>> ---
>> mm/memory-failure.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index 7023c3d81273..a2f4e8b00a26 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -1658,6 +1658,8 @@ int mf_dax_kill_procs(struct address_space *mapping, pgoff_t index,
>> collect_procs_fsdax(page, mapping, index, &to_kill);
>> unmap_and_kill(&to_kill, page_to_pfn(page), mapping,
>> index, mf_flags);
>> + /* Reinitialize to_kill list for later resuing. */
>
> s/resuing/reusing/ ?
OK.
>
>> + INIT_LIST_HEAD(&to_kill);
>
> How about adding list_del() in kill_procs()? Other callers now use
> to_kill only once, but fixing generally looks tidier to me.
That's a good idea. Will do it in v2. Many thanks for your review, Naoya!
Thanks,
Miaohe Lin
>
> Thanks,
> Naoya Horiguchi
>
On Fri, Aug 19, 2022 at 03:32:27PM +0800, Miaohe Lin wrote:
> On 2022/8/19 13:23, HORIGUCHI NAOYA(堀口 直也) wrote:
> > On Thu, Aug 18, 2022 at 09:00:14PM +0800, Miaohe Lin wrote:
> >> After kill_procs(), tk will be freed without being removed from the to_kill
> >> list. In the next iteration, the freed list entry in the to_kill list will
> >> be accessed, thus leading to use-after-free issue.
> >
> > kill_procs() runs over the to_kill list and frees all listed items in each
> > iteration. So just after returning from unmap_and_kill(), to_kill->next and
> > to_kill->prev still point to the addresses of struct to_kill which was the
> > first or last item (already freed!). This is bad-manered, but
> > collect_procs_fsdax() in the next iteration calls list_add_tail() and
> > overwrites the dangling pointers with newly allocated item. So this problem
>
> list_add_tail will do WRITE_ONCE(prev->next, new) where prev is already freed!
> Or am I miss something?
No, you're right. Thank you for explanation.
Thanks,
Naoya Horiguchi
On Fri, Aug 19, 2022 at 03:37:23PM +0800, Miaohe Lin wrote:
> On 2022/8/19 13:24, HORIGUCHI NAOYA(堀口 直也) wrote:
> > On Thu, Aug 18, 2022 at 09:00:15PM +0800, Miaohe Lin wrote:
> >> If try_to_unmap() fails, the hwpoisoned page still resides in the address
> >> space of some processes. We should kill these processes or the hwpoisoned
> >> page might be consumed later. collect_procs() is always called to collect
> >> relevant processes now so they can be killed later if unmap fails.
> >>
> >> Signed-off-by: Miaohe Lin <[email protected]>
> >> ---
> >> mm/memory-failure.c | 9 ++++-----
> >> 1 file changed, 4 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> >> index a2f4e8b00a26..5f9615a86296 100644
> >> --- a/mm/memory-failure.c
> >> +++ b/mm/memory-failure.c
> >> @@ -1396,7 +1396,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
> >> struct address_space *mapping;
> >> LIST_HEAD(tokill);
> >> bool unmap_success;
> >> - int kill = 1, forcekill;
> >> + int forcekill;
> >> bool mlocked = PageMlocked(hpage);
> >>
> >> /*
> >> @@ -1437,7 +1437,6 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
> >> if (page_mkclean(hpage)) {
> >> SetPageDirty(hpage);
> >> } else {
> >> - kill = 0;
> >> ttu |= TTU_IGNORE_HWPOISON;
> >> pr_info("%#lx: corrupted page was clean: dropped without side effects\n",
> >> pfn);
> >> @@ -1452,8 +1451,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
> >> * Error handling: We ignore errors here because
> >> * there's nothing that can be done.
> >
> > This above comment might be deprecated now (I'm not sure what this really mean),
> > so could you drop or update this?
>
> Do you mean remove the below comment? In fact, this doesn't make much sense for me.
>
> * Error handling: We ignore errors here because
> * there's nothing that can be done.
Yes, that's what I meant.
Thanks,
Naoya Horiguchi