On Thu, Sep 01, 2022 at 01:11:37PM -0500, Eric W. Biederman wrote:
> Andrei Vagin <avagin@xxxxxxxxx> writes:
>
> > On Tue, Aug 30, 2022 at 6:18 PM Andrei Vagin <avagin@xxxxxxxxx> wrote:
> >>On Tue, Aug 30, 2022 at 10:49:43PM +0300, Alexey Izbyshev wrote:
> > <snip>
> >>> @@ -1030,6 +1033,10 @@ static int exec_mmap(struct mm_struct *mm)
> >>> tsk->mm->vmacache_seqnum = 0;
> >>> vmacache_flush(tsk);
> >>> task_unlock(tsk);
> >>> +
> >>> + if (vfork)
> >>> + timens_on_fork(tsk->nsproxy, tsk);
> >>> +
> >>>
> >>> Similarly, even after a normal vfork(), time namespace switch could be
> >>> silently skipped if the parent dies before "tsk->vfork_done" is read. Again,
> >>> I don't know whether anybody cares, but this behavior seems non-obvious and
> >>> probably unintended to me.
> >> This is the more interesting case. I will try to find out how we can
> >> handle it properly.
> >
> > It might not be a good idea to use vfork_done in this case. Let's
> > think about what we have and what we want to change. We don't want to
> > allow switching timens if a process mm is used by someone else. But we
> > forgot to handle execve that creates a new mm, and we can't change this
> > behavior right now because it can affect current users. Right?
>
> What we can't changes are things that will break existing programs. If
> existing programs don't care we can change the behavior of the kernel.
I agree that it is very unlikely that anyone will notice
these changes. And it is hard to imagine that anyone uses the old
behavior intentionally.
>
> > So maybe the best choice, in this case, is to change behavior by adding
> > a new control that enables it. The first interface that comes to my mind
> > is to introduce a new ioctl for a namespace file descriptor. Here is a
> > draft patch below that should help to understand what I mean.
>
> I don't think adding a new control works, because programs that are
> calling vfork or posix_spawn today will stop working.
>
> We should recognize that basing things off of CLONE_VFORK was a bad idea
> as CLONE_VFORK is all about waiting for the created task to exec or
> exit, and really has nothing to do with creating a new mm.
>
> Instead I think the rule should be that a new time namespaces is
> installed as soon as we have a new mm.
>
> That will be a behavioral change if the time ns is unshared and then the
> program exec's instead of forking children, but I suspect it is the
> proper behavior all the same, and that existing userspace won't care.
> Especially since all of the vfork_done work is new behavior as
> of v6.0-rc1.
>
> Ugh. I just spotted another bug. The function timens_on_fork as
> written is not safe to call without first creating a fresh copy
> of the nsproxy, and we don't do that during exec. Because nsproxy
> is shared between tasks and processes updating the values needs to
> create a new nsproxy or other tasks/processes can be affected.
> Not hard to handle just something that needs to be addressed.
You are right. Thanks.
>
> Say something like this:
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 9a5ca7b82bfc..8a6947e631dd 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -979,12 +979,10 @@ static int exec_mmap(struct mm_struct *mm)
> {
> struct task_struct *tsk;
> struct mm_struct *old_mm, *active_mm;
> - bool vfork;
> int ret;
>
> /* Notify parent that we're no longer interested in the old VM */
> tsk = current;
> - vfork = !!tsk->vfork_done;
> old_mm = current->mm;
> exec_mm_release(tsk, old_mm);
> if (old_mm)
> @@ -1030,9 +1028,6 @@ static int exec_mmap(struct mm_struct *mm)
> vmacache_flush(tsk);
> task_unlock(tsk);
>
> - if (vfork)
> - timens_on_fork(tsk->nsproxy, tsk);
> -
> if (old_mm) {
> mmap_read_unlock(old_mm);
> BUG_ON(active_mm != old_mm);
> @@ -1303,6 +1298,10 @@ int begin_new_exec(struct linux_binprm * bprm)
>
> bprm->mm = NULL;
>
> + retval = exec_task_namespaces();
> + if (retval)
> + goto out_unlock;
> +
> #ifdef CONFIG_POSIX_TIMERS
> spin_lock_irq(&me->sighand->siglock);
> posix_cpu_timers_exit(me);
> diff --git a/include/linux/nsproxy.h b/include/linux/nsproxy.h
> index cdb171efc7cb..fee881cded01 100644
> --- a/include/linux/nsproxy.h
> +++ b/include/linux/nsproxy.h
> @@ -94,6 +94,7 @@ static inline struct cred *nsset_cred(struct nsset *set)
> int copy_namespaces(unsigned long flags, struct task_struct *tsk);
> void exit_task_namespaces(struct task_struct *tsk);
> void switch_task_namespaces(struct task_struct *tsk, struct nsproxy *new);
> +int exec_task_namespaces(void);
> void free_nsproxy(struct nsproxy *ns);
> int unshare_nsproxy_namespaces(unsigned long, struct nsproxy **,
> struct cred *, struct fs_struct *);
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 90c85b17bf69..b4a799d9c50f 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2043,18 +2043,6 @@ static __latent_entropy struct task_struct *copy_process(
> return ERR_PTR(-EINVAL);
> }
>
> - /*
> - * If the new process will be in a different time namespace
> - * do not allow it to share VM or a thread group with the forking task.
> - *
> - * On vfork, the child process enters the target time namespace only
> - * after exec.
> - */
> - if ((clone_flags & (CLONE_VM | CLONE_VFORK)) == CLONE_VM) {
> - if (nsp->time_ns != nsp->time_ns_for_children)
> - return ERR_PTR(-EINVAL);
> - }
pls don't remove this part. It was one of the concerns that vfork
doesn't work after unshare(CLONE_NEWTIME), but it is one of the standard
ways of creating a new process. For example, posix_spawn uses it.
> -
> if (clone_flags & CLONE_PIDFD) {
> /*
> * - CLONE_DETACHED is blocked so that we can potentially
> diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
> index b4cbb406bc28..b6647846fe42 100644
> --- a/kernel/nsproxy.c
> +++ b/kernel/nsproxy.c
> @@ -255,6 +255,24 @@ void exit_task_namespaces(struct task_struct *p)
> switch_task_namespaces(p, NULL);
> }
>
> +int exec_task_namespaces(void)
> +{
> + struct task_struct *tsk = current;
> + struct nsproxy *new;
> +
> + if (tsk->nsproxy->time_ns_for_children == tsk->nsproxy->time_ns)
> + return 0;
> +
> + new = create_new_namespaces(0, tsk, current_user_ns(), tsk->fs);
> + if (IS_ERR(new))
> + return PTR_ERR(new);
> +
> + timens_on_fork(new, tsk);
> + switch_task_namespaces(tsk, new);
> + return 0;
> +}
> +
> +
> static int check_setns_flags(unsigned long flags)
> {
> if (!flags || (flags & ~(CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
>
>
>
> To keep things from being too confusing it probably makes sense to
> rename the nsproxy variable from time_ns_for_children to
> time_ns_for_new_mm. Likewise timens_on_fork can be renamed
> timens_on_new_mm.
>
> But that would be follow up work.
>
> How does the above change sound to folks?
It looks good to me.
Thanks,
Andrei
