[PATCH] futex: Remove unnecessary warning from get_futex_key

[PATCH] futex: Remove unnecessary warning from get_futex_key

[Mel Gorman]

Commit 65d8fc777f6d ("futex: Remove requirement for lock_page() in
get_futex_key()") removed an unnecessary lock_page() with the side-effect
that page->mapping needed to be treated very carefully. Two defensive
warnings were added in case any assumption was wrong with the first warning
assuming a correct application would not truncate a mapping backing an
active futex key. Since merging, it has not triggered for any unexpected
reason but Mark Rutland reported the following bug;

------------[ cut here ]------------
kernel BUG at kernel/futex.c:679!
Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
Modules linked in:
CPU: 0 PID: 3695 Comm: syz-executor1 Not tainted 4.13.0-rc3-00020-g307fec773ba3 #3
Hardware name: linux,dummy-virt (DT)
task: ffff80001e271780 task.stack: ffff000010908000
PC is at get_futex_key+0x6a4/0xcf0 kernel/futex.c:679
LR is at get_futex_key+0x6a4/0xcf0 kernel/futex.c:679
pc : [<ffff00000821ac14>] lr : [<ffff00000821ac14>] pstate: 80000145

The fact that it's a bug instead of a warning was due to an unrelated issue
but the warning itself triggered because the underlying mapping changed. This
is an application issue but it's recoverable and the warning is unnecessary
so this patch removes the warning. The warning may be triggered with the
following test program from Mark although it may require NR_FUTEX_THREADS
to be less than the number of cores in the system.

#include <linux/futex.h>
#include <pthread.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/mman.h>
#include <sys/syscall.h>
#include <sys/time.h>
#include <unistd.h>

#define NR_FUTEX_THREADS 16
pthread_t threads[NR_FUTEX_THREADS];

void *mem;

#define MEM_PROT        (PROT_READ | PROT_WRITE)
#define MEM_SIZE        65536

static int futex_wrapper(int *uaddr, int op, int val,
                         const struct timespec *timeout,
                         int *uaddr2, int val3)
{
        syscall(SYS_futex, uaddr, op, val, timeout, uaddr2, val3);
}

void *poll_futex(void *unused)
{
        for (;;) {
                futex_wrapper(mem, FUTEX_CMP_REQUEUE_PI, 1, NULL, mem + 4, 1);
        }
}

int main(int argc, char *argv[])
{
        int i;

        mem = mmap(NULL, MEM_SIZE, MEM_PROT,
                   MAP_SHARED | MAP_ANONYMOUS, -1, 0);

        printf("Mapping @ %p\n", mem);

        printf("Creating futex threads...\n");

        for (i = 0; i < NR_FUTEX_THREADS; i++)
                pthread_create(&threads[i], NULL, poll_futex, NULL);

        printf("Flipping mapping...\n");
        for (;;) {
                mmap(mem, MEM_SIZE, MEM_PROT,
                     MAP_FIXED | MAP_SHARED | MAP_ANONYMOUS, -1, 0);
        }

        return 0;
}

Reported-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Mel Gorman <mgorman@suse.de>
Cc: stable@vger.kernel.org # 4.7+
---
 kernel/futex.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 16dbe4c93895..d0752185668d 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -676,7 +676,7 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw)
 		 * cases, therefore a successful atomic_inc return below will
 		 * guarantee that get_futex_key() will still imply smp_mb(); (B).
 		 */
-		if (WARN_ON_ONCE(!atomic_inc_not_zero(&inode->i_count))) {
+		if (!atomic_inc_not_zero(&inode->i_count)) {
 			rcu_read_unlock();
 			put_page(page);
 

Re: [PATCH] futex: Remove unnecessary warning from get_futex_key

[Davidlohr Bueso]

On Wed, 09 Aug 2017, Mel Gorman wrote:

>@@ -676,7 +676,7 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw)
> 		 * cases, therefore a successful atomic_inc return below will
> 		 * guarantee that get_futex_key() will still imply smp_mb(); (B).

You missed the comment above.

diff --git a/kernel/futex.c b/kernel/futex.c
index 16dbe4c93895..6b4a6a7cad3d 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -670,13 +670,14 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw)
 		 * this reference was taken by ihold under the page lock
 		 * pinning the inode in place so i_lock was unnecessary. The
 		 * only way for this check to fail is if the inode was
-		 * truncated in parallel so warn for now if this happens.
+		 * truncated in parallel -- which is a bizarre scenario, in
+		 * any case, just retry.
 		 *
 		 * We are not calling into get_futex_key_refs() in file-backed
 		 * cases, therefore a successful atomic_inc return below will
 		 * guarantee that get_futex_key() will still imply smp_mb(); (B).
 		 */
-		if (WARN_ON_ONCE(!atomic_inc_not_zero(&inode->i_count))) {
+		if (!atomic_inc_not_zero(&inode->i_count)) {
 			rcu_read_unlock();
 			put_page(page);
 

Thanks,
Davidlohr

[PATCH] futex: Remove unnecessary warning from get_futex_key

[Mel Gorman]

Commit 65d8fc777f6d ("futex: Remove requirement for lock_page() in
get_futex_key()") removed an unnecessary lock_page() with the side-effect
that page->mapping needed to be treated very carefully. Two defensive
warnings were added in case any assumption was missed and the first warning
assumed a correct application would not alter a mapping backing a futex key.
Since merging, it has not triggered for any unexpected case but Mark
Rutland reported the following bug triggering due to the first warning.

------------[ cut here ]------------
kernel BUG at kernel/futex.c:679!
Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
Modules linked in:
CPU: 0 PID: 3695 Comm: syz-executor1 Not tainted 4.13.0-rc3-00020-g307fec773ba3 #3
Hardware name: linux,dummy-virt (DT)
task: ffff80001e271780 task.stack: ffff000010908000
PC is at get_futex_key+0x6a4/0xcf0 kernel/futex.c:679
LR is at get_futex_key+0x6a4/0xcf0 kernel/futex.c:679
pc : [<ffff00000821ac14>] lr : [<ffff00000821ac14>] pstate: 80000145

The fact that it's a bug instead of a warning was due to an unrelated
issue but the warning itself triggered because the underlying mapping
changed. This is an application issue but from a kernel perspective it's
a recoverable situation and the warning is unnecessary so this patch
removes the warning. The warning may potentially be triggered with the
following test program from Mark although it may be necessary to adjust
NR_FUTEX_THREADS to be a value smaller than the number of CPUs in the system.

#include <linux/futex.h>
#include <pthread.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/mman.h>
#include <sys/syscall.h>
#include <sys/time.h>
#include <unistd.h>

#define NR_FUTEX_THREADS 16
pthread_t threads[NR_FUTEX_THREADS];

void *mem;

#define MEM_PROT        (PROT_READ | PROT_WRITE)
#define MEM_SIZE        65536

static int futex_wrapper(int *uaddr, int op, int val,
                         const struct timespec *timeout,
                         int *uaddr2, int val3)
{
        syscall(SYS_futex, uaddr, op, val, timeout, uaddr2, val3);
}

void *poll_futex(void *unused)
{
        for (;;) {
                futex_wrapper(mem, FUTEX_CMP_REQUEUE_PI, 1, NULL, mem + 4, 1);
        }
}

int main(int argc, char *argv[])
{
        int i;

        mem = mmap(NULL, MEM_SIZE, MEM_PROT,
                   MAP_SHARED | MAP_ANONYMOUS, -1, 0);

        printf("Mapping @ %p\n", mem);

        printf("Creating futex threads...\n");

        for (i = 0; i < NR_FUTEX_THREADS; i++)
                pthread_create(&threads[i], NULL, poll_futex, NULL);

        printf("Flipping mapping...\n");
        for (;;) {
                mmap(mem, MEM_SIZE, MEM_PROT,
                     MAP_FIXED | MAP_SHARED | MAP_ANONYMOUS, -1, 0);
        }

        return 0;
}

Reported-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Mel Gorman <mgorman@suse.de>
Cc: stable@vger.kernel.org # 4.7+
---
 kernel/futex.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 16dbe4c93895..f50b434756c1 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -670,13 +670,14 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw)
 		 * this reference was taken by ihold under the page lock
 		 * pinning the inode in place so i_lock was unnecessary. The
 		 * only way for this check to fail is if the inode was
-		 * truncated in parallel so warn for now if this happens.
+		 * truncated in parallel which is almost certainly an
+		 * application bug. In such a case, just retry.
 		 *
 		 * We are not calling into get_futex_key_refs() in file-backed
 		 * cases, therefore a successful atomic_inc return below will
 		 * guarantee that get_futex_key() will still imply smp_mb(); (B).
 		 */
-		if (WARN_ON_ONCE(!atomic_inc_not_zero(&inode->i_count))) {
+		if (!atomic_inc_not_zero(&inode->i_count)) {
 			rcu_read_unlock();
 			put_page(page);
 

Re: [PATCH] futex: Remove unnecessary warning from get_futex_key

[Mark Rutland]

On Wed, Aug 09, 2017 at 08:27:11AM +0100, Mel Gorman wrote:
> Commit 65d8fc777f6d ("futex: Remove requirement for lock_page() in
> get_futex_key()") removed an unnecessary lock_page() with the side-effect
> that page->mapping needed to be treated very carefully. Two defensive
> warnings were added in case any assumption was missed and the first warning
> assumed a correct application would not alter a mapping backing a futex key.
> Since merging, it has not triggered for any unexpected case but Mark
> Rutland reported the following bug triggering due to the first warning.

[...]

> Reported-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Mel Gorman <mgorman@suse.de>
> Cc: stable@vger.kernel.org # 4.7+
> ---
>  kernel/futex.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/futex.c b/kernel/futex.c
> index 16dbe4c93895..f50b434756c1 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -670,13 +670,14 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw)
>  		 * this reference was taken by ihold under the page lock
>  		 * pinning the inode in place so i_lock was unnecessary. The
>  		 * only way for this check to fail is if the inode was
> -		 * truncated in parallel so warn for now if this happens.
> +		 * truncated in parallel which is almost certainly an
> +		 * application bug. In such a case, just retry.
>  		 *
>  		 * We are not calling into get_futex_key_refs() in file-backed
>  		 * cases, therefore a successful atomic_inc return below will
>  		 * guarantee that get_futex_key() will still imply smp_mb(); (B).
>  		 */
> -		if (WARN_ON_ONCE(!atomic_inc_not_zero(&inode->i_count))) {
> +		if (!atomic_inc_not_zero(&inode->i_count)) {

I applied the same diff yesterday, and haven't seen anything go wrong
with my test case and/or with Syzkaller running, so FWIW:

Tested-by: Mark Rutland <mark.rutland@arm.com>

Thanks for putting this together!

Mark.

Re: [PATCH] futex: Remove unnecessary warning from get_futex_key

[Mel Gorman]

On Wed, Aug 09, 2017 at 03:05:19PM +0100, Mark Rutland wrote:
> > diff --git a/kernel/futex.c b/kernel/futex.c
> > index 16dbe4c93895..f50b434756c1 100644
> > --- a/kernel/futex.c
> > +++ b/kernel/futex.c
> > @@ -670,13 +670,14 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw)
> >  		 * this reference was taken by ihold under the page lock
> >  		 * pinning the inode in place so i_lock was unnecessary. The
> >  		 * only way for this check to fail is if the inode was
> > -		 * truncated in parallel so warn for now if this happens.
> > +		 * truncated in parallel which is almost certainly an
> > +		 * application bug. In such a case, just retry.
> >  		 *
> >  		 * We are not calling into get_futex_key_refs() in file-backed
> >  		 * cases, therefore a successful atomic_inc return below will
> >  		 * guarantee that get_futex_key() will still imply smp_mb(); (B).
> >  		 */
> > -		if (WARN_ON_ONCE(!atomic_inc_not_zero(&inode->i_count))) {
> > +		if (!atomic_inc_not_zero(&inode->i_count)) {
> 
> I applied the same diff yesterday, and haven't seen anything go wrong
> with my test case and/or with Syzkaller running, so FWIW:
> 
> Tested-by: Mark Rutland <mark.rutland@arm.com>
> 
> Thanks for putting this together!
> 

No problem. FWIW, I had the test case running for 12 hours in a loop as
well and other than having to adjust the number of threads doing futex()
to trigger the warning without the patch, I observed no other problems.
If Thomas is happy, I hope this can be merged for 4.13 (or picked up
directly by Linus if he feels like it). Even if it's delayed, I'll resubmit
to -stable manually if the "Cc: stable" gets stripped along the way.

-- 
Mel Gorman
SUSE Labs

Re: [PATCH] futex: Remove unnecessary warning from get_futex_key

[Peter Zijlstra]

On Wed, Aug 09, 2017 at 03:43:09PM +0100, Mel Gorman wrote:
> On Wed, Aug 09, 2017 at 03:05:19PM +0100, Mark Rutland wrote:
> > > diff --git a/kernel/futex.c b/kernel/futex.c
> > > index 16dbe4c93895..f50b434756c1 100644
> > > --- a/kernel/futex.c
> > > +++ b/kernel/futex.c
> > > @@ -670,13 +670,14 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw)
> > >  		 * this reference was taken by ihold under the page lock
> > >  		 * pinning the inode in place so i_lock was unnecessary. The
> > >  		 * only way for this check to fail is if the inode was
> > > -		 * truncated in parallel so warn for now if this happens.
> > > +		 * truncated in parallel which is almost certainly an
> > > +		 * application bug. In such a case, just retry.
> > >  		 *
> > >  		 * We are not calling into get_futex_key_refs() in file-backed
> > >  		 * cases, therefore a successful atomic_inc return below will
> > >  		 * guarantee that get_futex_key() will still imply smp_mb(); (B).
> > >  		 */
> > > -		if (WARN_ON_ONCE(!atomic_inc_not_zero(&inode->i_count))) {
> > > +		if (!atomic_inc_not_zero(&inode->i_count)) {
> > 
> > I applied the same diff yesterday, and haven't seen anything go wrong
> > with my test case and/or with Syzkaller running, so FWIW:
> > 
> > Tested-by: Mark Rutland <mark.rutland@arm.com>
> > 
> > Thanks for putting this together!
> > 
> 
> No problem. FWIW, I had the test case running for 12 hours in a loop as
> well and other than having to adjust the number of threads doing futex()
> to trigger the warning without the patch, I observed no other problems.
> If Thomas is happy, I hope this can be merged for 4.13 (or picked up
> directly by Linus if he feels like it). Even if it's delayed, I'll resubmit
> to -stable manually if the "Cc: stable" gets stripped along the way.

Probably best if Linus picks this up directly as Thomas is on holidays.

In any case,

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Re: [PATCH] futex: Remove unnecessary warning from get_futex_key

[Mel Gorman]

On Wed, Aug 09, 2017 at 05:08:34PM +0200, Peter Zijlstra wrote:
> On Wed, Aug 09, 2017 at 03:43:09PM +0100, Mel Gorman wrote:
> > On Wed, Aug 09, 2017 at 03:05:19PM +0100, Mark Rutland wrote:
> > > > diff --git a/kernel/futex.c b/kernel/futex.c
> > > > index 16dbe4c93895..f50b434756c1 100644
> > > > --- a/kernel/futex.c
> > > > +++ b/kernel/futex.c
> > > > @@ -670,13 +670,14 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw)
> > > >  		 * this reference was taken by ihold under the page lock
> > > >  		 * pinning the inode in place so i_lock was unnecessary. The
> > > >  		 * only way for this check to fail is if the inode was
> > > > -		 * truncated in parallel so warn for now if this happens.
> > > > +		 * truncated in parallel which is almost certainly an
> > > > +		 * application bug. In such a case, just retry.
> > > >  		 *
> > > >  		 * We are not calling into get_futex_key_refs() in file-backed
> > > >  		 * cases, therefore a successful atomic_inc return below will
> > > >  		 * guarantee that get_futex_key() will still imply smp_mb(); (B).
> > > >  		 */
> > > > -		if (WARN_ON_ONCE(!atomic_inc_not_zero(&inode->i_count))) {
> > > > +		if (!atomic_inc_not_zero(&inode->i_count)) {
> > > 
> > > I applied the same diff yesterday, and haven't seen anything go wrong
> > > with my test case and/or with Syzkaller running, so FWIW:
> > > 
> > > Tested-by: Mark Rutland <mark.rutland@arm.com>
> > > 
> > > Thanks for putting this together!
> > > 
> > 
> > No problem. FWIW, I had the test case running for 12 hours in a loop as
> > well and other than having to adjust the number of threads doing futex()
> > to trigger the warning without the patch, I observed no other problems.
> > If Thomas is happy, I hope this can be merged for 4.13 (or picked up
> > directly by Linus if he feels like it). Even if it's delayed, I'll resubmit
> > to -stable manually if the "Cc: stable" gets stripped along the way.
> 
> Probably best if Linus picks this up directly as Thomas is on holidays.
> 

Whoops, I had no idea. Primarily I wanted it go through Thomas under the
general heading of "if you screw up futex.c, Thomas will look for you and
he will find you". The patch in this case is trivial and enough people
have looked at it. Linus, can you pick it up directly please?

-- 
Mel Gorman
SUSE Labs

Re: [PATCH] futex: Remove unnecessary warning from get_futex_key

[Linus Torvalds]

On Wed, Aug 9, 2017 at 8:31 AM, Mel Gorman <mgorman@suse.de> wrote:
>
> Linus, can you pick it up directly please?

Done.

              Linus