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]

Date:	Mon, 16 Dec 2013 21:46:26 +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/16, Andrea Arcangeli wrote:
>
> On Mon, Dec 16, 2013 at 07:36:18PM +0100, Oleg Nesterov wrote:
> >
> > 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.
> >
>
> Yes it was supposed to do the get_page_unless_zero before the
> compound_lock.

OK,

> > 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)?
>
> ...
>
> If PG_tail gets cleared compound_trans_head returns "page".

Damn, indeed, thanks.

> > 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() ?
>
> Yes. We need to change to:
>
> 	if (order && (gfp_flags & __GFP_COMP))
> 		prep_compound_page(page, order);
> 	smp_wmb();
> 	/* as the compound_lock can be taken after it's refcounted */
> 	set_page_refcounted(page);
>
> __SetPageHead uses bts asm insn so literally only a "lock" prefix is
> missing in a assembly instruction. So the race window is incredibly
> small, but it must be fixed indeed. This also puts set_page_refcounted
> as the last action of buffered_rmqueue so there shouldn't be any other
> issues like this left in the page allocation code.
>
> Can you reorder set_page_refcount in your v2?

OK. I'll try to make something on Wednesday.


> > 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()).
>
> This looks a bug if you apply the patches to add THP in pagecache, and
> BUG_ON(!PageHuge) would also break THP in pagecache later (though
> safer than silently broken like now).
>
> It'd safer to read the basepage_index in a local variable just before
> doing the put_compund_tail but I agree it's not going to make a
> difference right now.

Yes, so lets discuss (and perhaps change) this later/separately.

Andrea, thanks again for your help.

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/