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@xxxxxxxxxx
>>> Tested-by: zhong jiang <zhongjiang@xxxxxxxxxx>
>>> Reported-by: syzbot+cbb52e396df3e565ab02@xxxxxxxxxxxxxxxxxxxxxxxxx
>>> Signed-off-by: Andrea Arcangeli <aarcange@xxxxxxxxxx>
>>> ---
>>> 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);
>
