linux-kernel - Re: PATCH? introduce get_compound_page (Was: process 'stuck' at exit)

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: <20131216183618.GA28252@redhat.com>
Date:	Mon, 16 Dec 2013 19:36:18 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Andrea Arcangeli <aarcange@...hat.com>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Dave Jones <davej@...hat.com>,
	Darren Hart <dvhart@...ux.intel.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Mel Gorman <mgorman@...e.de>
Subject: Re: PATCH? introduce get_compound_page (Was: process 'stuck' at
	exit)

On 12/13, Andrea Arcangeli wrote:
>
> On Fri, Dec 13, 2013 at 05:22:40PM +0100, Oleg Nesterov wrote:
> >
> > I'll try to make v2 based on -mm and your suggestions.
>
> Ok great!

Yes, it would be great, but I need your help again ;)

Let me quote the pseudo-code you sent me:

	put_compound_tail(page) {
		page_head = compound_trans_head(page);
		if (!__compound_tail_refcounted(page_head)) {
			...
			return ...;
		}

		flags = compound_lock_irqsave(page_head);
		...

Sure, put_compound_tail() should be the simplified version of
put_compound_page() which doesn't dec page_head->_count, this is clear.

But afaics, compound_lock_irqsave() above looks unsafe without
get_page_unless_zero(page_head) ? If we race with _split, page_head
can be freed and compound_lock() can race with, say, free_pages_check()
which plays with page->flags ?

So it seems that put_compound_tail() should also do get/put(head) like
put_compound_page() does, and this probably means we should factor out
the common code somehow.

Or I missed something?

OTOH, I can't really understand

	if (likely(page != page_head && get_page_unless_zero(page_head)))

in __get_page_tail() and put_compound_page().

First of all, is it really possible that page == compound_trans_head(page)?
We already verified that PG_tail was set. Of course this bit can be cleared
and ->first_page can be a dangling pointer but it can never be changed to
point to this page? (in fact, afaics it can be changed at all as long as we
have a reference, but this doesn't matter).

And compound_lock_irqsave() looks racy even after get_page_unless_zero().

For example, suppose that page_head was already freed and then re-allocated
as (say) alloc_pages(__GFP_COMP, 1). get_page_unless_zero() can succeed right
after prep_new_page() does set_page_refcounted(). Now, can't compound_lock()
race with the non-atomic prep_compound_page()->__SetPageHead() ?

Finally. basepage_index(page) after put_page(page) in get_futex_key() looks
confusing imho. I think this is correct, we already checked PageAnon() so it
can't be a thp page. But probably this needs a comment and __basepage_index()
should do BUG_ON(!PageHuge()).

Oleg.

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