futex: Remove unnecessary warning from get_futex_key
PATCH 1/1 futex: Remove unnecessary warning from get_futex_key
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 16pthread_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;
}--- kernel/futex.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
kernel/futex.c +3 -2
@@ -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);
Quoted 7 lines · Mel Gorman
Commit
65d8fc777f6d("futex: Remove requirement forlock_page()inget_futex_key()") removed an unnecessarylock_page()with the side-effect thatpage->mappingneeded 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.
[...]
Quoted 15 lines
Reported-by Mark Rutland <[email protected]>Signed-off-by Mel Gorman <[email protected]>Cc [email protected] # 4.7+--- kernel/futex.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)kernel/futex.c +0
-0@@ -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
kernel/futex.c get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw) · line 680
Quoted 10 lines
kernel/futex.c +3
-2-* 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:
Thanks for putting this together!
Mark.
Quoted 22 lines · Mark Rutland
Quoted 8 lines
kernel/futex.c +0
-0@@ -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 kernel/futex.c get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw) · line 680
Quoted 10 lines
kernel/futex.c +3
-2-* 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:
Quoted 4 lines
Tested-by Mark Rutland <[email protected]>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.
Signature · 2 lines
Mel Gorman SUSE Labs
Peter Zijlstra
Acked-byQuoted 28 lines · Mel Gorman
Quoted 22 lines · Mark Rutland
Quoted 8 lines
kernel/futex.c +0
-0@@ -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 kernel/futex.c get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw) · line 680
Quoted 10 lines
kernel/futex.c +3
-2-* 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:
Quoted 4 lines
Tested-by Mark Rutland <[email protected]>Thanks for putting this together!
Quoted 6 lines
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,
Quoted 29 lines · Peter Zijlstra
Quoted 28 lines · Mel Gorman
Quoted 26 lines · Mark Rutland
Quoted 8 lines
kernel/futex.c +0
-0@@ -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 kernel/futex.c get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw) · line 680
Quoted 10 lines
kernel/futex.c +3
-2-* 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 <[email protected]>Thanks for putting this together!
Quoted 9 lines
Quoted 6 lines
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?
Signature · 2 lines
Mel Gorman SUSE Labs
Quoted 2 lines · Mel Gorman
Linus, can you pick it up directly please?
Done.
Linus