[CRIU] [PATCH] restore: Serialize access to last_pid

[Dmitry Safonov]

On Fri, 15 Nov 2019 at 14:35, Cyrill Gorcunov <gorcunov at gmail.com> wrote:
>  When we do clone threads in a later stage of
>  restore procedure it may race with helpers
>  which do call clone_noasan by self.
>
>  In particular when restoring mount points
>  we do use try_remount_writable which is called
>  via call_helper_process and produce own fork.

try_remount_writable() is called from prepare_remaps() which is called only
in root_prepare_shared(). Could you produce a graph for the race?

===========================================
            THREAD 1                                THREAD 2
------------------------------------     -------------------------------------
                   ?                                                 ?
   try_remount_writable()                  clone_noasan()

Hmm?

[..]
> To be honest I have no clue right now how to
> provide some kind of test for this stuff.

Fault-injection?

[..]
>  criu/clone-noasan.c         | 63 ++++++++++++++++++++++++++++++++++++-

Please, keep only one function there - the file is compiled without sanitizer
instrumentation, so it's better to avoid inflating the file with new code.

[..]
> @@ -23,9 +80,13 @@ int clone_noasan(int (*fn)(void *), int flags, void *arg)
>  {
>         void *stack_ptr = (void *)round_down((unsigned long)&stack_ptr - 1024, 16);
>         BUG_ON((flags & CLONE_VM) && !(flags & CLONE_VFORK));
> +       int ret;
>         /*
>          * Reserve some bytes for clone() internal needs
>          * and use as stack the address above this area.
>          */
> -       return clone(fn, stack_ptr, flags, arg);
> +       context_lock();
> +       ret = clone(fn, stack_ptr, flags, arg);
> +       context_unlock();
> +       return ret;
>  }

I don't understand - you need to make an atomic section between
write(last_pid_fd, ...) and clone(), not just during clone().

--
             Dmitry