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: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);
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);