Re: [PATCH v2 1/3] cgroup/cpuset: Reject updates that empty the top sched domain

[Waiman Long <longman@redhat.com>]

On 11/5/26 8:03 AM, Guopeng Zhang wrote:
> From: Guopeng Zhang <zhangguopeng@kylinos.cn>
>
> Boot-isolated CPUs remain in top_cpuset.effective_cpus, but are excluded
> from its scheduling domain by generate_sched_domains(). Checking only
> effective_cpus when distributing CPUs to partitions can therefore allow
> all boot domain housekeeping CPUs to be taken from the top cpuset.
>
> The resulting empty domain triggers a warning in build_sched_domains()
> and can cause an out-of-bounds CPU access in build_perf_domains(). Both
> root and isolated partitions can trigger this, whether local or remote.
> For example, on a 32-CPU system booted with isolcpus=domain,10-31:
>
>    echo 0-9 > A/cpuset.cpus
>    echo root > A/cpuset.cpus.partition
>    Oops: general protection fault
>    RIP: 0010:build_perf_domains+...
>
> Require local and remote partition creation and CPU mask updates to
> leave an active boot domain housekeeping CPU in the top cpuset,
> accounting for CPUs returned by the same update.
>
> Add test cases to test_cpuset_prs.sh to verify that every partition
> update leaves an active boot domain housekeeping CPU in the top cpuset.
>
> Fixes: 9b496a8bbed9 ("cgroup/cpuset: Prevent leakage of isolated CPUs into sched domains")
> Co-developed-by: Ridong Chen <ridong.chen@linux.dev>
> Signed-off-by: Ridong Chen <ridong.chen@linux.dev>
> Signed-off-by: Guopeng Zhang <zhangguopeng@kylinos.cn>
> ---
>   kernel/cgroup/cpuset.c                        | 48 ++++++++++++++++++-
>   .../selftests/cgroup/test_cpuset_prs.sh       | 47 ++++++++++++++++++
>   2 files changed, 93 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index 3052413e41e7..c405719ccd12 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -1180,12 +1180,56 @@ static void update_partition_sd_lb(struct cpuset *cs, int old_prs)
>   		cpuset_force_rebuild();
>   }
>   
> +/*
> + * top_cpuset_sd_nocpu - Check if top_cpuset will have no sched domain CPU
> + * @add_cpus: CPUs to be taken away from top_cpuset
> + * @del_cpus: CPUs to be given back to top_cpuset, can be NULL
> + * Return: true if no active CPU will be left in the top_cpuset sched domain
> + *
> + * Note that @del_cpus may include CPUs that are also in @add_cpus and
> + * will stay in the partition. They are skipped by the @add_cpus check,
> + * so callers can pass the current effective_xcpus of a partition
> + * without masking it first.
> + *
> + * Boot time isolated CPUs stay in top_cpuset.effective_cpus, but they are
> + * excluded from the sched domain of top_cpuset in generate_sched_domains().
> + * So checking top_cpuset.effective_cpus alone is not enough to ensure that
> + * the top_cpuset sched domain won't become empty after @add_cpus are taken
> + * away from it. The sched domain of top_cpuset must keep at least one
> + * active boot domain housekeeping CPU.
> + */
> +static bool top_cpuset_sd_nocpu(const struct cpumask *add_cpus,
> +				const struct cpumask *del_cpus)
> +{
> +	const struct cpumask *hk_mask = housekeeping_cpumask(HK_TYPE_DOMAIN_BOOT);
> +	int cpu;
> +
> +	for_each_cpu_and(cpu, hk_mask, cpu_active_mask) {
> +		if (cpumask_test_cpu(cpu, add_cpus))
> +			continue;
> +		if (cpumask_test_cpu(cpu, top_cpuset.effective_cpus) ||
> +		    (del_cpus && cpumask_test_cpu(cpu, del_cpus)))
> +			return false;
> +	}
> +	return true;
> +}
> +
>   /*
>    * tasks_nocpu_error - Return true if tasks will have no effective_cpus
>    */
>   static bool tasks_nocpu_error(struct cpuset *parent, struct cpuset *cs,
>   			      struct cpumask *xcpus)
>   {
> +	/*
> +	 * The top_cpuset always has tasks (kthreads at least), so its sched
> +	 * domain can't be empty. The current effective_xcpus of a valid
> +	 * partition will be given back to top_cpuset if not in @xcpus.
> +	 */
> +	if (parent == &top_cpuset &&
> +	    top_cpuset_sd_nocpu(xcpus, is_partition_valid(cs)
> +				     ? cs->effective_xcpus : NULL))
> +		return true;
> +
>   	/*
>   	 * A populated partition (cs or parent) can't have empty effective_cpus
>   	 */
> @@ -1724,7 +1768,7 @@ static int remote_partition_enable(struct cpuset *cs, int new_prs,
>   	 */
>   	compute_excpus(cs, tmp->new_cpus);
>   	if (!cpumask_intersects(tmp->new_cpus, cpu_active_mask) ||
> -	    cpumask_subset(top_cpuset.effective_cpus, tmp->new_cpus))
> +	    top_cpuset_sd_nocpu(tmp->new_cpus, NULL))
>   		return PERR_INVCPUS;
>   	if (cpumask_intersects(tmp->new_cpus, subpartitions_cpus))
>   		return PERR_NOCPUS;
> @@ -1828,7 +1872,7 @@ static void remote_cpus_update(struct cpuset *cs, struct cpumask *xcpus,
>   		if (!capable(CAP_SYS_ADMIN))
>   			WRITE_ONCE(cs->prs_err, PERR_ACCESS);
>   		else if (cpumask_intersects(tmp->addmask, subpartitions_cpus) ||
> -			 cpumask_subset(top_cpuset.effective_cpus, tmp->addmask))
> +			 top_cpuset_sd_nocpu(tmp->addmask, tmp->delmask))
>   			WRITE_ONCE(cs->prs_err, PERR_NOCPUS);
>   		else if (prstate_housekeeping_conflict(prs, PRS_ROOT,
>   						       excpus, tmp->delmask))
The code change above looks good to me.
> diff --git a/tools/testing/selftests/cgroup/test_cpuset_prs.sh b/tools/testing/selftests/cgroup/test_cpuset_prs.sh
> index bdca6c471f87..e813ea9959f4 100755
> --- a/tools/testing/selftests/cgroup/test_cpuset_prs.sh
> +++ b/tools/testing/selftests/cgroup/test_cpuset_prs.sh
> @@ -133,6 +133,15 @@ LAST_PRESENT_CPU=$(cat /sys/devices/system/cpu/present | sed -e "s/.*[,-]//")
>   [[ -n "$FIRST_NOHZ_FULL_CPU" && "$FIRST_NOHZ_FULL_CPU" = 8 &&
>      "$LAST_NOHZ_FULL_CPU" = $LAST_PRESENT_CPU ]] && RUN_NOHZ_FULL_TESTS=1
>   
> +# These tests require boot domain housekeeping CPUs to be exactly 0-9.
> +# Check the actual masks to cover isolcpus= with or without explicit flags.
> +DOMAIN_ISOLCPUS="10-$LAST_PRESENT_CPU"
> +[[ "$LAST_PRESENT_CPU" = 10 ]] && DOMAIN_ISOLCPUS=10
I think you should use "==", not "=".
> +RUN_ISOLCPUS_DOMAIN_TESTS=
> +[[ -z "$NOHZ_FULL_CPUS" &&
> +	"$(cat /sys/devices/system/cpu/isolated)" = "$DOMAIN_ISOLCPUS" ]] &&
> +	RUN_ISOLCPUS_DOMAIN_TESTS=1

Why there shouldn't be any nohz_full CPUs? Domain isolated CPUs and 
nohz_full CPUs don't need to be exclusive.

Maybe you can move the new testing code below the ISOLCPUS section as 
the value of /sys/devices/system/cpu/isolated would have been read into 
the BOOT_ISOLCPUS variable which you can use instead of reading the 
sysfs file again.

> +
>   #
>   # If isolated CPUs have been reserved at boot time (as shown in
>   # /sys/devices/system/cpu/isolated), these isolated CPUs should be outside of
> @@ -602,6 +611,43 @@ ISOLCPUS_TEST_MATRIX=(
>   	" CX1-2,I:X1,I:P1 . .      .     X1,2,I  .      .      .     0 A1:1-2,I|XA1:1-2,I A1:P-1 ."
>   )
>   
> +# Test matrix for isolcpus=domain,10-<last> or isolcpus=10-<last> boots.
> +# The top cpuset must retain an active boot domain housekeeping CPU.
> +ISOLCPUS_DOMAIN_TEST_MATRIX=(
> +	#  old-A1 old-A2 old-A3 old-B1 new-A1 new-A2 new-A3 new-B1 fail ECPUs Pstate ISOLCPUS
> +	#  ------ ------ ------ ------ ------ ------ ------ ------ ---- ----- ------ --------
> +	# Neither a root nor an isolated partition may take all boot HK CPUs.
> +	" C0-9      .      .      .     P1      .      .      .     0 A1:0-9 A1:P-1 ."
> +	" C0-9      .      .      .     P2      .      .      .     0 A1:0-9 A1:P-2 ."
> +
> +	# Growing cpuset.cpus must not take the last top-level HK CPU.
> +	" C0-8:P1   .      .      .    C0-9     .      .      .     0 A1:0-9 A1:P-1 ."
> +
> +	# CPUs returned by the same update can keep the top sched domain alive.
> +	" C2-9:P2   .      .      .    C0-4     .      .      .     0 A1:0-4 A1:P2 0-4"
> +
> +	# Root and isolated partitions together must leave a top-level HK CPU.
> +	" C0-4:P1   .      .     C5-9    .      .      .     P2    0 A1:0-4|B1:5-9 A1:P1|B1:P-2 ."
> +
> +	# Remote partition creation must enforce the same constraint.
> +	" X0-9    CX0-9    .      .      .     P1      .      .     0 A2:0-9 A2:P-1 ."
> +
> +	# Growing a remote partition through cpuset.cpus.exclusive is rejected.
> +	" X0-9 C0-9:X0-8:P1 .     .      .    X0-9     .      .     0 A2:0-9 A2:P-1 ."
> +
> +	# Remote CPU replacements must account for the CPUs being returned.
> +	" X0-9 C0-9:X2-9:P2 .     .      .    X0-4     .      .     0 A2:0-4 A2:P2 0-4"
> +
> +	# Local exclusive CPU updates must not consume the last top-level HK CPU.
> +	" C0-9:X0-8:P2 .   .      .    X0-9     .      .      .     0 A1:0-9 A1:P-2 ."
> +
> +	# Ancestor updates must not grow a remote partition over the last HK CPU.
> +	" X0-8 CX0-9:P1    .      .    X0-9     .      .      .     0 A2:0-9 A2:P-1 ."
> +
> +	# An invalid partition can recover once it leaves a top-level HK CPU.
> +	" C0-9:P1   .      .      .    C0-8     .      .      .     0 A1:0-8 A1:P1 ."
> +)
> +

Could you also put this new test matrix together with the other test 
matrices?

Thanks,
Longman

>   # Test matrix to be run when boot time nohz_full CPUs cover all CPUs except CPUs 0-7
>   # This also means that CPU 8 is available and is a nohz_full CPU.
>   NOHZ_FULL_TEST_MATRIX=(
> @@ -1472,6 +1518,7 @@ test_inotify()
>   run_state_test TEST_MATRIX
>   run_remote_state_test REMOTE_TEST_MATRIX
>   TEST_ISOLCPUS=1 run_state_test ISOLCPUS_TEST_MATRIX
> +[[ -n "$RUN_ISOLCPUS_DOMAIN_TESTS" ]] && run_state_test ISOLCPUS_DOMAIN_TEST_MATRIX
>   [[ -n "$RUN_NOHZ_FULL_TESTS" ]] && run_state_test NOHZ_FULL_TEST_MATRIX
>   test_isolated
>   test_boot_isolated