Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 22 Jul 2020 15:44:06 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Qian Cai <cai@lca.pw>
Cc:     mark.rutland@arm.com, akpm@linux-foundation.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH -next] fork: silence a false postive warning in __mmdrop
Message-ID: <20200722134406.GN10769@hirez.programming.kicks-ass.net>
References: <20200604150344.1796-1-cai@lca.pw>
 <20200722100637.GR119549@hirez.programming.kicks-ass.net>
 <20200722131900.GA4041@lca.pw>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200722131900.GA4041@lca.pw>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.