| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sat, 15 Oct 2022 10:28:00 +0800
From: Miaohe Lin <linmiaohe@...wei.com>
To: Naoya Horiguchi <naoya.horiguchi@...ux.dev>, <linux-mm@...ck.org>
CC: Andrew Morton <akpm@...ux-foundation.org>,
David Hildenbrand <david@...hat.com>,
Mike Kravetz <mike.kravetz@...cle.com>,
Yang Shi <shy828301@...il.com>,
Oscar Salvador <osalvador@...e.de>,
Muchun Song <songmuchun@...edance.com>,
Jane Chu <jane.chu@...cle.com>,
Naoya Horiguchi <naoya.horiguchi@....com>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 4/4] mm/hwpoison: introduce per-memory_block hwpoison
counter
On 2022/10/7 9:07, Naoya Horiguchi wrote:
> From: Naoya Horiguchi <naoya.horiguchi@....com>
>
> Currently PageHWPoison flag does not behave well when experiencing memory
> hotremove/hotplug. Any data field in struct page is unreliable when the
> associated memory is offlined, and the current mechanism can't tell whether
> a memory block is onlined because a new memory devices is installed or
> because previous failed offline operations are undone. Especially if
> there's a hwpoisoned memory, it's unclear what the best option is.
>
> So introduce a new mechanism to make struct memory_block remember that
> a memory block has hwpoisoned memory inside it. And make any online event
> fail if the onlining memory block contains hwpoison. struct memory_block
> is freed and reallocated over ACPI-based hotremove/hotplug, but not over
> sysfs-based hotremove/hotplug. So the new counter can distinguish these
> cases.
>
> Signed-off-by: Naoya Horiguchi <naoya.horiguchi@....com>
> Reported-by: kernel test robot <lkp@...el.com>
> ---
> ChangeLog v5 -> v6:
> - fix build errors over memblk_nr_poison_inc() and memblk_nr_poison_sub(),
> - pass "struct memory_block *" to memblk_nr_poison() instead of pfn,
> - removed clear_hwpoisoned_pages() and call num_poisoned_pages_sub() directly.
> - add static keyword to the definition of memblk_nr_poison().
> - Mioahe added Reviewed-by for v5, but I have some non trivial changes in
> v6, so let me hold to add it.
> - unpoison_memory() properly cancels per-memblk hwpoison counter.
>
> ChangeLog v4 -> v5:
> - add Reported-by of lkp bot,
> - check both CONFIG_MEMORY_FAILURE and CONFIG_MEMORY_HOTPLUG in introduced #ifdefs,
> intending to fix "undefined reference" errors in aarch64.
>
> ChangeLog v3 -> v4:
> - fix build error (https://lore.kernel.org/linux-mm/202209231134.tnhKHRfg-lkp@intel.com/)
> by using memblk_nr_poison() to access to the member ->nr_hwpoison
> ---
> drivers/base/memory.c | 40 ++++++++++++++++++++++++++++++++++++++++
> include/linux/memory.h | 3 +++
> include/linux/mm.h | 18 ++++++++++++++++++
> mm/internal.h | 8 --------
> mm/memory-failure.c | 36 +++++++++++-------------------------
> mm/sparse.c | 2 --
> 6 files changed, 72 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index 9aa0da991cfb..5d00d8a14c79 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -175,6 +175,17 @@ int memory_notify(unsigned long val, void *v)
> return blocking_notifier_call_chain(&memory_chain, val, v);
> }
>
> +#if defined(CONFIG_MEMORY_FAILURE) && defined(CONFIG_MEMORY_HOTPLUG)
> +void memblk_nr_poison_inc(unsigned long pfn);
> +void memblk_nr_poison_sub(unsigned long pfn, long i);
> +static unsigned long memblk_nr_poison(struct memory_block *mem);
> +#else
> +static inline unsigned long memblk_nr_poison(struct memory_block *mem)
> +{
> + return 0;
> +}
> +#endif
> +
> static int memory_block_online(struct memory_block *mem)
> {
> unsigned long start_pfn = section_nr_to_pfn(mem->start_section_nr);
> @@ -183,6 +194,9 @@ static int memory_block_online(struct memory_block *mem)
> struct zone *zone;
> int ret;
>
> + if (memblk_nr_poison(mem))
> + return -EHWPOISON;
> +
> zone = zone_for_pfn_range(mem->online_type, mem->nid, mem->group,
> start_pfn, nr_pages);
>
> @@ -864,6 +878,7 @@ void remove_memory_block_devices(unsigned long start, unsigned long size)
> mem = find_memory_block_by_id(block_id);
> if (WARN_ON_ONCE(!mem))
> continue;
> + num_poisoned_pages_sub(-1UL, memblk_nr_poison(mem));
> unregister_memory_block_under_nodes(mem);
> remove_memory_block(mem);
> }
> @@ -1164,3 +1179,28 @@ int walk_dynamic_memory_groups(int nid, walk_memory_groups_func_t func,
> }
> return ret;
> }
> +
> +#if defined(CONFIG_MEMORY_FAILURE) && defined(CONFIG_MEMORY_HOTPLUG)
> +void memblk_nr_poison_inc(unsigned long pfn)
> +{
> + const unsigned long block_id = pfn_to_block_id(pfn);
> + struct memory_block *mem = find_memory_block_by_id(block_id);
> +
> + if (mem)
> + atomic_long_inc(&mem->nr_hwpoison);
> +}
> +
> +void memblk_nr_poison_sub(unsigned long pfn, long i)
> +{
> + const unsigned long block_id = pfn_to_block_id(pfn);
> + struct memory_block *mem = find_memory_block_by_id(block_id);
> +
> + if (mem)
> + atomic_long_sub(i, &mem->nr_hwpoison);
> +}
> +
> +static unsigned long memblk_nr_poison(struct memory_block *mem)
> +{
> + return atomic_long_read(&mem->nr_hwpoison);
> +}
> +#endif
> diff --git a/include/linux/memory.h b/include/linux/memory.h
> index aa619464a1df..ad8cd9bb3239 100644
> --- a/include/linux/memory.h
> +++ b/include/linux/memory.h
> @@ -85,6 +85,9 @@ struct memory_block {
> unsigned long nr_vmemmap_pages;
> struct memory_group *group; /* group (if any) for this block */
> struct list_head group_next; /* next block inside memory group */
> +#if defined(CONFIG_MEMORY_FAILURE) && defined(CONFIG_MEMORY_HOTPLUG)
> + atomic_long_t nr_hwpoison;
> +#endif
> };
>
> int arch_get_memory_phys_device(unsigned long start_pfn);
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 17119dbf8fad..f80269e90772 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3280,6 +3280,7 @@ extern int soft_offline_page(unsigned long pfn, int flags);
> extern int __get_huge_page_for_hwpoison(unsigned long pfn, int flags,
> bool *migratable_cleared);
> extern void num_poisoned_pages_inc(unsigned long pfn);
> +extern void num_poisoned_pages_sub(unsigned long pfn, long i);
The prototype of this function is: *inline* void num_poisoned_pages_sub(unsigned long pfn, long i).
The combination of inline and extern looks weird to me. Is this a common use case?
Anyway, this patch looks good to me. Thanks.
Reviewed-by: Miaohe Lin <linmiaohe@...wei.com>
Thanks,
Miaohe Lin
Powered by blists - more mailing lists