Re: [PATCH 02/46] fork: take mmap_lock before copying mm_struct

[Pedro Falcato]

On Wed, Oct 28, 2026 at 03:48:09AM +0800, Barry Song wrote:
> On Wed, Oct 28, 2026 at 3:07 AM David Hildenbrand (Arm)
> <david@xxxxxxxxxx> wrote:
> >
> > On 10/27/26 11:38, Pedro Falcato wrote:
> > > On Tue, Oct 27, 2026 at 12:17:22AM +0100, David Hildenbrand (Arm) wrote:
> > >> dup_mm() copies the parent's mm_struct before dup_mmap() takes the
> > >> mmap_lock. A concurrent brk() can leave the child with a stale mm->brk
> > >> and newer heap mappings, causing later brk() requests to fail.
> > >>
> > >> Other operations (e.g., PR_SET_MM) on the parent's mm_struct can similarly
> > >> cause inconsistencies.
> > >>
> > >> As a simple and robust fix, grab the parent's mmap_lock earlier. This
> > >> way we don't need to update individual mm fields in dup_mmap() and
> > >> keep missing ones. Stop updating the 4 existing members (total_vm, data_vm,
> > >> exec_vm, stack_vm), as that is now no longer required.
> > >>
> > >> Keep releasing the parent's mmap_lock in dup_mmap(), before
> > >> userfaultfd completion, to avoid moving the userfaultfd handling into the
> > >> caller. This should get cleaned up later.
> > >>
> > >> Move uprobe_start_dup_mmap() to preserve the lock ordering.
> > >>
> > >> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > >> Cc: stable@xxxxxxxxxxxxxxx
> > >> Reported-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
> > >> Reviewed-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>
> > >> Signed-off-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
> > >
> > > Reviewed-by: Pedro Falcato <pfalcato@xxxxxxx>
> > >
> > > FWIW, strictly speaking malloc() isn't usable in a forked
> > > (multi-threaded) child, but glibc/musl provide this in a best-effort way.
> >
> > Yeah, I'm more concerned about other things that can race ... :)
> 
> Regarding the race with memcpy(mm, oldmm, sizeof(*mm)), we
> still have one [1] even after this patch.
> 
> That is, copying mm_struct can happen concurrently with
> lru_gen_del_mm(). However, this race doesn't seem harmful.
> I might be missing something here though.

I don't think it's harmful as well. mm_init() reinitializes those fields.

Though I hate that memcpy. I think that ideally we copy by member and
data_race() benign races, like what is done in vm_area_init_from().

Anyway, KCSAN reports aren't the most serious thing in the world, so this can
(should?) be cleaned up later.

-- 
Pedro