linux-kernel - Re: [PATCH v5 1/3] sched: Use user_cpus_ptr for saving user provided cpumask in sched

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Yvyp02LLIQQPs5d6@worktop.programming.kicks-ass.net>
Date:   Wed, 17 Aug 2022 10:41:55 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Waiman Long <longman@...hat.com>
Cc:     Ingo Molnar <mingo@...hat.com>, Juri Lelli <juri.lelli@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        Valentin Schneider <vschneid@...hat.com>,
        Tejun Heo <tj@...nel.org>, Zefan Li <lizefan.x@...edance.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Will Deacon <will@...nel.org>, cgroups@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH v5 1/3] sched: Use user_cpus_ptr for saving user provided
 cpumask in sched_setaffinity()

On Tue, Aug 16, 2022 at 03:27:32PM -0400, Waiman Long wrote:
> @@ -2981,25 +2969,21 @@ static int restrict_cpus_allowed_ptr(struct task_struct *p,
>  		goto err_unlock;
>  	}
>  
> -	if (!cpumask_and(new_mask, &p->cpus_mask, subset_mask)) {
> +
> +	if (p->user_cpus_ptr)
> +		not_empty = cpumask_and(new_mask, p->user_cpus_ptr, subset_mask);
> +	else
> +		not_empty = cpumask_and(new_mask, cpu_online_mask, subset_mask);
> +
> +	if (!not_empty) {
>  		err = -EINVAL;
>  		goto err_unlock;
>  	}
>  
> -	/*
> -	 * We're about to butcher the task affinity, so keep track of what
> -	 * the user asked for in case we're able to restore it later on.
> -	 */
> -	if (user_mask) {
> -		cpumask_copy(user_mask, p->cpus_ptr);
> -		p->user_cpus_ptr = user_mask;
> -	}
> -
>  	return __set_cpus_allowed_ptr_locked(p, new_mask, 0, rq, &rf);
>  
>  err_unlock:
>  	task_rq_unlock(rq, p, &rf);
> -	kfree(user_mask);
>  	return err;
>  }
>  
> @@ -3049,34 +3033,27 @@ void force_compatible_cpus_allowed_ptr(struct task_struct *p)
>  }
>  
>  static int
> -__sched_setaffinity(struct task_struct *p, const struct cpumask *mask);
> +__sched_setaffinity(struct task_struct *p, const struct cpumask *mask, bool save_mask);
>  
>  /*
>   * Restore the affinity of a task @p which was previously restricted by a
> - * call to force_compatible_cpus_allowed_ptr(). This will clear (and free)
> - * @p->user_cpus_ptr.
> + * call to force_compatible_cpus_allowed_ptr().
>   *
>   * It is the caller's responsibility to serialise this with any calls to
>   * force_compatible_cpus_allowed_ptr(@p).
>   */
>  void relax_compatible_cpus_allowed_ptr(struct task_struct *p)
>  {
> -	struct cpumask *user_mask = p->user_cpus_ptr;
> -	unsigned long flags;
> +	const struct cpumask *user_mask = p->user_cpus_ptr;
> +
> +	if (!user_mask)
> +		user_mask = cpu_online_mask;
>  
>  	/*
> -	 * Try to restore the old affinity mask. If this fails, then
> -	 * we free the mask explicitly to avoid it being inherited across
> -	 * a subsequent fork().
> +	 * Try to restore the old affinity mask with __sched_setaffinity().
> +	 * Cpuset masking will be done there too.
>  	 */
> -	if (!user_mask || !__sched_setaffinity(p, user_mask))
> -		return;
> -
> -	raw_spin_lock_irqsave(&p->pi_lock, flags);
> -	user_mask = clear_user_cpus_ptr(p);
> -	raw_spin_unlock_irqrestore(&p->pi_lock, flags);
> -
> -	kfree(user_mask);
> +	__sched_setaffinity(p, user_mask, false);
>  }
>  
>  void set_task_cpu(struct task_struct *p, unsigned int new_cpu)


Would it not be simpler to write it something like so?

---
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 03053eebb22e..cdae4d50a588 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2955,7 +2955,6 @@ static int restrict_cpus_allowed_ptr(struct task_struct *p,
 	struct rq_flags rf;
 	struct rq *rq;
 	int err;
-	bool not_empty;
 
 	rq = task_rq_lock(p, &rf);
 
@@ -2969,13 +2968,7 @@ static int restrict_cpus_allowed_ptr(struct task_struct *p,
 		goto err_unlock;
 	}
 
-
-	if (p->user_cpus_ptr)
-		not_empty = cpumask_and(new_mask, p->user_cpus_ptr, subset_mask);
-	else
-		not_empty = cpumask_and(new_mask, cpu_online_mask, subset_mask);
-
-	if (!not_empty) {
+	if (!cpumask_and(new_mask, task_user_cpus(p), subset_mask)) {
 		err = -EINVAL;
 		goto err_unlock;
 	}
@@ -3044,16 +3037,11 @@ __sched_setaffinity(struct task_struct *p, const struct cpumask *mask, bool save
  */
 void relax_compatible_cpus_allowed_ptr(struct task_struct *p)
 {
-	const struct cpumask *user_mask = p->user_cpus_ptr;
-
-	if (!user_mask)
-		user_mask = cpu_online_mask;
-
 	/*
 	 * Try to restore the old affinity mask with __sched_setaffinity().
 	 * Cpuset masking will be done there too.
 	 */
-	__sched_setaffinity(p, user_mask, false);
+	__sched_setaffinity(p, task_user_cpus(p), false);
 }
 
 void set_task_cpu(struct task_struct *p, unsigned int new_cpu)
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 15eefcd65faa..426e9b64b587 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -1881,6 +1881,13 @@ static inline void dirty_sched_domain_sysctl(int cpu)
 #endif
 
 extern int sched_update_scaling(void);
+
+static inline const struct cpumask *task_user_cpus(struct task_struct *p)
+{
+	if (!p->user_cpus_ptr)
+		return cpus_possible_mask; /* &init_task.cpus_mask */
+	return p->user_cpus_ptr;
+}
 #endif /* CONFIG_SMP */
 
 #include "stats.h"