[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 6 Jun 2016 15:07:19 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Mike Galbraith <umgwanakikbuti@...il.com>
Cc: Mel Gorman <mgorman@...e.de>, lkml <linux-kernel@...r.kernel.org>,
Thomas Gleixner <tglx@...utronix.de>,
Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
kirill@...temov.name
Subject: Re: [patch] futex: Fix v4.6+ ltp futex_wait04 regression
On Mon, Jun 06, 2016 at 11:40:41AM +0200, Mike Galbraith wrote:
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -469,7 +469,7 @@ get_futex_key(u32 __user *uaddr, int fsh
> {
> unsigned long address = (unsigned long)uaddr;
> struct mm_struct *mm = current->mm;
> - struct page *page;
> + struct page *page, *pinned = NULL;
> struct address_space *mapping;
> int err, ro = 0;
>
> @@ -530,8 +530,18 @@ get_futex_key(u32 __user *uaddr, int fsh
> * considered here and page lock forces unnecessarily serialization
> * From this point on, mapping will be re-verified if necessary and
> * page lock will be acquired only if it is unavoidable
> - */
> + *
> + * If we're dealing with a compound page, save our reference to the
> + * page we locked in memory above, and take a new reference on the
> + * page head, dropping the previously pinned page reference on retry.
> + */
> + if (unlikely(pinned && page != pinned))
> + put_page(pinned);
> + pinned = page;
> page = compound_head(page);
> + if (unlikely(pinned != page))
> + get_page(page);
Not needed, since {get,put}_page() explicitly use compound_head() to
track the reference count on.
> +
> mapping = READ_ONCE(page->mapping);
>
> /*
> @@ -560,12 +570,14 @@ get_futex_key(u32 __user *uaddr, int fsh
> lock_page(page);
> shmem_swizzled = PageSwapCache(page) || page->mapping;
> unlock_page(page);
> - put_page(page);
>
> - if (shmem_swizzled)
> + if (shmem_swizzled) {
> + put_page(page);
> goto again;
> + }
>
> - return -EFAULT;
> + err = -EFAULT;
> + goto out;
This also simplifies away
> }
>
> /*
> @@ -654,12 +666,14 @@ get_futex_key(u32 __user *uaddr, int fsh
>
> key->both.offset |= FUT_OFF_INODE; /* inode-based key */
> key->shared.inode = inode;
> - key->shared.pgoff = basepage_index(page);
> + key->shared.pgoff = basepage_index(pinned);
But this seems to be the actual fix; because while I think the compound
page cannot change from under us, it can change between futex
invocations, and then having a key change is _bad_.
But someone other than me would have to concurr, its been too long since
I looked at all this compound muck.
> rcu_read_unlock();
> }
>
> out:
> put_page(page);
> + if (unlikely(pinned != page))
> + put_page(pinned);
and that too can go.
> return err;
> }
Powered by blists - more mailing lists