Re: [PATCH v2 1/2] mmap locking API: Order lock of nascent mm outside lock of live mm

[Johannes Berg <johannes@sipsolutions.net>]

Hi Jann,

> > > +++ b/arch/um/include/asm/mmu_context.h
> > > @@ -48,9 +48,8 @@ static inline void activate_mm(struct mm_struct *old, struct mm_struct *new)
> > >        * when the new ->mm is used for the first time.
> > >        */
> > >       __switch_mm(&new->context.id);
> > > -     mmap_write_lock_nested(new, SINGLE_DEPTH_NESTING);
> > > +     mmap_assert_write_locked(new);
> > >       uml_setup_stubs(new);
> > > -     mmap_write_unlock(new);
> > >  }
> > 
> > FWIW, this was I believe causing lockdep issues.
> > 
> > I think I had previously determined that this was pointless, since it's
> > still nascent and cannot be used yet?
> 
> Well.. the thing is that with patch 2/2, I'm not just protecting the
> mm while it hasn't been installed yet, but also after it's been
> installed, until setup_arg_pages() is done (which still uses a VMA
> pointer that we obtained really early in the nascent phase). 

Oh, sure. I was referring only to the locking in UML's activate_mm(),
quoted above. Sorry for not making that clear.

> So in summary, I think the code currently is not (visibly) buggy in
> the sense that you can make it do something bad, but it's extremely
> fragile and probably only safe by chance. This patchset is partly my
> attempt to make this a bit more future-proof before someone comes
> along and turns it into an actual memory corruption bug with some
> innocuous little change. (Because I've had a situation before where I
> thought "oh, this looks really fragile and only works by chance, but
> eh, it's not really worth changing that code" and then the next time I
> looked, it had turned into a security bug that had already made its
> way into kernel releases people were using.)

Right.

> > But I didn't really know for sure,
> > and this patch was never applied:
> > 
> > https://patchwork.ozlabs.org/project/linux-um/patch/20200604133752.397dedea0758.I7a24aaa26794eb3fa432003c1bf55cbb816489e2@changeid/
> 
> Eeeh... with all the kernel debugging infrastructure *disabled*,

but I didn't have it disabled, I had lockdep enabled, and lockdep was
complaining (now granted, I was still on 5.8 for that patch):

=============================
[ BUG: Invalid wait context ]
5.8.0-00006-gef4b340c886a #23 Not tainted
-----------------------------
swapper/1 is trying to lock:
000000006e54c160 (&mm->mmap_lock/1){....}-{3:3}, at: begin_new_exec+0x6c5/0xb26
other info that might help us debug this:
context-{4:4}
3 locks held by swapper/1:
 #0: 00000000705f4548 (&sig->cred_guard_mutex){+.+.}-{3:3}, at: __do_execve_file+0x12c/0x7ea
 #1: 00000000705f45e0 (&sig->exec_update_mutex){+.+.}-{3:3}, at: begin_new_exec+0x5db/0xb26
 #2: 00000000705e05a8 (&p->alloc_lock){+.+.}-{2:2}, at: begin_new_exec+0x66b/0xb26
stack backtrace:
CPU: 0 PID: 1 Comm: swapper Not tainted 5.8.0-00006-gef4b340c886a #23
Stack:
 6057fa2d 705e0760 705ebbb0 00000133
 6008d289 705e0760 705e0040 00000003
 705ebbc0 6028e02f 705ebc50 60080b29
Call Trace:
 [<6008d289>] ? printk+0x0/0x94
 [<60024a1a>] show_stack+0x153/0x174
 [<6008d289>] ? printk+0x0/0x94
 [<6028e02f>] dump_stack+0x34/0x36
 [<60080b29>] __lock_acquire+0x515/0x15f5
 [<6007c593>] ? hlock_class+0x0/0xa1
 [<6007fd90>] lock_acquire+0x347/0x42d
 [<6013def5>] ? begin_new_exec+0x6c5/0xb26
 [<60039b51>] ? set_signals+0x29/0x3f
 [<600835c1>] ? lock_acquired+0x310/0x320
 [<6013b5ce>] ? would_dump+0x0/0x8a
 [<600798fd>] down_write_nested+0x2f/0x83
 [<6013def5>] ? begin_new_exec+0x6c5/0xb26
 [<600798ce>] ? down_write_nested+0x0/0x83
 [<6013def5>] begin_new_exec+0x6c5/0xb26
 [<6019593b>] ? load_elf_phdrs+0x6f/0x9d
 [<60298d55>] ? memcmp+0x0/0x20
 [<60196612>] load_elf_binary+0x2cb/0xc49
 [...]

but it really looks just about the same on v5.9-rc8.

> > I guess your patches will also fix the lockdep complaints in UML in this
> > area, I hope I'll be able to test it soon.
> 
> That would be a nice side effect. :)

It does indeed fix it :)

johannes