Re: [patch] futex: Fix v4.6+ ltp futex_wait04 regression

Peter Zijlstra
2016-06-06T13:07:27Z

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;
> }