mm_update_next_owner() checks the children / real_parent->children to
avoid the "everything else" loop in the likely case, but this won't work
if a child/sibling has a zombie leader with ->mm == NULL.

Move the for_each_thread() logic into try_to_set_owner(), if nothing else
this makes the children/siblings/everything searches more consistent.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/exit.c | 40 ++++++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index a1ef5f23d5be..cc56edc1103e 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -440,7 +440,7 @@ static void coredump_task_exit(struct task_struct *tsk)
 
 #ifdef CONFIG_MEMCG
 /* drops tasklist_lock if succeeds */
-static bool try_to_set_owner(struct task_struct *tsk, struct mm_struct *mm)
+static bool __try_to_set_owner(struct task_struct *tsk, struct mm_struct *mm)
 {
 	bool ret = false;
 
@@ -456,12 +456,28 @@ static bool try_to_set_owner(struct task_struct *tsk, struct mm_struct *mm)
 	return ret;
 }
 
+static bool try_to_set_owner(struct task_struct *g, struct mm_struct *mm)
+{
+	struct task_struct *t;
+
+	for_each_thread(g, t) {
+		struct mm_struct *t_mm = READ_ONCE(t->mm);
+		if (t_mm == mm) {
+			if (__try_to_set_owner(t, mm))
+				return true;
+		} else if (t_mm)
+			break;
+	}
+
+	return false;
+}
+
 /*
  * A task is exiting.   If it owned this mm, find a new owner for the mm.
  */
 void mm_update_next_owner(struct mm_struct *mm)
 {
-	struct task_struct *c, *g, *p = current;
+	struct task_struct *g, *p = current;
 
 	/*
 	 * If the exiting or execing task is not the owner, it's
@@ -483,19 +499,17 @@ void mm_update_next_owner(struct mm_struct *mm)
 	/*
 	 * Search in the children
 	 */
-	list_for_each_entry(c, &p->children, sibling) {
-		if (c->mm == mm && try_to_set_owner(c, mm))
+	list_for_each_entry(g, &p->children, sibling) {
+		if (try_to_set_owner(g, mm))
 			goto ret;
 	}
-
 	/*
 	 * Search in the siblings
 	 */
-	list_for_each_entry(c, &p->real_parent->children, sibling) {
-		if (c->mm == mm && try_to_set_owner(c, mm))
+	list_for_each_entry(g, &p->real_parent->children, sibling) {
+		if (try_to_set_owner(g, mm))
 			goto ret;
 	}
-
 	/*
 	 * Search through everything else, we should not get here often.
 	 */
@@ -504,14 +518,8 @@ void mm_update_next_owner(struct mm_struct *mm)
 			break;
 		if (g->flags & PF_KTHREAD)
 			continue;
-		for_each_thread(g, c) {
-			struct mm_struct *c_mm = READ_ONCE(c->mm);
-			if (c_mm == mm) {
-				if (try_to_set_owner(c, mm))
-					goto ret;
-			} else if (c_mm)
-				break;
-		}
+		if (try_to_set_owner(g, mm))
+			goto ret;
 	}
 	read_unlock(&tasklist_lock);
 	/*
-- 
2.25.1.362.g51ebf55

On Wed 26-06-24 17:29:30, Oleg Nesterov wrote:
> mm_update_next_owner() checks the children / real_parent->children to
> avoid the "everything else" loop in the likely case, but this won't work
> if a child/sibling has a zombie leader with ->mm == NULL.
> 
> Move the for_each_thread() logic into try_to_set_owner(), if nothing else
> this makes the children/siblings/everything searches more consistent.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  kernel/exit.c | 40 ++++++++++++++++++++++++----------------
>  1 file changed, 24 insertions(+), 16 deletions(-)
> 
> diff --git a/kernel/exit.c b/kernel/exit.c
> index a1ef5f23d5be..cc56edc1103e 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -440,7 +440,7 @@ static void coredump_task_exit(struct task_struct *tsk)
>  
>  #ifdef CONFIG_MEMCG
>  /* drops tasklist_lock if succeeds */
> -static bool try_to_set_owner(struct task_struct *tsk, struct mm_struct *mm)
> +static bool __try_to_set_owner(struct task_struct *tsk, struct mm_struct *mm)
>  {
>  	bool ret = false;
>  
> @@ -456,12 +456,28 @@ static bool try_to_set_owner(struct task_struct *tsk, struct mm_struct *mm)
>  	return ret;
>  }
>  
> +static bool try_to_set_owner(struct task_struct *g, struct mm_struct *mm)
> +{
> +	struct task_struct *t;
> +
> +	for_each_thread(g, t) {
> +		struct mm_struct *t_mm = READ_ONCE(t->mm);
> +		if (t_mm == mm) {
> +			if (__try_to_set_owner(t, mm))
> +				return true;
> +		} else if (t_mm)
> +			break;
> +	}
> +
> +	return false;
> +}
> +
>  /*
>   * A task is exiting.   If it owned this mm, find a new owner for the mm.
>   */
>  void mm_update_next_owner(struct mm_struct *mm)
>  {
> -	struct task_struct *c, *g, *p = current;
> +	struct task_struct *g, *p = current;
>  
>  	/*
>  	 * If the exiting or execing task is not the owner, it's
> @@ -483,19 +499,17 @@ void mm_update_next_owner(struct mm_struct *mm)
>  	/*
>  	 * Search in the children
>  	 */
> -	list_for_each_entry(c, &p->children, sibling) {
> -		if (c->mm == mm && try_to_set_owner(c, mm))
> +	list_for_each_entry(g, &p->children, sibling) {
> +		if (try_to_set_owner(g, mm))
>  			goto ret;
>  	}
> -
>  	/*
>  	 * Search in the siblings
>  	 */
> -	list_for_each_entry(c, &p->real_parent->children, sibling) {
> -		if (c->mm == mm && try_to_set_owner(c, mm))
> +	list_for_each_entry(g, &p->real_parent->children, sibling) {
> +		if (try_to_set_owner(g, mm))
>  			goto ret;
>  	}
> -
>  	/*
>  	 * Search through everything else, we should not get here often.
>  	 */
> @@ -504,14 +518,8 @@ void mm_update_next_owner(struct mm_struct *mm)
>  			break;
>  		if (g->flags & PF_KTHREAD)
>  			continue;
> -		for_each_thread(g, c) {
> -			struct mm_struct *c_mm = READ_ONCE(c->mm);
> -			if (c_mm == mm) {
> -				if (try_to_set_owner(c, mm))
> -					goto ret;
> -			} else if (c_mm)
> -				break;
> -		}
> +		if (try_to_set_owner(g, mm))
> +			goto ret;
>  	}
>  	read_unlock(&tasklist_lock);
>  	/*
> -- 
> 2.25.1.362.g51ebf55

-- 
Michal Hocko
SUSE Labs