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

[Barry Song]

On Wed, Oct 28, 2026 at 8:55 PM Pedro Falcato <pfalcato@xxxxxxx> wrote:
>
> 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().

Yes. I was suggesting something like using data_race() to
silence the warning[1], but applying it to memcpy() also
seems a bit too drastic.

Another option is to copy members one by one, skipping those
that don't need to be copied. However, that would make the
code quite lengthy, and the copying might also be slower?

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

Yep, I don't think there are any blockers for this patch. I had
actually given my Reviewed-by tag before raising this race.

David discussed races between memcpy() of mm_struct
and other operations. I recalled another related case, so I
posted it here. Hopefully, I'm not just making noise.

[1] https://lore.kernel.org/linux-mm/CAGsJ_4xnM66UZ-n55AHW4UHn+PSCP7G-gGUHUfSt6MRZGeRnfw@xxxxxxxxxxxxxx/

Best Regards
Barry