Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1608300 > unrolled thread

Re: [x86/mm/gup] 2947ba054a [ 71.329069] kernel BUG at include/linux/pagemap.h:151!

Started by"Kirill A. Shutemov" <kirill@shutemov.name>
First post2017-03-24 11:30 +0100
Last post2017-03-28 08:40 +0200
Articles 5 — 4 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [x86/mm/gup] 2947ba054a [   71.329069] kernel BUG at  include/linux/pagemap.h:151! "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-03-24 11:30 +0100
    Re: [x86/mm/gup] 2947ba054a [   71.329069] kernel BUG at  include/linux/pagemap.h:151! Peter Zijlstra <peterz@infradead.org> - 2017-03-24 12:00 +0100
      Re: [x86/mm/gup] 2947ba054a [   71.329069] kernel BUG at  include/linux/pagemap.h:151! "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2017-03-24 13:00 +0100
        Re: [x86/mm/gup] 2947ba054a [   71.329069] kernel BUG at  include/linux/pagemap.h:151! "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-03-27 17:40 +0200
        [tip:x86/mm] mm: Fix false-positive VM_BUG_ON() in  page_cache_{get,add}_speculative() "tip-bot for Kirill A. Shutemov" <tipbot@zytor.com> - 2017-03-28 08:40 +0200

#1608300 — Re: [x86/mm/gup] 2947ba054a [ 71.329069] kernel BUG at include/linux/pagemap.h:151!

From"Kirill A. Shutemov" <kirill@shutemov.name>
Date2017-03-24 11:30 +0100
SubjectRe: [x86/mm/gup] 2947ba054a [ 71.329069] kernel BUG at include/linux/pagemap.h:151!
Message-ID<touDM-64-15@gated-at.bofh.it>
On Mon, Mar 20, 2017 at 06:51:24AM +0800, Fengguang Wu wrote:
> [   71.329069] kernel BUG at include/linux/pagemap.h:151!
> [   71.332456] invalid opcode: 0000 [#1]
> [   71.334359] CPU: 0 PID: 458 Comm: trinity-c0 Not tainted 4.11.0-rc2-00251-g2947ba0 #1
> [   71.338444] task: ffff88001f19ab00 task.stack: ffff88001f084000
> [   71.340586] RIP: 0010:gup_pud_range+0x56f/0x63d
> [   71.342886] RSP: 0018:ffff88001f087ba8 EFLAGS: 00010046
> [   71.345607] RAX: 0000000080000000 RBX: 000000000164e000 RCX: ffff88001e0badc0
> [   71.347923] RDX: dead000000000100 RSI: 0000000000000001 RDI: ffff88001e0badc0
> [   71.350249] RBP: ffff88001f087c38 R08: ffff88001f087cf8 R09: ffff88001f087c6c
> [   71.352741] R10: 0000000000000000 R11: ffff88001f19b0f0 R12: ffff88001f087c6c
> [   71.356086] R13: ffff88001e0badc0 R14: 800000001e7b7867 R15: 0000000000000000
> [   71.359328] FS:  00007f7ea7b60700(0000) GS:ffffffffae02f000(0000) knlGS:0000000000000000
> [   71.361945] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   71.363806] CR2: 00000000013eb130 CR3: 0000000017ddb000 CR4: 00000000000006f0
> [   71.366122] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [   71.368424] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 00000000000d0602
> [   71.370729] Call Trace:
> [   71.371537]  __get_user_pages_fast+0x107/0x136
> [   71.373435]  get_user_pages_fast+0x78/0x89
> [   71.375447]  get_futex_key+0xfd/0x350
> [   71.376999]  ? simple_write_end+0x83/0xbe
> [   71.378614]  futex_requeue+0x1a3/0x585
> [   71.380244]  do_futex+0x834/0x86f
> [   71.381893]  ? kvm_clock_read+0x16/0x1e
> [   71.383794]  ? paravirt_sched_clock+0x9/0xd
> [   71.385857]  ? lock_release+0x11e/0x328
> [   71.387760]  SyS_futex+0x125/0x135
> [   71.389446]  ? write_seqcount_end+0x1a/0x1f
> [   71.391499]  ? vtime_account_user+0x4b/0x50
> [   71.393404]  do_syscall_64+0x61/0x74
> [   71.394806]  entry_SYSCALL64_slow_path+0x25/0x25

Looks like a false-negative in_atomic() in page_cache_get_speculative().
We are under local_irq_save() it should be atomic enough.

Not sure why other architectures haven't seen this before. Maybe TINY_RCU
plus PREEMPT is not common enough.

And unlike other architectures which uses generic GUP_fast(), we don't
really need page_cache_get_speculative() x86. get_page() would work
perfectly fine as we don't free pages until IPI broadcast is completed.
So if you saw it in page tables it will not go away under you.

I'm not sure what is the best way to fix this.
Few options:
 - Drop the VM_BUG();
 - Bump preempt count during __get_user_pages_fast();
 - Use get_page() instead of page_cache_get_speculative() on x86.

Any opinions?

-- 
 Kirill A. Shutemov

[toc] | [next] | [standalone]

#1608309

FromPeter Zijlstra <peterz@infradead.org>
Date2017-03-24 12:00 +0100
Message-ID<tov6O-la-3@gated-at.bofh.it>
In reply to#1608300
On Fri, Mar 24, 2017 at 01:24:36PM +0300, Kirill A. Shutemov wrote:

> I'm not sure what is the best way to fix this.
> Few options:
>  - Drop the VM_BUG();
>  - Bump preempt count during __get_user_pages_fast();
>  - Use get_page() instead of page_cache_get_speculative() on x86.
> 
> Any opinions?

I think I'm in favour of the first; either remove or amend to include
irqs_disabled() or so.

This in favour of keeping the variants of GUP down.

[toc] | [prev] | [next] | [standalone]

#1608363

From"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Date2017-03-24 13:00 +0100
Message-ID<tow2S-10z-25@gated-at.bofh.it>
In reply to#1608309
On Fri, Mar 24, 2017 at 11:51:53AM +0100, Peter Zijlstra wrote:
> On Fri, Mar 24, 2017 at 01:24:36PM +0300, Kirill A. Shutemov wrote:
> 
> > I'm not sure what is the best way to fix this.
> > Few options:
> >  - Drop the VM_BUG();
> >  - Bump preempt count during __get_user_pages_fast();
> >  - Use get_page() instead of page_cache_get_speculative() on x86.
> > 
> > Any opinions?
> 
> I think I'm in favour of the first; either remove or amend to include
> irqs_disabled() or so.
> 
> This in favour of keeping the variants of GUP down.

Something like this?

-------------------8<-----------------------

From d2f416a3ee3e5dbb10e59d0b374d382fdc4ba082 Mon Sep 17 00:00:00 2001
From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Date: Fri, 24 Mar 2017 14:13:05 +0300
Subject: [PATCH] mm: Fix false-positive VM_BUG_ON() in
 page_cache_{get,add}_speculative

0day triggered this:

  kernel BUG at include/linux/pagemap.h:151!
  invalid opcode: 0000 [#1]
  CPU: 0 PID: 458 Comm: trinity-c0 Not tainted 4.11.0-rc2-00251-g2947ba0 #1
  task: ffff88001f19ab00 task.stack: ffff88001f084000
  RIP: 0010:gup_pud_range+0x56f/0x63d
  RSP: 0018:ffff88001f087ba8 EFLAGS: 00010046
  RAX: 0000000080000000 RBX: 000000000164e000 RCX: ffff88001e0badc0
  RDX: dead000000000100 RSI: 0000000000000001 RDI: ffff88001e0badc0
  RBP: ffff88001f087c38 R08: ffff88001f087cf8 R09: ffff88001f087c6c
  R10: 0000000000000000 R11: ffff88001f19b0f0 R12: ffff88001f087c6c
  R13: ffff88001e0badc0 R14: 800000001e7b7867 R15: 0000000000000000
  FS:  00007f7ea7b60700(0000) GS:ffffffffae02f000(0000) knlGS:0000000000000000
  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  CR2: 00000000013eb130 CR3: 0000000017ddb000 CR4: 00000000000006f0
  DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
  DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 00000000000d0602
  Call Trace:
   __get_user_pages_fast+0x107/0x136
   get_user_pages_fast+0x78/0x89
   get_futex_key+0xfd/0x350
   ? simple_write_end+0x83/0xbe
   futex_requeue+0x1a3/0x585
   do_futex+0x834/0x86f
   ? kvm_clock_read+0x16/0x1e
   ? paravirt_sched_clock+0x9/0xd
   ? lock_release+0x11e/0x328
   SyS_futex+0x125/0x135
   ? write_seqcount_end+0x1a/0x1f
   ? vtime_account_user+0x4b/0x50
   do_syscall_64+0x61/0x74
   entry_SYSCALL64_slow_path+0x25/0x25

It' VM_BUG_ON() due to false-negative in_atomic(). We call
page_cache_get_speculative() with disabled local interrupts.
It should be atomic enough.

Let's check for disabled interrupts in the VM_BUG_ON() too.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Reported-by: Fengguang Wu <fengguang.wu@intel.com>
---
 include/linux/pagemap.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index f84cf5f76366..e7bbd9d4dc6c 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -148,7 +148,7 @@ static inline int page_cache_get_speculative(struct page *page)
 
 #ifdef CONFIG_TINY_RCU
 # ifdef CONFIG_PREEMPT_COUNT
-	VM_BUG_ON(!in_atomic());
+	VM_BUG_ON(!in_atomic() && !irqs_disabled());
 # endif
 	/*
 	 * Preempt must be disabled here - we rely on rcu_read_lock doing
@@ -186,7 +186,7 @@ static inline int page_cache_add_speculative(struct page *page, int count)
 
 #if !defined(CONFIG_SMP) && defined(CONFIG_TREE_RCU)
 # ifdef CONFIG_PREEMPT_COUNT
-	VM_BUG_ON(!in_atomic());
+	VM_BUG_ON(!in_atomic() && !irqs_disabled());
 # endif
 	VM_BUG_ON_PAGE(page_count(page) == 0, page);
 	page_ref_add(page, count);
-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1609956

From"Kirill A. Shutemov" <kirill@shutemov.name>
Date2017-03-27 17:40 +0200
Message-ID<tpEUq-1P8-25@gated-at.bofh.it>
In reply to#1608363
On Fri, Mar 24, 2017 at 02:47:09PM +0300, Kirill A. Shutemov wrote:
> 
> From d2f416a3ee3e5dbb10e59d0b374d382fdc4ba082 Mon Sep 17 00:00:00 2001
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Date: Fri, 24 Mar 2017 14:13:05 +0300
> Subject: [PATCH] mm: Fix false-positive VM_BUG_ON() in
>  page_cache_{get,add}_speculative
> 
> 0day triggered this:
> 
>   kernel BUG at include/linux/pagemap.h:151!
>   invalid opcode: 0000 [#1]
>   CPU: 0 PID: 458 Comm: trinity-c0 Not tainted 4.11.0-rc2-00251-g2947ba0 #1
>   task: ffff88001f19ab00 task.stack: ffff88001f084000
>   RIP: 0010:gup_pud_range+0x56f/0x63d
>   RSP: 0018:ffff88001f087ba8 EFLAGS: 00010046
>   RAX: 0000000080000000 RBX: 000000000164e000 RCX: ffff88001e0badc0
>   RDX: dead000000000100 RSI: 0000000000000001 RDI: ffff88001e0badc0
>   RBP: ffff88001f087c38 R08: ffff88001f087cf8 R09: ffff88001f087c6c
>   R10: 0000000000000000 R11: ffff88001f19b0f0 R12: ffff88001f087c6c
>   R13: ffff88001e0badc0 R14: 800000001e7b7867 R15: 0000000000000000
>   FS:  00007f7ea7b60700(0000) GS:ffffffffae02f000(0000) knlGS:0000000000000000
>   CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>   CR2: 00000000013eb130 CR3: 0000000017ddb000 CR4: 00000000000006f0
>   DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>   DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 00000000000d0602
>   Call Trace:
>    __get_user_pages_fast+0x107/0x136
>    get_user_pages_fast+0x78/0x89
>    get_futex_key+0xfd/0x350
>    ? simple_write_end+0x83/0xbe
>    futex_requeue+0x1a3/0x585
>    do_futex+0x834/0x86f
>    ? kvm_clock_read+0x16/0x1e
>    ? paravirt_sched_clock+0x9/0xd
>    ? lock_release+0x11e/0x328
>    SyS_futex+0x125/0x135
>    ? write_seqcount_end+0x1a/0x1f
>    ? vtime_account_user+0x4b/0x50
>    do_syscall_64+0x61/0x74
>    entry_SYSCALL64_slow_path+0x25/0x25
> 
> It' VM_BUG_ON() due to false-negative in_atomic(). We call
> page_cache_get_speculative() with disabled local interrupts.
> It should be atomic enough.
> 
> Let's check for disabled interrupts in the VM_BUG_ON() too.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Reported-by: Fengguang Wu <fengguang.wu@intel.com>

Ingo, could you get it applied along x86-gup transition?

Or do you see any problem with the patch?

> ---
>  include/linux/pagemap.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index f84cf5f76366..e7bbd9d4dc6c 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -148,7 +148,7 @@ static inline int page_cache_get_speculative(struct page *page)
>  
>  #ifdef CONFIG_TINY_RCU
>  # ifdef CONFIG_PREEMPT_COUNT
> -	VM_BUG_ON(!in_atomic());
> +	VM_BUG_ON(!in_atomic() && !irqs_disabled());
>  # endif
>  	/*
>  	 * Preempt must be disabled here - we rely on rcu_read_lock doing
> @@ -186,7 +186,7 @@ static inline int page_cache_add_speculative(struct page *page, int count)
>  
>  #if !defined(CONFIG_SMP) && defined(CONFIG_TREE_RCU)
>  # ifdef CONFIG_PREEMPT_COUNT
> -	VM_BUG_ON(!in_atomic());
> +	VM_BUG_ON(!in_atomic() && !irqs_disabled());
>  # endif
>  	VM_BUG_ON_PAGE(page_count(page) == 0, page);
>  	page_ref_add(page, count);
-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1610365 — [tip:x86/mm] mm: Fix false-positive VM_BUG_ON() in page_cache_{get,add}_speculative()

From"tip-bot for Kirill A. Shutemov" <tipbot@zytor.com>
Date2017-03-28 08:40 +0200
Subject[tip:x86/mm] mm: Fix false-positive VM_BUG_ON() in page_cache_{get,add}_speculative()
Message-ID<tpSXo-3CJ-27@gated-at.bofh.it>
In reply to#1608363
Commit-ID:  591a3d7c09fa08baff48ad86c2347dbd28a52753
Gitweb:     http://git.kernel.org/tip/591a3d7c09fa08baff48ad86c2347dbd28a52753
Author:     Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
AuthorDate: Fri, 24 Mar 2017 14:13:05 +0300
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 28 Mar 2017 08:23:27 +0200

mm: Fix false-positive VM_BUG_ON() in page_cache_{get,add}_speculative()

0day testing by Fengguang Wu triggered this crash while running Trinity:

  kernel BUG at include/linux/pagemap.h:151!
  ...
  CPU: 0 PID: 458 Comm: trinity-c0 Not tainted 4.11.0-rc2-00251-g2947ba0 #1
  ...
  Call Trace:
   __get_user_pages_fast()
   get_user_pages_fast()
   get_futex_key()
   futex_requeue()
   do_futex()
   SyS_futex()
   do_syscall_64()
   entry_SYSCALL64_slow_path()

It' VM_BUG_ON() due to false-negative in_atomic(). We call
page_cache_get_speculative() with disabled local interrupts.
It should be atomic enough.

So let's check for disabled interrupts in the VM_BUG_ON() condition
too, to resolve this.

( This got triggered by the conversion of the x86 GUP code to the
  generic GUP code. )

Reported-by: Fengguang Wu <fengguang.wu@intel.com>
Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
Cc: Kirill A. Shutemov <kirill@shutemov.name>
Cc: LKP <lkp@01.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-mm@kvack.org
Link: http://lkml.kernel.org/r/20170324114709.pcytvyb3d6ajux33@black.fi.intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/pagemap.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 84943e8..316a19f 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -148,7 +148,7 @@ static inline int page_cache_get_speculative(struct page *page)
 
 #ifdef CONFIG_TINY_RCU
 # ifdef CONFIG_PREEMPT_COUNT
-	VM_BUG_ON(!in_atomic());
+	VM_BUG_ON(!in_atomic() && !irqs_disabled());
 # endif
 	/*
 	 * Preempt must be disabled here - we rely on rcu_read_lock doing
@@ -186,7 +186,7 @@ static inline int page_cache_add_speculative(struct page *page, int count)
 
 #if !defined(CONFIG_SMP) && defined(CONFIG_TREE_RCU)
 # ifdef CONFIG_PREEMPT_COUNT
-	VM_BUG_ON(!in_atomic());
+	VM_BUG_ON(!in_atomic() && !irqs_disabled());
 # endif
 	VM_BUG_ON_PAGE(page_count(page) == 0, page);
 	page_ref_add(page, count);

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web