linux-kernel - Re: [PATCH] mm: Warn once when a page is freed with PG

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: <20090723102938.GA27731@csn.ul.ie>
Date:	Thu, 23 Jul 2009 11:29:39 +0100
From:	Mel Gorman <mel@....ul.ie>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Christoph Lameter <cl@...ux-foundation.org>,
	kosaki.motohiro@...fujitsu.com, maximlevitsky@...il.com,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	Lee.Schermerhorn@...com, penberg@...helsinki.fi,
	hannes@...xchg.org, jirislaby@...il.com
Subject: Re: [PATCH] mm: Warn once when a page is freed with PG_mlocked set
	V2

On Wed, Jul 22, 2009 at 04:06:49PM -0700, Andrew Morton wrote:
> On Wed, 15 Jul 2009 10:31:54 -0400 (EDT)
> Christoph Lameter <cl@...ux-foundation.org> wrote:
> 
> > On Wed, 15 Jul 2009, Mel Gorman wrote:
> > 
> > > -static inline int free_pages_check(struct page *page)
> > > +static inline int free_pages_check(struct page *page, int wasMlocked)
> > >  {
> > > +	WARN_ONCE(wasMlocked, KERN_WARNING
> > > +		"Page flag mlocked set for process %s at pfn:%05lx\n"
> > > +		"page:%p flags:0x%lX\n",
> > > +		current->comm, page_to_pfn(page),
> > > +		page, page->flags|__PG_MLOCKED);
> > > +
> > >  	if (unlikely(page_mapcount(page) |
> > 
> > There is already a free_page_mlocked() that is only called if the mlock
> > bit is set. Move it into there to avoid having to run two checks in the
> > hot codee path?
> 
> Agreed.
> 
> This patch gratuitously adds hotpath overhead.  Moving the change to be
> inside those preexisting wasMlocked tests will reduce its overhead a lot.
> 

It adds code duplication then, one of which is in a fast path.

> As it stands, I'm really doubting that the patch's utility is worth its
> cost.
> 

I'm happy to let this one drop. It seemed like it would be nice for debugging
while there are still corner cases where mlocked pages are getting freed
instead of torn down but we already account for that situation occuring. While
I think it'll be tricky to spot, it's probably preferable to having warnings
spew out onto dmesg.

> Also, it's a bit of a figleaf, but please consider making more use of
> CONFIG_DEBUG_VM (see VM_BUG_ON()).
> 

In this particular case, I doubted that DEBUG_VM would be set on
machines that were triggering the mlock corner case but yeah, this does
look more like a DEBUG_VM than a production thing.

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab
--
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/