linux-kernel - Re: [PATCH] mm: fix maybe-uninitialized warning in section

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:   Mon, 23 Jan 2017 14:38:27 -0800
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Dan Williams <dan.j.williams@...el.com>,
        Yasuaki Ishimatsu <yasu.isimatu@...il.com>,
        Fabian Frederick <fabf@...net.be>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm: fix maybe-uninitialized warning in
 section_deactivate()

On Mon, 23 Jan 2017 17:51:17 +0100 Arnd Bergmann <arnd@...db.de> wrote:

> gcc cannot track the combined state of the 'mask' variable across the
> barrier in pgdat_resize_unlock() at compile time, so it warns that we
> can run into undefined behavior:
> 
> mm/sparse.c: In function 'section_deactivate':
> mm/sparse.c:802:7: error: 'early_section' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> 
> We know that this can't happen because the spin_unlock() doesn't
> affect the mask variable, so this is a false-postive warning, but
> rearranging the code to bail out earlier here makes it obvious
> to the compiler as well.
> 
> ...
>
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -807,23 +807,24 @@ static void section_deactivate(struct pglist_data *pgdat, unsigned long pfn,
>  	unsigned long mask = section_active_mask(pfn, nr_pages), flags;
>  
>  	pgdat_resize_lock(pgdat, &flags);
> -	if (!ms->usage) {
> -		mask = 0;
> -	} else if ((ms->usage->map_active & mask) != mask) {
> -		WARN(1, "section already deactivated active: %#lx mask: %#lx\n",
> -				ms->usage->map_active, mask);
> -		mask = 0;
> -	} else {
> -		early_section = is_early_section(ms);
> -		ms->usage->map_active ^= mask;
> -		if (ms->usage->map_active == 0) {
> -			usage = ms->usage;
> -			ms->usage = NULL;
> -			memmap = sparse_decode_mem_map(ms->section_mem_map,
> -					section_nr);
> -			ms->section_mem_map = 0;
> -		}
> +	if (!ms->usage ||
> +	    WARN((ms->usage->map_active & mask) != mask,
> +		 "section already deactivated active: %#lx mask: %#lx\n",
> +			ms->usage->map_active, mask)) {
> +		pgdat_resize_unlock(pgdat, &flags);
> +		return;
>  	}
> +
> +	early_section = is_early_section(ms);
> +	ms->usage->map_active ^= mask;
> +	if (ms->usage->map_active == 0) {
> +		usage = ms->usage;
> +		ms->usage = NULL;
> +		memmap = sparse_decode_mem_map(ms->section_mem_map,
> +				section_nr);
> +		ms->section_mem_map = 0;
> +	}
> +

hm, OK, that looks equivalent.

I wonder if we still need the later

	if (!mask)
		return;

I wonder if this code is appropriately handling the `mask == -1' case. 
section_active_mask() can do that.

What does that -1 in section_active_mask() mean anyway?  Was it really
intended to represent the all-ones pattern or is it an error?  If the
latter, was it appropriate for section_active_mask() to return an
unsigned type?

How come section_active_mask() is __init but its caller
section_deactivate() is not?