linux-kernel - Re: [PATCH] Do not account for the address space used by hugetlbfs using VM

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:	Wed, 11 Feb 2009 12:03:17 +0000
From:	Andy Whitcroft <apw@...onical.com>
To:	Mel Gorman <mel@....ul.ie>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	Hugh Dickins <hugh@...itas.com>,
	Lee Schermerhorn <Lee.Schermerhorn@...com>,
	Greg KH <gregkh@...e.de>,
	Maksim Yevmenkin <maksim.yevmenkin@...il.com>,
	Nick Piggin <npiggin@...e.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	will@...wder-design.com, Rik van Riel <riel@...hat.com>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Mikos Szeredi <miklos@...redi.hu>, wli@...ementarian.org
Subject: Re: [PATCH] Do not account for the address space used by hugetlbfs
	using VM_ACCOUNT V2 (Was Linus 2.6.29-rc4)

On Wed, Feb 11, 2009 at 10:30:01AM +0000, Mel Gorman wrote:

> > >  	/*
> > >  	 * Shared mappings base their reservation on the number of pages that
> > >  	 * are already allocated on behalf of the file. Private mappings need
> > > @@ -2285,22 +2283,25 @@ int hugetlb_reserve_pages(struct inode *inode,
> > >  	 */
> > >  	if (!vma || vma->vm_flags & VM_SHARED)
> > >  		chg = region_chg(&inode->i_mapping->private_list, from, to);
> > 
> > I thought the region map for a VM_SHARED mapping is meant to contain
> > those pages for which we already have a reservation allocated.  So that
> > if we have overlapping VM_RESERVE and VM_NORESERVE mappings we know that
> > we did have a page reserved at fault time and know whether we can take
> > it from the reserve portion of the pool.  By letting this get executed
> > for VM_NORESERVE mappings that would seem to be getting out of sync,
> > which doesn't sound right. 
> 
> This part doesn't get executed for NORESERVE so while region_chg() took
> place to calculate a reservation, region_add() did not get called to
> commit it.

Bah I wrote those damn routines and even I get confused.  As you say
they are a prepare/commit pair and this is only the prepare phase you
are executing, no persistant change is made; so its safe.

> > > +	if (acctflag & VM_NORESERVE) {
> > > +		reset_vma_resv_huge_pages(vma);
> > 
> > Why do we now need to do this in the non-reserve case?  We didn't need
> > to do it before.
> > 
> 
> True, it was largely defensive against anything being in page_private
> that would make it think it had reserves. 

Defensive is good.

> > > +		return 0;
> > > +	}
> > > +
> > 
> > This also seems like a semantic change.  Previously NO_RESERVE did not
> > take quota, now it does.  NO_RESERVE used to mean that we took our
> > chances on there being pages available at fault time both for quota and
> > in the pools. Now it means that we only risk there being no pages.
> > Does that not significantly change semantics. 
> 
> Yes, and this was a mistake. For noreserve mappings, we may now be taking
> twice the amount of quota and probably leaking it. This is wrong and I need
> to move the check for quota below the check for VM_NORESERVE. Good spot.

Thanks.

-apw
--
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/