Re: [PATCH 1/2] userfaultfd: use RCU to free the task struct when fork fails

[Kirill Tkhai <ktkhai@virtuozzo.com>]

On 26.03.2019 11:18, Kirill Tkhai wrote:
> On 26.03.2019 11:07, Kirill Tkhai wrote:
>> On 26.03.2019 01:56, Andrea Arcangeli wrote:
>>> MEMCG depends on the task structure not to be freed under
>>> rcu_read_lock() in get_mem_cgroup_from_mm() after it dereferences
>>> mm->owner.
>>>
>>> An alternate possible fix would be to defer the delivery of the
>>> userfaultfd contexts to the monitor until after fork() is guaranteed
>>> to succeed. Such a change would require more changes because it would
>>> create a strict ordering dependency where the uffd methods would need
>>> to be called beyond the last potentially failing branch in order to be
>>> safe. This solution as opposed only adds the dependency to common code
>>> to set mm->owner to NULL and to free the task struct that was pointed
>>> by mm->owner with RCU, if fork ends up failing. The userfaultfd
>>> methods can still be called anywhere during the fork runtime and the
>>> monitor will keep discarding orphaned "mm" coming from failed forks in
>>> userland.
>>>
>>> This race condition couldn't trigger if CONFIG_MEMCG was set =n at
>>> build time.
>>>
>>> Fixes: 893e26e61d04 ("userfaultfd: non-cooperative: Add fork() event")
>>> Cc: stable@kernel.org
>>> Tested-by: zhong jiang <zhongjiang@huawei.com>
>>> Reported-by: syzbot+cbb52e396df3e565ab02@syzkaller.appspotmail.com
>>> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
>>> ---
>>>  kernel/fork.c | 34 ++++++++++++++++++++++++++++++++--
>>>  1 file changed, 32 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/kernel/fork.c b/kernel/fork.c
>>> index 9dcd18aa210b..a19790e27afd 100644
>>> --- a/kernel/fork.c
>>> +++ b/kernel/fork.c
>>> @@ -952,6 +952,15 @@ static void mm_init_aio(struct mm_struct *mm)
>>>  #endif
>>>  }
>>>  
>>> +static __always_inline void mm_clear_owner(struct mm_struct *mm,
>>> +					   struct task_struct *p)
>>> +{
>>> +#ifdef CONFIG_MEMCG
>>> +	if (mm->owner == p)
>>> +		WRITE_ONCE(mm->owner, NULL);
>>> +#endif
>>> +}
>>> +
>>>  static void mm_init_owner(struct mm_struct *mm, struct task_struct *p)
>>>  {
>>>  #ifdef CONFIG_MEMCG
>>> @@ -1331,6 +1340,7 @@ static struct mm_struct *dup_mm(struct task_struct *tsk)
>>>  free_pt:
>>>  	/* don't put binfmt in mmput, we haven't got module yet */
>>>  	mm->binfmt = NULL;
>>> +	mm_init_owner(mm, NULL);
>>>  	mmput(mm);
>>>  
>>>  fail_nomem:
>>> @@ -1662,6 +1672,24 @@ static inline void rcu_copy_process(struct task_struct *p)
>>>  #endif /* #ifdef CONFIG_TASKS_RCU */
>>>  }
>>>  
>>> +#ifdef CONFIG_MEMCG
>>> +static void __delayed_free_task(struct rcu_head *rhp)
>>> +{
>>> +	struct task_struct *tsk = container_of(rhp, struct task_struct, rcu);
>>> +
>>> +	free_task(tsk);
>>> +}
>>> +#endif /* CONFIG_MEMCG */
>>> +
>>> +static __always_inline void delayed_free_task(struct task_struct *tsk)
>>> +{
>>> +#ifdef CONFIG_MEMCG
>>> +	call_rcu(&tsk->rcu, __delayed_free_task);
>>> +#else /* CONFIG_MEMCG */
>>> +	free_task(tsk);
>>> +#endif /* CONFIG_MEMCG */
>>> +}
>>> +
>>>  /*
>>>   * This creates a new process as a copy of the old one,
>>>   * but does not actually start it yet.
>>> @@ -2123,8 +2151,10 @@ static __latent_entropy struct task_struct *copy_process(
>>>  bad_fork_cleanup_namespaces:
>>>  	exit_task_namespaces(p);
>>>  bad_fork_cleanup_mm:
>>> -	if (p->mm)
>>> +	if (p->mm) {
>>> +		mm_clear_owner(p->mm, p);
>>>  		mmput(p->mm);
>>> +	}
>>>  bad_fork_cleanup_signal:
>>>  	if (!(clone_flags & CLONE_THREAD))
>>>  		free_signal_struct(p->signal);
>>> @@ -2155,7 +2185,7 @@ static __latent_entropy struct task_struct *copy_process(
>>>  bad_fork_free:
>>>  	p->state = TASK_DEAD;
>>>  	put_task_stack(p);
>>> -	free_task(p);
>>> +	delayed_free_task(p);
>>
>> Can't call_rcu(&p->rcu, delayed_put_task_struct) be used instead this?
> 
> I mean:
> 
> refcount_set(&tsk->usage, 2);

I.e., refcount_set(&tsk->usage, 1);

> call_rcu(&p->rcu, delayed_put_task_struct);
> 
> And:
> 
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 3c516c6f7ce4..27cdf61b51a1 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -715,7 +715,9 @@ static inline void put_signal_struct(struct signal_struct *sig)
>  
>  void __put_task_struct(struct task_struct *tsk)
>  {
> -	WARN_ON(!tsk->exit_state);
> +	if (!tsk->exit_state)
> +	/* Cleanup of copy_process() */
> +		goto free;
>  	WARN_ON(refcount_read(&tsk->usage));
>  	WARN_ON(tsk == current);
>  
> @@ -727,6 +729,7 @@ void __put_task_struct(struct task_struct *tsk)
>  	put_signal_struct(tsk->signal);
>  
>  	if (!profile_handoff_task(tsk))
> +free:
>  		free_task(tsk);
>  }
>  EXPORT_SYMBOL_GPL(__put_task_struct);
>