Re: [PATCH-next v9 03/11] cgroup/cpuset: Prevent race between task attach and cpuset state change

[Ridong Chen <ridong.chen@linux.dev>]

On 6/30/2026 11:33 AM, Waiman Long wrote:
> Commit e44193d39e8d ("cpuset: let hotplug propagation work wait for
> task attaching") was introduced to let hotplug operation to wait
> until the completion of task attach operation. However, it is still
> possible that the states of the source or destination cpuset can
> be changed between the cpuset_can_attach() call and the subsequent
> cpuset_attach()/cpuset_cacnel_attach() call.
> 
> As a result, data gathered during cpuset_can_attach() cannot be reliably
> used in the subsequent cpuset_attach()/cpuset_cacnel_attach()
> call at all. Make the task attach operation more robust
> and allow the sharing of data between cpuset_can_attach() and
> cpuset_attach()/cpuset_cacnel_attach() by making cpuset_write_resmask()
> and cpuset_partition_write() wait for the completion of task attach
> as well.
> 

Nit.

s/cpuset_cacnel_attach/cpuset_cancel_attach/

> Ideally, an ongoing task attach operation should block any cpuset write
> operation that can change its internal state until the operation is
> completed. However, the attach_in_progress flag is currently per cpuset
> and only the destination cpuset will have this flag set. The flag is not
> set in the source cpuset where the tasks will be moved from. Even if we
> extend the scope to include the source cpuset, it will not block cpuset
> operation that changes the state of one of its ancestor cpuset which may
> indirectly impact the state of the source or destination cpuset. It may
> be too costly to set the flag for the whole subtree, it is far easier
> to just make the flag global and block all the cpuset write operation
> whenever a task attach operation is in progress.
> 
> Make that change by creating a new cpuset attach context (attach_ctx)
> structure to hold the global in_progress flag and use it for blocking
> cpuset write operation if a cpuset attach operation is in progress. Also
> add a new wait_attach_done_lock() helper to do the waiting for an
> ongoing attach operation and acquire the cpuset_mutex.
> 
> The comments about validate_change() are no longer valid as it won't
> be called at all if an attach operation is in progress. So the comments
> can be removed.
> 
> The per-cpuset attach_in_progress flag is also currently used in
> partition_is_populated() and cpuset_is_populated() to determine if
> an empty cpuset will have incoming task. This check will no longer be
> needed as this function will not be called when there is a task attach
> in progress. So the flag check is now removed.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
>   kernel/cgroup/cpuset-internal.h | 11 +---
>   kernel/cgroup/cpuset.c          | 90 +++++++++++++++++++--------------
>   2 files changed, 53 insertions(+), 48 deletions(-)
> 
> diff --git a/kernel/cgroup/cpuset-internal.h b/kernel/cgroup/cpuset-internal.h
> index 140700e5e236..df662c7fd1a4 100644
> --- a/kernel/cgroup/cpuset-internal.h
> +++ b/kernel/cgroup/cpuset-internal.h
> @@ -145,12 +145,6 @@ struct cpuset {
>   	 */
>   	nodemask_t old_mems_allowed;
>   
> -	/*
> -	 * Tasks are being attached to this cpuset.  Used to prevent
> -	 * zeroing cpus/mems_allowed between ->can_attach() and ->attach().
> -	 */
> -	int attach_in_progress;
> -
>   	/* partition root state */
>   	int partition_root_state;
>   
> @@ -269,10 +263,7 @@ static inline int nr_cpusets(void)
>   static inline bool cpuset_is_populated(struct cpuset *cs)
>   {
>   	lockdep_assert_cpuset_lock_held();
> -
> -	/* Cpusets in the process of attaching should be considered as populated */
> -	return cgroup_is_populated(cs->css.cgroup) ||
> -		cs->attach_in_progress;
> +	return cgroup_is_populated(cs->css.cgroup);
>   }
>   
>   /**
> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index 431bf210aa52..1a78d0590737 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -356,6 +356,33 @@ static struct workqueue_struct *cpuset_migrate_mm_wq;
>   
>   static DECLARE_WAIT_QUEUE_HEAD(cpuset_attach_wq);
>   
> +/*
> + * Cpuset task attach context
> + * Protected by cpuset_mutex
> + */
> +static struct {
> +	int in_progress;
> +} attach_ctx;
> +
> +/*
> + * Wait if task attach is in progress until it is done and then acquire
> + * cpuset_mutex before returning.
> + */
> +static void wait_attach_done_lock(void)
> +	    __acquires(&cpuset_mutex)
> +{
> +	for (;;) {
> +		mutex_lock(&cpuset_mutex);
> +		if (!attach_ctx.in_progress)
> +			return;
> +
> +		mutex_unlock(&cpuset_mutex);
> +
> +		/* Wait until attach operation is done to prevent racing */
> +		wait_event(cpuset_attach_wq, attach_ctx.in_progress == 0);
> +	}
> +}
> +
>   static inline void check_insane_mems_config(nodemask_t *nodes)
>   {
>   	if (!cpusets_insane_config() &&
> @@ -368,22 +395,22 @@ static inline void check_insane_mems_config(nodemask_t *nodes)
>   }
>   
>   /*
> - * decrease cs->attach_in_progress.
> - * wake_up cpuset_attach_wq if cs->attach_in_progress==0.
> + * decrease attach_ctx.in_progress.
> + * wake_up cpuset_attach_wq if attach_ctx.in_progress==0.
>    */
> -static inline void dec_attach_in_progress_locked(struct cpuset *cs)
> +static inline void dec_attach_in_progress_locked(void)
>   {
>   	lockdep_assert_cpuset_lock_held();
>   
> -	cs->attach_in_progress--;
> -	if (!cs->attach_in_progress)
> +	attach_ctx.in_progress--;
> +	if (!attach_ctx.in_progress)
>   		wake_up(&cpuset_attach_wq);
>   }
>   
> -static inline void dec_attach_in_progress(struct cpuset *cs)
> +static inline void dec_attach_in_progress(void)
>   {
>   	mutex_lock(&cpuset_mutex);
> -	dec_attach_in_progress_locked(cs);
> +	dec_attach_in_progress_locked();
>   	mutex_unlock(&cpuset_mutex);
>   }
>   
> @@ -432,8 +459,7 @@ static inline bool partition_is_populated(struct cpuset *cs,
>   	 * nr_populated_domain_children may include populated
>   	 * csets from descendants that are partitions.
>   	 */
> -	if (cgroup_has_tasks(cs->css.cgroup) ||
> -	    cs->attach_in_progress)
> +	if (cgroup_has_tasks(cs->css.cgroup))
>   		return true;
>   
>   	rcu_read_lock();
> @@ -3091,11 +3117,7 @@ static int cpuset_can_attach(struct cgroup_taskset *tset)
>   	cs->dl_bw_cpu = cpu;
>   
>   out_success:
> -	/*
> -	 * Mark attach is in progress.  This makes validate_change() fail
> -	 * changes which zero cpus/mems_allowed.
> -	 */
> -	cs->attach_in_progress++;
> +	attach_ctx.in_progress++;
>   
>   out_unlock:
>   	if (ret)
> @@ -3113,7 +3135,7 @@ static void cpuset_cancel_attach(struct cgroup_taskset *tset)
>   	cs = css_cs(css);
>   
>   	mutex_lock(&cpuset_mutex);
> -	dec_attach_in_progress_locked(cs);
> +	dec_attach_in_progress_locked();
>   
>   	if (cs->dl_bw_cpu >= 0)
>   		dl_bw_free(cs->dl_bw_cpu, cs->sum_migrate_dl_bw);
> @@ -3226,7 +3248,7 @@ static void cpuset_attach(struct cgroup_taskset *tset)
>   		reset_migrate_dl_data(cs);
>   	}
>   
> -	dec_attach_in_progress_locked(cs);
> +	dec_attach_in_progress_locked();
>   
>   	mutex_unlock(&cpuset_mutex);
>   }
> @@ -3246,7 +3268,12 @@ ssize_t cpuset_write_resmask(struct kernfs_open_file *of,
>   		return -EACCES;
>   
>   	buf = strstrip(buf);
> -	cpuset_full_lock();
> +
> +	/* cpuset_mutex acquired in wait_attach_done_lock() */
> +	mutex_lock(&cpuset_top_mutex);
> +	cpus_read_lock();
> +	wait_attach_done_lock();
> +

Would it be cleaner to just pass this into cpuset_full_lock() as a flag?

void cpuset_full_lock(bool wait_attach)
{
       mutex_lock(&cpuset_top_mutex);
       cpus_read_lock();
       if (wait_attach)
               wait_attach_done_lock();
       else
               mutex_lock(&cpuset_mutex);
}
Then the two write paths become a single cpuset_full_lock(true). The
downside is the other 6 callers would need cpuset_full_lock(false). Not 
sure it's worth it — what do you think?

>   	if (!is_cpuset_online(cs))
>   		goto out_unlock;
>   
> @@ -3377,7 +3404,10 @@ static ssize_t cpuset_partition_write(struct kernfs_open_file *of, char *buf,
>   	else
>   		return -EINVAL;
>   
> -	cpuset_full_lock();
> +	mutex_lock(&cpuset_top_mutex);
> +	cpus_read_lock();
> +	wait_attach_done_lock();
> +
>   	if (is_cpuset_online(cs))
>   		retval = update_prstate(cs, val);
>   	cpuset_update_sd_hk_unlock();
> @@ -3616,11 +3646,7 @@ static int cpuset_can_fork(struct task_struct *task, struct css_set *cset)
>   	if (ret)
>   		goto out_unlock;
>   
> -	/*
> -	 * Mark attach is in progress.  This makes validate_change() fail
> -	 * changes which zero cpus/mems_allowed.
> -	 */
> -	cs->attach_in_progress++;
> +	attach_ctx.in_progress++;
>   out_unlock:
>   	mutex_unlock(&cpuset_mutex);
>   	return ret;
> @@ -3638,7 +3664,7 @@ static void cpuset_cancel_fork(struct task_struct *task, struct css_set *cset)
>   	if (same_cs)
>   		return;
>   
> -	dec_attach_in_progress(cs);
> +	dec_attach_in_progress();
>   }
>   
>   /*
> @@ -3670,7 +3696,7 @@ static void cpuset_fork(struct task_struct *task)
>   	guarantee_online_mems(cs, &cpuset_attach_nodemask_to);
>   	cpuset_attach_task(cs, task);
>   
> -	dec_attach_in_progress_locked(cs);
> +	dec_attach_in_progress_locked();
>   	mutex_unlock(&cpuset_mutex);
>   }
>   
> @@ -3774,20 +3800,8 @@ static void cpuset_hotplug_update_tasks(struct cpuset *cs, struct tmpmasks *tmp)
>   	bool remote;
>   	int partcmd = -1;
>   	struct cpuset *parent;
> -retry:
> -	wait_event(cpuset_attach_wq, cs->attach_in_progress == 0);
> -
> -	mutex_lock(&cpuset_mutex);
> -
> -	/*
> -	 * We have raced with task attaching. We wait until attaching
> -	 * is finished, so we won't attach a task to an empty cpuset.
> -	 */
> -	if (cs->attach_in_progress) {
> -		mutex_unlock(&cpuset_mutex);
> -		goto retry;
> -	}
>   
> +	wait_attach_done_lock();
>   	parent = parent_cs(cs);
>   	compute_effective_cpumask(&new_cpus, cs, parent);
>   	compute_effective_nodemask(&new_mems, cs, parent);

Overall this looks good to me.

Reviewed-by: Ridong Chen <ridong.chen@linux.dev>

-- 
Best regards
Ridong