Messages in this thread | | | Date | Wed, 22 Jul 2020 15:44:06 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH -next] fork: silence a false postive warning in __mmdrop |
| |
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.
| |