linux-kernel - Re: [PATCH v3 4/4] mm/sparse: Optimize memmap allocation during sparse

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]

Date:   Fri, 6 Apr 2018 07:50:18 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Baoquan He <bhe@...hat.com>, linux-kernel@...r.kernel.org,
        akpm@...ux-foundation.org, pagupta@...hat.com
Cc:     linux-mm@...ck.org, kirill.shutemov@...ux.intel.com
Subject: Re: [PATCH v3 4/4] mm/sparse: Optimize memmap allocation during
 sparse_init()

I'm having a really hard time tying all the pieces back together.  Let
me give it a shot and you can tell me where I go wrong.

On 02/27/2018 07:26 PM, Baoquan He wrote:
> In sparse_init(), two temporary pointer arrays, usemap_map and map_map
> are allocated with the size of NR_MEM_SECTIONS.

In sparse_init(), two temporary pointer arrays, usemap_map and map_map
are allocated to hold the maps for every possible memory section
(NR_MEM_SECTIONS).  However, we obviously only need the array sized for
nr_present_sections (introduced in patch 1).

The reason this is a problem is that, with 5-level paging,
NR_MEM_SECTIONS (8M->512M) went up dramatically and these temporary
arrays can eat all of memory, like on kdump kernels.

This patch does two things: it makes sure to give usemap_map/mem_map a
less gluttonous size on small systems, and it changes the map allocation
and handling to handle the now more compact, less sparse arrays.

---

The code looks fine to me.  It's a bit of a shame that there's no
verification to ensure that idx_present never goes beyond the shiny new
nr_present_sections.

> @@ -583,6 +592,7 @@ void __init sparse_init(void)
>  	unsigned long *usemap;
>  	unsigned long **usemap_map;
>  	int size;
> +	int idx_present = 0;

I wonder whether idx_present is a good name.  Isn't it the number of
consumed mem_map[]s or usemaps?

> 
>  		if (!map) {
>  			ms->section_mem_map = 0;
> +			idx_present++;
>  			continue;
>  		}
>  

This hunk seems logically odd to me.  I would expect a non-used section
to *not* consume an entry from the temporary array.  Why does it?  The
error and success paths seem to do the same thing.