Message-ID: <da95e333b4c508ddf8130f8f2d2cbb92@ispras.ru>
Date:   Wed, 07 Sep 2022 08:33:20 +0300
From:   Alexey Izbyshev <izbyshev@...ras.ru>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Andrei Vagin <avagin@...il.com>,
        Florian Weimer <fweimer@...hat.com>,
        Christian Brauner <brauner@...nel.org>,
        Dmitry Safonov <0x7f454c46@...il.com>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Kees Cook <keescook@...omium.org>
Subject: Re: Potentially undesirable interactions between vfork() and time
 namespaces

On 2022-09-07 01:16, Eric W. Biederman wrote:
> Alexey Izbyshev <izbyshev@...ras.ru> writes:
> 
>> On 2022-09-01 21:11, Eric W. Biederman wrote:
>>> Andrei Vagin <avagin@...il.com> writes:
>>> 
>>>> On Tue, Aug 30, 2022 at 6:18 PM Andrei Vagin <avagin@...il.com> 
>>>> 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.
>>> 
>>>> 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.
>>> 
>> While vfork_done work is indeed new, preservation of 
>> time_ns_for_children on
>> execve() instead of switching to it is how time namespaces were 
>> originally
>> implemented in 5.6. If this can be changed even now, thereby fixing 
>> the original
>> design, that's great, I just want to point out that it's not the 
>> recent 6.0 work
>> that is being fixed. Fixes/clarifications for man pages[1][2], which 
>> talk about
>> "subsequently created children", will also be needed.
>> 
>> [1] https://man7.org/linux/man-pages/man7/time_namespaces.7.html
>> [2] https://man7.org/linux/man-pages/man2/unshare.2.html
> 
> Sorry, yes.
> 
> That is something to be double checked.
> 
> I can't see where it would make sense to unshare a time namespace and
> then call exec, instead of calling exit.  So I suspect we can just
> change this behavior and no one will notice.
> 
One can imagine a helper binary that calls unshare, forks some children 
in new namespaces, and then calls exec to hand off actual work to 
another binary (which might not expect being in the new time namespace). 
I'm purely theorizing here, however. Keeping a special case for vfork() 
based only on FUD is likely a net negative, so it'd be nice to hear 
actual time namespace users speak up, and switch to the solution you 
suggested if they don't care.

The "unshare" tool from util-linux will also change behavior if called 
without "--fork" (e.g. "unshare --user --time"), but that would be 
unusual usage (just as for "--pid"), so most people probably don't do 
that (or don't care about the time namespace of the exec'ed process, but 
care only about its children).

>>> 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.
>>> 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);
>>> -	}
>>> -
>>>  	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.
>>> 
>> Do you imply renaming "/proc/[pid]/ns/time_for_children" as well, or 
>> will it be
>> preserved for compatibility?
> 
> Unfortunately I don't think we can change that one.  We could add
> another better named one, update the tools to use it.  Then wait a
> couple of millenia and remove the current name.  Depending it might be
> worth it, but only if you have a lot of patience.
> 
I agree with you and Andrei that the name in /proc shouldn't be changed. 
I was asking only to understand the scope of changes that you suggested.

> We should get the implementation details sorted out first, and the
> in-kernel name before touching the proc files.
> 
FWIW, your patch looks good to me. I've also run some simple manual 
tests with it applied on top of 6.0.0-rc4, and it works as expected.

I've also noticed one missed optimization in copy_namespaces().

    if (likely(!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
                  CLONE_NEWPID | CLONE_NEWNET |
                  CLONE_NEWCGROUP | CLONE_NEWTIME)))) {
        if (likely(old_ns->time_ns_for_children == old_ns->time_ns)) {
            get_nsproxy(old_ns);
            return 0;
        }
    } else if (!ns_capable(user_ns, CAP_SYS_ADMIN))
        return -EPERM;

The time ns comparison on the fast path was originally added together 
with time namespace support, and back then clone(CLONE_VM) wasn't 
allowed with non-matching time_ns and time_ns_for_children. Then 
Andrei's patch 133e2d3e81 allowed clone(CLONE_VM|CLONE_VFORK) in this 
case, and your patch removes CLONE_VM restriction altogether, so 
non-matching time_ns/time_ns_for_children are simply inherited if 
CLONE_VM is set. However, the fast path didn't learn about that, so 
copy_namespaces() will uselessly create a new nsproxy even though 
timens_on_fork() won't be called. Probably the fast path check should be 
fixed.

Thanks,
Alexey

Powered by blists - more mailing lists