Re: [PATCH] futex: Remove unnecessary warning from get_futex_key
On Wed, Aug 9, 2017 at 8:31 AM, Mel Gormanwrote: > > Linus, can you pick it up directly please? Done. Linus
Re: [PATCH] futex: Remove unnecessary warning from get_futex_key
On Wed, Aug 9, 2017 at 8:31 AM, Mel Gorman wrote: > > Linus, can you pick it up directly please? Done. Linus
Re: [PATCH] futex: Remove unnecessary warning from get_futex_key
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(>i_count))) { > > > > + if (!atomic_inc_not_zero(>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> > > > > > 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
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(>i_count))) { > > > > + if (!atomic_inc_not_zero(>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 > > > > > > 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
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(>i_count))) { > > > + if (!atomic_inc_not_zero(>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> > > > 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)
Re: [PATCH] futex: Remove unnecessary warning from get_futex_key
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(>i_count))) { > > > + if (!atomic_inc_not_zero(>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 > > > > 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)
Re: [PATCH] futex: Remove unnecessary warning from get_futex_key
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(>i_count))) { > > + if (!atomic_inc_not_zero(>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> > 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
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(>i_count))) { > > + if (!atomic_inc_not_zero(>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 > > 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
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> Signed-off-by: Mel Gorman > Cc: sta...@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(>i_count))) { > + if (!atomic_inc_not_zero(>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 Thanks for putting this together! Mark.
Re: [PATCH] futex: Remove unnecessary warning from get_futex_key
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 > Signed-off-by: Mel Gorman > Cc: sta...@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(>i_count))) { > + if (!atomic_inc_not_zero(>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 Thanks for putting this together! Mark.
[PATCH] 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: 80001e271780 task.stack: 10908000 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 : [] lr : [] pstate: 8145 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 #include #include #include #include #include #include #include #define NR_FUTEX_THREADS 16 pthread_t threads[NR_FUTEX_THREADS]; void *mem; #define MEM_PROT(PROT_READ | PROT_WRITE) #define MEM_SIZE65536 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([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 RutlandSigned-off-by: Mel Gorman Cc: sta...@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(>i_count))) { + if (!atomic_inc_not_zero(>i_count)) { rcu_read_unlock(); put_page(page);
[PATCH] 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: 80001e271780 task.stack: 10908000 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 : [] lr : [] pstate: 8145 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 #include #include #include #include #include #include #include #define NR_FUTEX_THREADS 16 pthread_t threads[NR_FUTEX_THREADS]; void *mem; #define MEM_PROT(PROT_READ | PROT_WRITE) #define MEM_SIZE65536 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([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 Signed-off-by: Mel Gorman Cc: sta...@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(>i_count))) { + if (!atomic_inc_not_zero(>i_count)) { rcu_read_unlock(); put_page(page);
Re: [PATCH] futex: Remove unnecessary warning from get_futex_key
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(>i_count))) { + if (!atomic_inc_not_zero(>i_count)) { rcu_read_unlock(); put_page(page); Thanks, Davidlohr
Re: [PATCH] futex: Remove unnecessary warning from get_futex_key
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(>i_count))) { + if (!atomic_inc_not_zero(>i_count)) { rcu_read_unlock(); put_page(page); Thanks, Davidlohr
[PATCH] 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 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: 80001e271780 task.stack: 10908000 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 : [] lr : [] pstate: 8145 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 #include #include #include #include #include #include #include #define NR_FUTEX_THREADS 16 pthread_t threads[NR_FUTEX_THREADS]; void *mem; #define MEM_PROT(PROT_READ | PROT_WRITE) #define MEM_SIZE65536 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([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 RutlandSigned-off-by: Mel Gorman Cc: sta...@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(>i_count))) { + if (!atomic_inc_not_zero(>i_count)) { rcu_read_unlock(); put_page(page);
[PATCH] 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 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: 80001e271780 task.stack: 10908000 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 : [] lr : [] pstate: 8145 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 #include #include #include #include #include #include #include #define NR_FUTEX_THREADS 16 pthread_t threads[NR_FUTEX_THREADS]; void *mem; #define MEM_PROT(PROT_READ | PROT_WRITE) #define MEM_SIZE65536 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([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 Signed-off-by: Mel Gorman Cc: sta...@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(>i_count))) { + if (!atomic_inc_not_zero(>i_count)) { rcu_read_unlock(); put_page(page);