linux-kernel - Re: [PATCH 1/4] mm/memory_hotplug: Make add_memory_resource use __try_online

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: <20180620151819.3f39226998bd80f7161fcea5@linux-foundation.org>
Date:   Wed, 20 Jun 2018 15:18:19 -0700
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     osalvador@...hadventures.net
Cc:     mhocko@...e.com, vbabka@...e.cz, pasha.tatashin@...cle.com,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Oscar Salvador <osalvador@...e.de>
Subject: Re: [PATCH 1/4] mm/memory_hotplug: Make add_memory_resource use
 __try_online_node

On Fri,  1 Jun 2018 14:53:18 +0200 osalvador@...hadventures.net wrote:

> From: Oscar Salvador <osalvador@...e.de>
> 
> add_memory_resource() contains code to allocate a new node in case
> it is necessary.
> Since try_online_node() also hast some code for this purpose,
> let us make use of that and remove duplicate code.
> 
> This introduces __try_online_node(), which is called by add_memory_resource()
> and try_online_node().
> __try_online_node() has two new parameters, start_addr of the node,
> and if the node should be onlined and registered right away.
> This is always wanted if we are calling from do_cpu_up(), but not
> when we are calling from memhotplug code.
> Nothing changes from the point of view of the users of try_online_node(),
> since try_online_node passes start_addr=0 and online_node=true to
> __try_online_node().
> 
> ...
>
> @@ -1126,17 +1136,14 @@ int __ref add_memory_resource(int nid, struct resource *res, bool online)
>  	 */
>  	memblock_add_node(start, size, nid);
>  
> -	new_node = !node_online(nid);
> -	if (new_node) {
> -		pgdat = hotadd_new_pgdat(nid, start);
> -		ret = -ENOMEM;
> -		if (!pgdat)
> -			goto error;
> -	}
> +	ret = __try_online_node (nid, start, false);
> +	new_node = !!(ret > 0);

I don't think __try_online_node() will ever return a value greater than
zero.  I assume what was meant was

	new_node = !!(ret >= 0);

which may as well be

	new_node = (ret >= 0);

since both sides have bool type.

The fact that testing didn't detect this is worrisome....

> +	if (ret < 0)
> +		goto error;
> +
>  
>  	/* call arch's memory hotadd */
>  	ret = arch_add_memory(nid, start, size, NULL, true);
> -
>  	if (ret < 0)
>  		goto error;
>  
> 
> ...
>