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
