On 4/8/22 06:53, Naoya Horiguchi wrote:
> From: Naoya Horiguchi <naoya.horiguchi@xxxxxxx>
>
> There is a race condition between memory_failure_hugetlb() and hugetlb
> free/demotion, which causes setting PageHWPoison flag on the wrong page.
> The one simple result is that wrong processes can be killed, but another
> (more serious) one is that the actual error is left unhandled, so no one
> prevents later access to it, and that might lead to more serious results
> like consuming corrupted data.
>
> Think about the below race window:
>
> CPU 1 CPU 2
> memory_failure_hugetlb
> struct page *head = compound_head(p);
> hugetlb page might be freed to
> buddy, or even changed to another
> compound page.
>
> get_hwpoison_page -- page is not what we want now...
>
> The current code first does prechecks roughly and then reconfirms
> after taking refcount, but it's found that it makes code overly
> complicated, so move the prechecks in a single hugetlb_lock range.
>
> A newly introduced function, try_memory_failure_hugetlb(), always
> takes hugetlb_lock (even for non-hugetlb pages). That can be
> improved, but memory_failure() is rare in principle, so should
> not be a big problem.
Thanks!
I think this approach is the safest and makes the code easier to understand.
>
> Fixes: 761ad8d7c7b5 ("mm: hwpoison: introduce memory_failure_hugetlb()")
> Reported-by: Mike Kravetz <mike.kravetz@xxxxxxxxxx>
> Signed-off-by: Naoya Horiguchi <naoya.horiguchi@xxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> ---
...
> ---
> include/linux/hugetlb.h | 6 ++
> include/linux/mm.h | 8 +++
> mm/hugetlb.c | 10 +++
> mm/memory-failure.c | 145 ++++++++++++++++++++++++++++------------
> 4 files changed, 127 insertions(+), 42 deletions(-)
>
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 53c1b6082a4c..ac2a1d758a80 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -169,6 +169,7 @@ long hugetlb_unreserve_pages(struct inode *inode, long start, long end,
> long freed);
> bool isolate_huge_page(struct page *page, struct list_head *list);
> int get_hwpoison_huge_page(struct page *page, bool *hugetlb);
> +int get_huge_page_for_hwpoison(unsigned long pfn, int flags);
> void putback_active_hugepage(struct page *page);
> void move_hugetlb_state(struct page *oldpage, struct page *newpage, int reason);
> void free_huge_page(struct page *page);
> @@ -378,6 +379,11 @@ static inline int get_hwpoison_huge_page(struct page *page, bool *hugetlb)
> return 0;
> }
>
> +static inline int get_huge_page_for_hwpoison(unsigned long pfn, int flags)
> +{
> + return 0;
> +}
> +
> static inline void putback_active_hugepage(struct page *page)
> {
> }
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index e34edb775334..9f44254af8ce 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3197,6 +3197,14 @@ extern int sysctl_memory_failure_recovery;
> extern void shake_page(struct page *p);
> extern atomic_long_t num_poisoned_pages __read_mostly;
> extern int soft_offline_page(unsigned long pfn, int flags);
> +#ifdef CONFIG_MEMORY_FAILURE
> +extern int __get_huge_page_for_hwpoison(unsigned long pfn, int flags);
> +#else
> +static inline int __get_huge_page_for_hwpoison(unsigned long pfn, int flags)
> +{
> + return 0;
> +}
> +#endif
>
> #ifndef arch_memory_failure
> static inline int arch_memory_failure(unsigned long pfn, int flags)
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index f8ca7cca3c1a..3fc721789743 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -6785,6 +6785,16 @@ int get_hwpoison_huge_page(struct page *page, bool *hugetlb)
> return ret;
> }
>
> +int get_huge_page_for_hwpoison(unsigned long pfn, int flags)
> +{
> + int ret;
> +
> + spin_lock_irq(&hugetlb_lock);
> + ret = __get_huge_page_for_hwpoison(pfn, flags);
> + spin_unlock_irq(&hugetlb_lock);
> + return ret;
> +}
> +
> void putback_active_hugepage(struct page *page)
> {
> spin_lock_irq(&hugetlb_lock);
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index dcb6bb9cf731..2020944398c9 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1498,50 +1498,113 @@ static int try_to_split_thp_page(struct page *page, const char *msg)
> return 0;
> }
>
> -static int memory_failure_hugetlb(unsigned long pfn, int flags)
> +/*
> + * Called from hugetlb code with hugetlb_lock held.
> + *
> + * Return values:
> + * 0 - free hugepage
> + * 1 - in-use hugepage
> + * 2 - not a hugepage
> + * -EBUSY - the hugepage is busy (try to retry)
> + * -EHWPOISON - the hugepage is already hwpoisoned
> + */
> +int __get_huge_page_for_hwpoison(unsigned long pfn, int flags)
> +{
> + struct page *page = pfn_to_page(pfn);
> + struct page *head = compound_head(page);
> + int ret = 2; /* fallback to normal page handling */
> + bool count_increased = false;
> +
> + if (!PageHeadHuge(head))
> + goto out;
> +
> + if (flags & MF_COUNT_INCREASED) {
> + ret = 1;
> + count_increased = true;
> + } else if (HPageFreed(head) || HPageMigratable(head)) {
> + ret = get_page_unless_zero(head);
> + if (ret)
> + count_increased = true;
The above code works as designed, but may be a bit confusing. If HPageFreed()
we KNOW ref count is zero, so no need to even call get_page_unless_zero() as
it will always return false in this case. It might be more clear if written
as separate else if statements such as:
} else if (HPageFreed(head)) {
ret = 0;
} else if (HPageMigratable(head)) {
ret = get_page_unless_zero(head);
if (ret)
count_increased = true;
Not insisting this be changed. Just easier to understand IMO.
Again, thanks for your work on this!
Reviewed-by: Mike Kravetz <mike.kravetz@xxxxxxxxxx>
--
Mike Kravetz
