linux-kernel - Re: [PATCH] oom: fix integer overflow of points in oom

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, 31 Oct 2011 16:49:42 +0100
From:	Frantisek Hrbata <fhrbata@...hat.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	rientjes@...gle.com, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
	kosaki.motohiro@...fujitsu.com, minchan.kim@...il.com,
	stable@...nel.org, eteo@...hat.com, pmatouse@...hat.com
Subject: Re: [PATCH] oom: fix integer overflow of points in oom_badness

On Mon, Oct 31, 2011 at 04:28:33PM +0100, Oleg Nesterov wrote:
> On 10/31, Frantisek Hrbata wrote:
> >
> > My understanding is that we may just change the type of points variable from int
> > to long and keep the current imho clearer(better readable) computation. There
> > should not be an overflow on 32bit and there is a plenty of space for 64bit.
> > If you like this solution better I will post the patch as v2.
> 
> Up to maintainer, but personally I think the simple s/int/long/ looks better.
> Everything like get_mm_*/nr_ptes returns long.

Agreed. I will post v2 with the int => long change.

> 
> Anyway good catch. Imho stable needs the fix too.
> 
> Cosmetic nit,
> 
> > -	points = get_mm_rss(p->mm) + p->mm->nr_ptes;
> > -	points += get_mm_counter(p->mm, MM_SWAPENTS);
> > +	points = (int)((get_mm_rss(p->mm) + get_mm_counter(p->mm, MM_SWAPENTS) +
>                  ^^^^^
> 
> Why do we need the explicit typecast? It buys nothing and looks a bit confusing.

You are right, it's not needed. I just wanted to make the cast more visible, but
some kind of comment would be probably better here.

> 
> And, if you prefer "int", perhaps something like
> 
> 	- 	points *= 1000;
> 	-	points /= totalpages;
> 	+	/* avoid the possible overflow */
> 	+	points = points * 1000L / totalpages;
> 
> looks a bit more readable with the same effect. But I won't insist, this is
> up to you and David.

Sure, this looks much better than the one line expression in the patch I sent.
If David or others decide to not go with the int=>long change I think we should
use this.

> 
> Oleg.
> 

Many thanks Oleg.

-- 
Frantisek Hrbata
--
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/