Re: [PATCH 2/2] mm/memory: Document how we make a coherent memory snapshot

[Jann Horn <jannh@google.com>]

On Wed, Jun 4, 2025 at 10:10 PM Peter Xu <peterx@redhat.com> wrote:
> On Wed, Jun 04, 2025 at 08:11:08PM +0200, Jann Horn wrote:
> > On Wed, Jun 4, 2025 at 7:04 PM Peter Xu <peterx@redhat.com> wrote:
> > > On Tue, Jun 03, 2025 at 08:21:03PM +0200, Jann Horn wrote:
> > > > It is not currently documented that the child of fork() should receive a
> > > > coherent snapshot of the parent's memory, or how we get such a snapshot.
> > > > Add a comment block to explain this.
> > > >
> > > > Signed-off-by: Jann Horn <jannh@google.com>
> > > > ---
> > > >  kernel/fork.c | 34 ++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 34 insertions(+)
> > > >
> > > > diff --git a/kernel/fork.c b/kernel/fork.c
> > > > index 85afccfdf3b1..f78f5df596a9 100644
> > > > --- a/kernel/fork.c
> > > > +++ b/kernel/fork.c
> > > > @@ -604,6 +604,40 @@ static void dup_mm_exe_file(struct mm_struct *mm, struct mm_struct *oldmm)
> > > >  }
> > > >
> > > >  #ifdef CONFIG_MMU
> > > > +/*
> > > > + * Anonymous memory inherited by the child MM must, on success, contain a
> > > > + * coherent snapshot of corresponding anonymous memory in the parent MM.
> > >
> > > Should we better define what is a coherent snapshot?  Or maybe avoid using
> > > this term which seems to apply to the whole mm?
> > >
> > > I think it's at least not a snapshot of whole mm at a specific time,
> > > because as long as there can be more than one concurrent writers (hence, it
> > > needs to be at least 3 threads in the parent process, 1 in charge of fork),
> > > this can happen:
> > >
> > >   parent writer 1      parent writer 2    parent fork thr
> > >   ---------------      ---------------    ---------------
> > >                                           wr-protect P1
> > >   write P1                                                  <---- T1
> > >   (trapped, didn't happen)
> > >                        write PN                             <---- T2
> > >                        (went through)
> > >                                           ...
> > >                                           wr-protect PN
> > >
> > > The result of above would be that child process will see a mixture of old
> > > P1 (at timestamp T1) but updated P2 (timestamp T2).  I don't think it's
> > > impossible that the userapp could try to serialize "write P1" and "write
> > > PN" operations in a way that it would also get a surprise seeing in the
> > > child PN updated but P1 didn't.
> >
> > If the write at T1 hits a page fault, then it doesn't actually happen
> > at T1. The write instruction starts doing something at T1, but it does
> > not fully retire, and the architectural register state does not
> > change, and in particular the instruction pointer does not advance
> > past this instruction; just like when speculative execution is aborted
> > after a branch misprediction, except that the CPU raises an exception
> > and we enter the page fault handler. The write actually happens when
> > the instruction is executed a second time after page fault handling
> > has completed after the mmap lock is dropped. (Unless something during
> > page fault handling raises a signal, in which case the instruction
> > might never architecturally execute.)
>
> Fair enough.  So maybe that's something like a best-effort whole mm
> snapshot anytime happened during the fork() but before releasing mmap write
> lock.
>
> Your comment did mention one exception on the kernel, is it still pretty
> easy to happen?  I'm thinking this use case of trying to load some data
> from a O_DIRECT fd and then set the var to show it's loaded:
>
>   bool data_read=0
>   read(...);
>   data_read=1;
>
> Then IIUC this can happen:
>
>     parent thread 1                        parent fork thr
>     ---------------                        ---------------
>     read(...)
>       using O_DIRECT on priv-anon buffers P1
>       pin_user_pages
>                                            fork() happens
>                                              Sees P1 pinned
>                                              P1 early COW (child sees no data loaded)
>       memcpy()
>     set data_read=1
>     (data_read can be a global private var on P2)
>                                              P2 wr-protected (child sees data_read=1)
>
> Hence in child even if it sees data_read=1 it is possible the buffer may be
> uninitialized, or the buffer is partly loaded, still racing with the kernel
> early COW.

Urgh. True, I had not considered that case.

> I'm not sure if I understand it correct this time as you discussed in the
> comment. If so, should we still not emphasize too much on the kernel
> providing coherent mm snapshot, at least emphasize the best-effort part
> (both in comment of patch 2, but also in patch subjects)?  After all, it
> seems it isn't straightforward for any userapp to see when that coherency
> will be violated.

Yeah, at least I should add a big caveat in this comment about how
O_DIRECT read buffers might be stale in such a case, and that we're
doing the best we can with a single-pass approach. Urgh.

> From that POV, maybe it's better we should still suggest the undefined
> behavior, even if it'll recover the old behavior some existing use case?