linux-kernel - Re: [PATCH v1 1/4] mm/memory_hotplug: Release memory resource after arch_remove

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1555501962.3139.10.camel@suse.de>
Date:   Wed, 17 Apr 2019 13:52:42 +0200
From:   Oscar Salvador <osalvador@...e.de>
To:     David Hildenbrand <david@...hat.com>, linux-mm@...ck.org
Cc:     linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Michal Hocko <mhocko@...e.com>,
        Pavel Tatashin <pasha.tatashin@...een.com>,
        Wei Yang <richard.weiyang@...il.com>, Qian Cai <cai@....pw>,
        Arun KS <arunks@...eaurora.org>,
        Mathieu Malaterre <malat@...ian.org>
Subject: Re: [PATCH v1 1/4] mm/memory_hotplug: Release memory resource after
 arch_remove_memory()

On Tue, 2019-04-09 at 12:01 +0200, David Hildenbrand wrote:
> __add_pages() doesn't add the memory resource, so __remove_pages()
> shouldn't remove it. Let's factor it out. Especially as it is a
> special
> case for memory used as system memory, added via add_memory() and
> friends.

I would call the special case the other way, aka: zone_device hooking
into hotplug path.

> 
> We now remove the resource after removing the sections instead of
> doing
> it the other way around. I don't think this change is problematic.
> 
> add_memory()
> 	register memory resource
> 	arch_add_memory()
> 
> remove_memory
> 	arch_remove_memory()
> 	release memory resource
> 
> While at it, explain why we ignore errors and that it only happeny if
> we remove memory in a different granularity as we added it.

In the future we may want to allow drivers to hook directly into
arch_add_memory()/arch_remove_memory(), and this will lead to different
granularity in hot_add/hot_remove operations. 

At least that was one of the conclusions I drew from the last vmemmap-
patchset.
So, we will have to see how we can handle those kind of errors.

> 
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Oscar Salvador <osalvador@...e.de>
> Cc: Michal Hocko <mhocko@...e.com>
> Cc: David Hildenbrand <david@...hat.com>
> Cc: Pavel Tatashin <pasha.tatashin@...een.com>
> Cc: Wei Yang <richard.weiyang@...il.com>
> Cc: Qian Cai <cai@....pw>
> Cc: Arun KS <arunks@...eaurora.org>
> Cc: Mathieu Malaterre <malat@...ian.org>
> Signed-off-by: David Hildenbrand <david@...hat.com>

Besides what Andrew pointed out about the types of start,size, I do not
see anything wrong:

Reviewed-by: Oscar Salvador <osalvador@...e.de>

> ---
>  mm/memory_hotplug.c | 34 ++++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 4970ff658055..696ed7ee5e28 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -562,20 +562,6 @@ int __remove_pages(struct zone *zone, unsigned
> long phys_start_pfn,
>  	if (is_dev_zone(zone)) {
>  		if (altmap)
>  			map_offset = vmem_altmap_offset(altmap);
> -	} else {
> -		resource_size_t start, size;
> -
> -		start = phys_start_pfn << PAGE_SHIFT;
> -		size = nr_pages * PAGE_SIZE;
> -
> -		ret = release_mem_region_adjustable(&iomem_resource,
> start,
> -					size);
> -		if (ret) {
> -			resource_size_t endres = start + size - 1;
> -
> -			pr_warn("Unable to release resource <%pa-
> %pa> (%d)\n",
> -					&start, &endres, ret);
> -		}
>  	}
>  
>  	clear_zone_contiguous(zone);
> @@ -1820,6 +1806,25 @@ void try_offline_node(int nid)
>  }
>  EXPORT_SYMBOL(try_offline_node);
>  
> +static void __release_memory_resource(u64 start, u64 size)
> +{
> +	int ret;
> +
> +	/*
> +	 * When removing memory in the same granularity as it was
> added,
> +	 * this function never fails. It might only fail if
> resources
> +	 * have to be adjusted or split. We'll ignore the error, as
> +	 * removing of memory cannot fail.
> +	 */
> +	ret = release_mem_region_adjustable(&iomem_resource, start,
> size);
> +	if (ret) {
> +		resource_size_t endres = start + size - 1;
> +
> +		pr_warn("Unable to release resource <%pa-%pa>
> (%d)\n",
> +			&start, &endres, ret);
> +	}
> +}
> +
>  /**
>   * remove_memory
>   * @nid: the node ID
> @@ -1854,6 +1859,7 @@ void __ref __remove_memory(int nid, u64 start,
> u64 size)
>  	memblock_remove(start, size);
>  
>  	arch_remove_memory(nid, start, size, NULL);
> +	__release_memory_resource(start, size);
>  
>  	try_offline_node(nid);
>  
-- 
Oscar Salvador
SUSE L3