linux-kernel - Re: [PATCH 1/2] mm: change free_cma and free

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: <20150929101327.GW25655@suse.de>
Date:	Tue, 29 Sep 2015 11:13:27 +0100
From:	Mel Gorman <mgorman@...e.de>
To:	Alexandru Moise <00moses.alexander00@...il.com>
Cc:	akpm@...ux-foundation.org, vbabka@...e.cz, mhocko@...e.com,
	rientjes@...gle.com, js1304@...il.com, hannes@...xchg.org,
	alexander.h.duyck@...hat.com, sasha.levin@...cle.com,
	linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] mm: change free_cma and free_pages declarations to
 unsigned

On Sun, Sep 27, 2015 at 09:04:16PM +0000, Alexandru Moise wrote:
> Their stored values come from zone_page_state() which returns
> an unsigned long. To improve code correctness we should avoid
> mixing signed and unsigned integers.
> 
> Signed-off-by: Alexandru Moise <00moses.alexander00@...il.com>
> ---
>  mm/page_alloc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 48aaf7b..f55e3a2 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2242,7 +2242,7 @@ static bool __zone_watermark_ok(struct zone *z, unsigned int order,
>  	/* free_pages may go negative - that's OK */
>  	long min = mark;
>  	int o;
> -	long free_cma = 0;
> +	unsigned long free_cma = 0;
>  

NAK.

free_cma is used with free_pages which is explicitly commented as saying
it can go negative. With your patch, there is a signed/unsigned operation
where the unsigned type cannot fit into the signed type which casts them
both to unsigned which is then broken for the comparison.  This patch
looks broken for very subtle reasons. Please do not do any similar style
patches to this because they can introduce subtle breakage if issues are
not caught at review.

-- 
Mel Gorman
SUSE Labs
--
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/