Re: [PATCH] [RFC] brk randomization: compute RLIMIT_DATA properly (was Re: x86: randomize brk() and RLIMIT_DATA)

[Hugh Dickins <hugh@veritas.com>]

On Fri, 26 Oct 2007, Jiri Kosina wrote:
> On Thu, 25 Oct 2007, Sami Farin wrote:
> > > > > Would be neat if randomized brk and setrlimit(RLIMIT_DATA, ...)
> > > > > worked in a predictable way:
> > > > this isn't a valid case afaics; even on "traditional x86" (before we 
> > > > changed the address space layout, or even today if you have an 
> > > > unlimited stack rlimit) this isn't going to work. applications 
> > > > really shouldn't use (s)brk() but malloc(); you have to be able to 
> > > > fall back to mmap regardless of what you do.
> > > I tend to agree here with Arjan. However it probably would make no 
> > > harm to make at least a little bit consisten behavior of setrlimit(), 
> > > though it has a little use in such cases.
> > > Sami, does the patch below work for you?
> > Thanks, Jiri, now RLIMIT_DATA works as expected. Using only RLIMIT_AS to 
> > limit processes' memory usage is not very easy.  It includes also 
> > libraries mapped read-only, I have to check/modify the limits when I 
> > update/add libraries,...
> 
> This patch is [RFC] as I am not sure whether it is worth it (see Arjan's 
> comment above).

It is worth it.

Arjan's right that RLIMIT_DATA has difficulty making sense, but
you shouldn't be randomizing its breakage further with your patch.

> 
> 
> From: Jiri Kosina <jkosina@suse.cz>
> 
> brk randomization: compute RLIMIT_DATA properly
> 
> In cases of heap area placed at randomly-generated offset from 
> mm->end_data (arch_randomize_brk()), we need to subtract the value of the 
> offset for setrlimit(RLIMIT_DATA) to work properly -- otherwise we count 
> the unoccupied memory between mm->end_data and mm->start_brk as occupied.
> 
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index facc1a7..c7ade18 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -251,7 +251,8 @@ asmlinkage unsigned long sys_brk(unsigned long brk)
>  	 * not page aligned -Ram Gupta
>  	 */
>  	rlim = current->signal->rlim[RLIMIT_DATA].rlim_cur;
> -	if (rlim < RLIM_INFINITY && brk - mm->start_data > rlim)
> +	if (rlim < RLIM_INFINITY && brk - mm->start_data -
> +			(mm->start_brk - mm->end_data) > rlim)
>  		goto out;
>  
>  	newbrk = PAGE_ALIGN(brk);

I find the order in that test mysterious.  Others might ask you to use
an intermediate variable, I don't care about that, but I would find

if (rlim < RLIM_INFINITY &&
	(brk - mm->start_brk) + (mm->end_data - mm->start_data) > rlim)

much easier to understand.

Hugh