linux-kernel - Re: [PATCH] x86, mm: clean up setup_node

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] [day] [month] [year] [list]

Date:	Thu, 3 Mar 2011 14:04:23 -0800 (PST)
From:	David Rientjes <rientjes@...gle.com>
To:	Yinghai Lu <yinghai@...nel.org>
cc:	Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...e.hu>,
	"H. Peter Anvin" <hpa@...or.com>, Tejun Heo <tj@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86, mm: clean up setup_node_bootmem

On Thu, 3 Mar 2011, Yinghai Lu wrote:

> >> Index: linux-2.6/arch/x86/mm/numa_64.c
> >> ===================================================================
> >> --- linux-2.6.orig/arch/x86/mm/numa_64.c
> >> +++ linux-2.6/arch/x86/mm/numa_64.c
> >> @@ -237,21 +237,18 @@ int __init numa_add_memblk(int nid, u64
> >>  }
> >>  
> >>  /* Initialize bootmem allocator for a node */
> >> -void __init
> >> +static void __init
> >>  setup_node_bootmem(int nodeid, unsigned long start, unsigned long end)
> >>  {
> >>  	unsigned long start_pfn, last_pfn, nodedata_phys;
> >>  	const int pgdat_size = roundup(sizeof(pg_data_t), PAGE_SIZE);
> >>  	int nid;
> >>  
> >> -	if (!end)
> >> -		return;
> >> -
> >>  	/*
> >>  	 * Don't confuse VM with a node that doesn't have the
> >>  	 * minimum amount of memory:
> >>  	 */
> >> -	if (end && (end - start) < NODE_MIN_SIZE)
> >> +	if (end < (start +  NODE_MIN_SIZE))
> >>  		return;
> >>  
> >>  	start = roundup(start, ZONE_ALIGN);
> >> @@ -557,8 +554,7 @@ static int __init numa_register_memblks(
> >>  			end = max(mi->blk[i].end, end);
> >>  		}
> >>  
> >> -		if (start < end)
> >> -			setup_node_bootmem(nid, start, end);
> >> +		setup_node_bootmem(nid, start, end);
> >>  	}
> >>  
> >>  	return 0;
> >>
> > 
> > Good catch on finding only the one caller of setup_node_bootmem().
> > 
> > I'd actually rather see the validity checking being done in 
> > numa_register_memblks(), though, because it's a bug for a node to be set 
> > in node_possible_map without having a valid 
> > [mi->blk[i].start, mi->blk[i].end) range.
> > 
> > So could this be
> > 
> > 	BUG_ON(end < start);
> 
> no, it could cause crash here
> 
>         /* Finally register nodes. */
>         for_each_node_mask(nid, node_possible_map) {
>                 u64 start = (u64)max_pfn << PAGE_SHIFT;
>                 u64 end = 0;
> 
>                 for (i = 0; i < mi->nr_blks; i++) {
>                         if (nid != mi->blk[i].nid)
>                                 continue;
>                         start = min(mi->blk[i].start, start);
>                         end = max(mi->blk[i].end, end);
>                 }
> 
> could have some node without memory. and it could be set in node_possible_map, and it will have end = 0, and start max_pfn<<page_shift.
> 

Ok, I didn't realize this was being used for memoryless nodes.

> > 	/*
> > 	 * Don't confuse the VM with a node that doesn't have the minimum
> > 	 * amount of memory.
> > 	 */
> > 	if (end < start + NODE_MIN_SIZE) {
> > 		node_clear(nid, node_possible_map);
> 
> why touch that possible_map ?
> online_map is for that purpose that state node have ram.
> 

Hmm, that's the purpose of N_NORMAL_MEMORY, not node_online_map.  I 
understand why we don't want to register bootmem for a node that has such 
little memory that we disregard it, but shouldn't we then consider it to 
no longer be possible?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/