[-next] fork: silence a false postive warning in __mmdrop
diff mbox series

Message ID 20200604150344.1796-1-cai@lca.pw
State In Next
Commit 456eb20ceb120f59575764b979fedba9044d3926
Headers show
Series
  • [-next] fork: silence a false postive warning in __mmdrop
Related show

Commit Message

Qian Cai June 4, 2020, 3:03 p.m. UTC
The linux-next commit bf2c59fce407 ("sched/core: Fix illegal RCU from
offline CPUs") delayed,

idle->active_mm = &init_mm;

into finish_cpu() instead of idle_task_exit() which results in a false
positive warning that was originally designed in the commit 3eda69c92d47
("kernel/fork.c: detect early free of a live mm").

 WARNING: CPU: 127 PID: 72976 at kernel/fork.c:697
 __mmdrop+0x230/0x2c0
 do_exit+0x424/0xfa0
 Call Trace:
 do_exit+0x424/0xfa0
 do_group_exit+0x64/0xd0
 sys_exit_group+0x24/0x30
 system_call_exception+0x108/0x1d0
 system_call_common+0xf0/0x278

Fixes: bf2c59fce407 ("sched/core: Fix illegal RCU from offline CPUs")
Signed-off-by: Qian Cai <cai@lca.pw>
---
 kernel/fork.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Qian Cai July 21, 2020, 5:30 p.m. UTC | #1
On Thu, Jun 04, 2020 at 11:03:44AM -0400, Qian Cai wrote:
> The linux-next commit bf2c59fce407 ("sched/core: Fix illegal RCU from
> offline CPUs") delayed,
> 
> idle->active_mm = &init_mm;
> 
> into finish_cpu() instead of idle_task_exit() which results in a false
> positive warning that was originally designed in the commit 3eda69c92d47
> ("kernel/fork.c: detect early free of a live mm").
> 
>  WARNING: CPU: 127 PID: 72976 at kernel/fork.c:697
>  __mmdrop+0x230/0x2c0
>  do_exit+0x424/0xfa0
>  Call Trace:
>  do_exit+0x424/0xfa0
>  do_group_exit+0x64/0xd0
>  sys_exit_group+0x24/0x30
>  system_call_exception+0x108/0x1d0
>  system_call_common+0xf0/0x278
> 
> Fixes: bf2c59fce407 ("sched/core: Fix illegal RCU from offline CPUs")
> Signed-off-by: Qian Cai <cai@lca.pw>

Peter, Andrew, can you take a look at this trivial patch? The offensive commit
is now in the mainline, and it is quite easy to trigger this warning by a
non-root user.

# git clone https://gitlab.com/cailca/linux-mm
# cd linux-mm; make
# ./random -x 0-100 -f (it will switch to use a non-root user.)

[14933.435035][T1176230] ------------[ cut here ]------------
[14933.435062][T1176230] WARNING: CPU: 25 PID: 1176230 at kernel/fork.c:679 __mmdrop+0x1e0/0x270
[14933.435086][T1176230] Modules linked in: vfio_pci vfio_virqfd vfio_iommu_spapr_tce vfio vfio_spapr_eeh loop kvm_hv kvm ip_tables x_tables sd_mod tg3 firmware_class ahci libahci libphy libata dm_mirror dm_region_hash dm_log dm_mod
[14933.435176][T1176230] CPU: 25 PID: 1176230 Comm: trinity-c25 Not tainted 5.8.0-rc6-next-20200721 #5
[14933.435200][T1176230] NIP:  c0000000000c4560 LR: c0000000000d4fb4 CTR: 0000000000000000
[14933.435223][T1176230] REGS: c00000001520f910 TRAP: 0700   Not tainted  (5.8.0-rc6-next-20200721)
[14933.435255][T1176230] MSR:  9000000000029033 <SF,HV,EE,ME,IR,DR,RI,LE>  CR: 24002822  XER: 20040000
[14933.435294][T1176230] CFAR: c0000000000c43f4 IRQMASK: 0 
[14933.435294][T1176230] GPR00: c0000000000d4fb4 c00000001520fba0 c000000005911500 c000000f6e227080 
[14933.435294][T1176230] GPR04: 0000000000000001 fffffffffffff026 c0000007aa2a17e0 0000000000000001 
[14933.435294][T1176230] GPR08: 0000000000000000 c000000f6e227080 0000000000000001 bfffffffffffffff 
[14933.435294][T1176230] GPR12: 0000000000002000 c000000ffffeb780 0000000010037be0 00000000109ec8e0 
[14933.435294][T1176230] GPR16: 00000000109eccb0 0000000010037ff0 0000000000000001 00007fff8d000000 
[14933.435294][T1176230] GPR20: 00000000100380d0 00000000100380a8 000000001003a1d0 0000000010037f10 
[14933.435294][T1176230] GPR24: 00007fff8df100e0 00000000ed54649a c000000f6e227128 c00000001520fcc8 
[14933.435294][T1176230] GPR28: c000000fe7540890 c000000f6e227080 c000000fe7540080 c000000f6e227080 
[14933.435470][T1176230] NIP [c0000000000c4560] __mmdrop+0x1e0/0x270
[14933.435493][T1176230] LR [c0000000000d4fb4] do_exit+0x424/0xee0
[14933.435514][T1176230] Call Trace:
[14933.435524][T1176230] [c00000001520fba0] [c00c0000000164c0] 0xc00c0000000164c0 (unreliable)
[14933.435550][T1176230] [c00000001520fc60] [c0000000000d4fb4] do_exit+0x424/0xee0
[14933.435575][T1176230] [c00000001520fd60] [c0000000000d5b2c] do_group_exit+0x5c/0xd0
[14933.435600][T1176230] [c00000001520fda0] [c0000000000d5bbc] sys_exit_group+0x1c/0x20
[14933.435626][T1176230] [c00000001520fdc0] [c00000000002c978] system_call_exception+0xf8/0x180
[14933.435654][T1176230] [c00000001520fe20] [c00000000000c9e8] system_call_common+0xe8/0x214
[14933.435686][T1176230] Instruction dump:
[14933.435706][T1176230] 480c35e9 60000000 4bffff68 60000000 7c832378 38800000 482a8d81 60000000 
[14933.435750][T1176230] 4bfffee8 60000000 60000000 60000000 <0fe00000> 4bfffe94 60000000 60000000 
[14933.435795][T1176230] irq event stamp: 370444
[14933.435817][T1176230] hardirqs last  enabled at (370443): [<c0000000003b8964>] __slab_free+0x2e4/0x5e0
[14933.435868][T1176230] hardirqs last disabled at (370444): [<c00000000000964c>] program_check_common_virt+0x2bc/0x310
[14933.435913][T1176230] softirqs last  enabled at (352366): [<c0000000009b0790>] __do_softirq+0x650/0x920
[14933.435948][T1176230] softirqs last disabled at (352359): [<c0000000000d70dc>] irq_exit+0x17c/0x1b0
[14933.435971][T1176230] ---[ end trace 5823e9bcf4dee099 ]---

> ---
>  kernel/fork.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 142b23645d82..5334efd2a680 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -694,7 +694,6 @@ void __mmdrop(struct mm_struct *mm)
>  {
>  	BUG_ON(mm == &init_mm);
>  	WARN_ON_ONCE(mm == current->mm);
> -	WARN_ON_ONCE(mm == current->active_mm);
>  	mm_free_pgd(mm);
>  	destroy_context(mm);
>  	mmu_notifier_subscriptions_destroy(mm);
> -- 
> 2.21.0 (Apple Git-122.2)
>
Peter Zijlstra July 22, 2020, 10:06 a.m. UTC | #2
On Thu, Jun 04, 2020 at 11:03:44AM -0400, Qian Cai wrote:
> The linux-next commit bf2c59fce407 ("sched/core: Fix illegal RCU from
> offline CPUs") delayed,
> 
> idle->active_mm = &init_mm;
> 
> into finish_cpu() instead of idle_task_exit() which results in a false
> positive warning that was originally designed in the commit 3eda69c92d47
> ("kernel/fork.c: detect early free of a live mm").
> 
>  WARNING: CPU: 127 PID: 72976 at kernel/fork.c:697
>  __mmdrop+0x230/0x2c0
>  do_exit+0x424/0xfa0
>  Call Trace:
>  do_exit+0x424/0xfa0
>  do_group_exit+0x64/0xd0
>  sys_exit_group+0x24/0x30
>  system_call_exception+0x108/0x1d0
>  system_call_common+0xf0/0x278

Please explain; because afaict this is a use-after-free.

The thing is __mmdrop() is going to actually free the mm, so then what
is finish_cpu()'s mmdrop() going to do?

->active_mm() should have a refcount on the mm.

> Fixes: bf2c59fce407 ("sched/core: Fix illegal RCU from offline CPUs")
> Signed-off-by: Qian Cai <cai@lca.pw>
> ---
>  kernel/fork.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 142b23645d82..5334efd2a680 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -694,7 +694,6 @@ void __mmdrop(struct mm_struct *mm)
>  {
>  	BUG_ON(mm == &init_mm);
>  	WARN_ON_ONCE(mm == current->mm);
> -	WARN_ON_ONCE(mm == current->active_mm);
>  	mm_free_pgd(mm);
>  	destroy_context(mm);
>  	mmu_notifier_subscriptions_destroy(mm);
> -- 
> 2.21.0 (Apple Git-122.2)
>
Qian Cai July 22, 2020, 1:19 p.m. UTC | #3
On Wed, Jul 22, 2020 at 12:06:37PM +0200, peterz@infradead.org wrote:
> On Thu, Jun 04, 2020 at 11:03:44AM -0400, Qian Cai wrote:
> > The linux-next commit bf2c59fce407 ("sched/core: Fix illegal RCU from
> > offline CPUs") delayed,
> > 
> > idle->active_mm = &init_mm;
> > 
> > into finish_cpu() instead of idle_task_exit() which results in a false
> > positive warning that was originally designed in the commit 3eda69c92d47
> > ("kernel/fork.c: detect early free of a live mm").
> > 
> >  WARNING: CPU: 127 PID: 72976 at kernel/fork.c:697
> >  __mmdrop+0x230/0x2c0
> >  do_exit+0x424/0xfa0
> >  Call Trace:
> >  do_exit+0x424/0xfa0
> >  do_group_exit+0x64/0xd0
> >  sys_exit_group+0x24/0x30
> >  system_call_exception+0x108/0x1d0
> >  system_call_common+0xf0/0x278
> 
> Please explain; because afaict this is a use-after-free.
> 
> The thing is __mmdrop() is going to actually free the mm, so then what
> is finish_cpu()'s mmdrop() going to do?
> 
> ->active_mm() should have a refcount on the mm.

Well, the refcount issue you mentioned then happens all before bf2c59fce407 was
introduced as well, but then it looks harmless because mmdrop() in finish_cpu()
will do,

	if (unlikely(atomic_dec_and_test(&mm->mm_count)))
		__mmdrop(mm);

where that atomic_dec_and_test() see the negative refcount and will not involve
__mmdrop() again. It is not clear to me that once the CPU is offline if it
needs to care about its idle thread mm_count at all. Even if this refcount
issue is finally addressed, it could hit this warning in finish_cpu() without
this patch.

On the other hand, if you look at the commit 3eda69c92d47, it is clearly that
the assumption of,

   WARN_ON_ONCE(mm == current->active_mm);

is totally gone due to bf2c59fce407. Thus, the patch is to fix that discrepancy
first and then I'll look at that the imbalance mmdrop()/mmgrab() elsewhere.

> 
> > Fixes: bf2c59fce407 ("sched/core: Fix illegal RCU from offline CPUs")
> > Signed-off-by: Qian Cai <cai@lca.pw>
> > ---
> >  kernel/fork.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index 142b23645d82..5334efd2a680 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -694,7 +694,6 @@ void __mmdrop(struct mm_struct *mm)
> >  {
> >  	BUG_ON(mm == &init_mm);
> >  	WARN_ON_ONCE(mm == current->mm);
> > -	WARN_ON_ONCE(mm == current->active_mm);
> >  	mm_free_pgd(mm);
> >  	destroy_context(mm);
> >  	mmu_notifier_subscriptions_destroy(mm);
> > -- 
> > 2.21.0 (Apple Git-122.2)
> >
Peter Zijlstra July 22, 2020, 1:44 p.m. UTC | #4
On Wed, Jul 22, 2020 at 09:19:00AM -0400, Qian Cai wrote:
> On Wed, Jul 22, 2020 at 12:06:37PM +0200, peterz@infradead.org wrote:
> > On Thu, Jun 04, 2020 at 11:03:44AM -0400, Qian Cai wrote:
> > > The linux-next commit bf2c59fce407 ("sched/core: Fix illegal RCU from
> > > offline CPUs") delayed,
> > > 
> > > idle->active_mm = &init_mm;
> > > 
> > > into finish_cpu() instead of idle_task_exit() which results in a false
> > > positive warning that was originally designed in the commit 3eda69c92d47
> > > ("kernel/fork.c: detect early free of a live mm").
> > > 
> > >  WARNING: CPU: 127 PID: 72976 at kernel/fork.c:697
> > >  __mmdrop+0x230/0x2c0
> > >  do_exit+0x424/0xfa0
> > >  Call Trace:
> > >  do_exit+0x424/0xfa0
> > >  do_group_exit+0x64/0xd0
> > >  sys_exit_group+0x24/0x30
> > >  system_call_exception+0x108/0x1d0
> > >  system_call_common+0xf0/0x278
> > 
> > Please explain; because afaict this is a use-after-free.
> > 
> > The thing is __mmdrop() is going to actually free the mm, so then what
> > is finish_cpu()'s mmdrop() going to do?
> > 
> > ->active_mm() should have a refcount on the mm.
> 
> Well, the refcount issue you mentioned then happens all before bf2c59fce407 was
> introduced as well, but then it looks harmless because mmdrop() in finish_cpu()
> will do,
> 
> 	if (unlikely(atomic_dec_and_test(&mm->mm_count)))
> 		__mmdrop(mm);

That's not harmless, that's a use-after-free. Those can cause memory
corruption bugs and the like at best. Who knows what's at the location
of mm->mm_count after we've already freed it.

> where that atomic_dec_and_test() see the negative refcount and will not involve
> __mmdrop() again. It is not clear to me that once the CPU is offline if it
> needs to care about its idle thread mm_count at all. Even if this refcount
> issue is finally addressed, it could hit this warning in finish_cpu() without
> this patch.
> 
> On the other hand, if you look at the commit 3eda69c92d47, it is clearly that
> the assumption of,
> 
>    WARN_ON_ONCE(mm == current->active_mm);
> 
> is totally gone due to bf2c59fce407. Thus, the patch is to fix that discrepancy
> first and then I'll look at that the imbalance mmdrop()/mmgrab() elsewhere.

No, you're talking nonsense. We must not free @mm when
'current->active_mm == mm', never.

Patch
diff mbox series

diff --git a/kernel/fork.c b/kernel/fork.c
index 142b23645d82..5334efd2a680 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -694,7 +694,6 @@  void __mmdrop(struct mm_struct *mm)
 {
 	BUG_ON(mm == &init_mm);
 	WARN_ON_ONCE(mm == current->mm);
-	WARN_ON_ONCE(mm == current->active_mm);
 	mm_free_pgd(mm);
 	destroy_context(mm);
 	mmu_notifier_subscriptions_destroy(mm);