[Devel] [PATCH hci-8.1 v3 0/2] sched: Fix incorrect nproc report inside CT

[Devel] [PATCH hci-8.1 v3 0/2] sched: Fix incorrect nproc report inside CT

[Vladimir Riabchun]

We observed weird nproc behavior - it reported 64 CPUs
no matter what the CT limits were or how many CPUs the host
actually had.

nproc code uses sched_getaffinity syscall under the hood[1].
It appears that this syscall returned complete nonsence when
called from VE - we ignored both num_online_vcpus and
cgroup cpuset limitations.

Now it is fixed and a new selftest is added.

[1]: https://github.com/coreutils/gnulib/blob/master/lib/nproc.c#L144
https://virtuozzo.atlassian.net/browse/VSTOR-141682

Signed-off-by: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
---
Changes in v3:
- Patch 1: num_online_vcpus() -> task_nr_cpus(p)
- Patch 2: updated a comment and moved CPU check in setup fixture to the top.
- Link to v2: http://lore.virtuozzo.com/20261028-ct-invalid-nrpoc-v2-0-968fe985ce27@virtuozzo.com

Changes in v2:
- Updated vCPU logic in patch 1. Now we don't return actual
  cpumask, insted first min(vcpu, count(mask)) bits are set.
  This is closer to the real CT resources.
- Patch 2: adjusted logic and added one more test.
- Link to v1: http://lore.virtuozzo.com/20261027-ct-invalid-nrpoc-v1-0-48727c3afe16@virtuozzo.com

---
Vladimir Riabchun (2):
      sched: Fix sched_getaffinity syscall
      selftests: ve: Test sched_getaffinity in VE

 kernel/sched/syscalls.c                            |  18 +-
 tools/testing/selftests/ve/.gitignore              |   1 +
 tools/testing/selftests/ve/Makefile                |   1 +
 .../selftests/ve/ve_sched_getaffinity_test.c       | 211 +++++++++++++++++++++
 4 files changed, 226 insertions(+), 5 deletions(-)
---
base-commit: b499efa17aa9fb8f20512d1831d1cb61b22e60d3
change-id: 20261027-ct-invalid-nrpoc-f183b3ba691d

Best regards,
--  
Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH hci-8.1 v3 1/2] sched: Fix sched_getaffinity syscall

[Vladimir Riabchun]

We observed incorrect cpu numbers inside containers:
[root@localhost ~]# nproc
64

even when there were much less physical/virtual CPUs.

The root cause is incorrect bitmap_fill usage - it sets whole
integers only, without handling bit tails, so the actual number of
set bits is 64 = bits in ULONG.
If we use cpumask only or vcpus only, nproc and other getaffinity
users will get incorrect info about CT resources.

We can't run on more than count(cpuset) CPUs and we are
limited by vCPU number. So the minimum of these two is the
real limit.

Return first bits set - anyway they can't be used for anything
except counting from inside CT. Host behavior is not changed.

It is also intentional that we perform VE check against current
exec env - CT users will get virtualized view and host users
will get real affinity without vCPU info.

https://virtuozzo.atlassian.net/browse/VSTOR-141682

Signed-off-by: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
---
 kernel/sched/syscalls.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/kernel/sched/syscalls.c b/kernel/sched/syscalls.c
index 6d921043e9e7..ce5200c84573 100644
--- a/kernel/sched/syscalls.c
+++ b/kernel/sched/syscalls.c
@@ -1316,15 +1316,23 @@ long sched_getaffinity(pid_t pid, struct cpumask *mask)
 	if (retval)
 		return retval;
 
+	scoped_guard (raw_spinlock_irqsave, &p->pi_lock) {
+		cpumask_and(mask, &p->cpus_mask, cpu_active_mask);
+	}
+
 	if (!ve_is_super(get_exec_env())) {
+		/*
+		 * setaffinity is not available inside CT, so these bits
+		 * can only be used for counting.
+		 * We can't have more than vcpus and we can't run on CPUs
+		 * outside the mask. Reporting smallest will provide true
+		 * information about CT resources.
+		 */
+		unsigned int vcpus_avail = min(task_nr_cpus(p), cpumask_weight(mask));
 		cpumask_clear(mask);
-		bitmap_fill(cpumask_bits(mask), num_online_vcpus());
-		return 0;
+		bitmap_set(cpumask_bits(mask), 0, vcpus_avail);
 	}
 
-	guard(raw_spinlock_irqsave)(&p->pi_lock);
-	cpumask_and(mask, &p->cpus_mask, cpu_active_mask);
-
 	return 0;
 }
 

-- 
2.52.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v3 1/2] sched: Fix sched_getaffinity syscall

[no-reply]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] sched: sched_getaffinity() takes vCPU limit from target task, not caller
- [Low] sched: sched_getaffinity() in CT can return an empty mask with no floor of 1
--

commit 89d21190c29ff2e196f8d22db77072e4bd2c2544
Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>

sched: Fix sched_getaffinity syscall

This patch changes sched_getaffinity() so that, inside a container, the
reported mask is a prefix of set bits. The number of bits is the smaller of
the task's vCPU limit and the weight of cpus_mask & cpu_active_mask. Host
behavior is unchanged.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-141682

> diff --git a/kernel/sched/syscalls.c b/kernel/sched/syscalls.c
> --- a/kernel/sched/syscalls.c
> +++ b/kernel/sched/syscalls.c

[ ... ]

> @@ -1316,15 +1316,23 @@ long sched_getaffinity(pid_t pid, struct cpumask *mask)
>  	if (retval)
>  		return retval;
>  
> +	scoped_guard (raw_spinlock_irqsave, &p->pi_lock) {
> +		cpumask_and(mask, &p->cpus_mask, cpu_active_mask);
> +	}
> +
>  	if (!ve_is_super(get_exec_env())) {
> +		/*
> +		 * setaffinity is not available inside CT, so these bits
> +		 * can only be used for counting.
> +		 * We can't have more than vcpus and we can't run on CPUs
> +		 * outside the mask. Reporting smallest will provide true
> +		 * information about CT resources.
> +		 */
> +		unsigned int vcpus_avail = min(task_nr_cpus(p), cpumask_weight(mask));

[Severity: Low]
Previously the vCPU limit came from num_online_vcpus(), which is
task_nr_cpus(current), so it was based on the caller. Now it comes from
task_nr_cpus(p), the target task, while the VE check still uses the
caller's exec env.

Can a task in a CT that calls sched_getaffinity(pid) for another task
in a different task group now get a different CPU count than it gets for
itself?

For example, a task in a child cpu cgroup has nr_cpus of 0, which is
clamped to num_online_cpus(). The CT root group may have a lower limit.

Callers that query themselves, such as nproc, see no change. The other
users of num_online_vcpus() (/proc/cpuinfo, sysfs, net-procfs) still use
the caller's limit.

Is this change intentional? The commit message only mentions that the VE
check is done against the current exec env, and does not mention that the
vCPU limit is now taken from the target task.

>  		cpumask_clear(mask);
> +		bitmap_set(cpumask_bits(mask), 0, vcpus_avail);

[Severity: Low]
Can this return an empty mask inside a CT?

vcpus_avail depends on cpumask_weight(mask), which is the weight of
p->cpus_mask & cpu_active_mask. If that intersection is empty, vcpus_avail
is 0 and bitmap_set(..., 0, 0) leaves the mask all zero.

One way this can happen is during a CPU hotplug window. The task's
cpus_mask may only hold CPUs that are already cleared from
cpu_active_mask. select_fallback_rq() does not repair the mask until the
task next wakes.

The old CT path used num_online_vcpus(), which is task_nr_cpus(current).
That function is clamped to at least num_online_cpus(), so it never
returned an empty mask. In this case nproc and similar tools would report
0.

Would a floor of 1 be appropriate here?

>  	}
>  
> -	guard(raw_spinlock_irqsave)(&p->pi_lock);
> -	cpumask_and(mask, &p->cpus_mask, cpu_active_mask);
> -
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261029-ct-invalid-nrpoc-v3-0-89d4b0b7d520@virtuozzo.com?part=1

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v3 1/2] sched: Fix sched_getaffinity syscall

[Vladimir Riabchun]

On 10/29/26 11:21, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Low] sched: sched_getaffinity() takes vCPU limit from target task, not caller
> - [Low] sched: sched_getaffinity() in CT can return an empty mask with no floor of 1
> --
> 
> commit 89d21190c29ff2e196f8d22db77072e4bd2c2544
> Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> 
> sched: Fix sched_getaffinity syscall
> 
> This patch changes sched_getaffinity() so that, inside a container, the
> reported mask is a prefix of set bits. The number of bits is the smaller of
> the task's vCPU limit and the weight of cpus_mask & cpu_active_mask. Host
> behavior is unchanged.
> 
> Link: https://virtuozzo.atlassian.net/browse/VSTOR-141682
> 
>> diff --git a/kernel/sched/syscalls.c b/kernel/sched/syscalls.c
>> --- a/kernel/sched/syscalls.c
>> +++ b/kernel/sched/syscalls.c
> 
> [ ... ]
> 
>> @@ -1316,15 +1316,23 @@ long sched_getaffinity(pid_t pid, struct cpumask *mask)
>>   	if (retval)
>>   		return retval;
>>   
>> +	scoped_guard (raw_spinlock_irqsave, &p->pi_lock) {
>> +		cpumask_and(mask, &p->cpus_mask, cpu_active_mask);
>> +	}
>> +
>>   	if (!ve_is_super(get_exec_env())) {
>> +		/*
>> +		 * setaffinity is not available inside CT, so these bits
>> +		 * can only be used for counting.
>> +		 * We can't have more than vcpus and we can't run on CPUs
>> +		 * outside the mask. Reporting smallest will provide true
>> +		 * information about CT resources.
>> +		 */
>> +		unsigned int vcpus_avail = min(task_nr_cpus(p), cpumask_weight(mask));
> 
> [Severity: Low]
> Previously the vCPU limit came from num_online_vcpus(), which is
> task_nr_cpus(current), so it was based on the caller. Now it comes from
> task_nr_cpus(p), the target task, while the VE check still uses the
> caller's exec env.
> 
> Can a task in a CT that calls sched_getaffinity(pid) for another task
> in a different task group now get a different CPU count than it gets for
> itself?
> 
> For example, a task in a child cpu cgroup has nr_cpus of 0, which is
> clamped to num_online_cpus(). The CT root group may have a lower limit.
> 
> Callers that query themselves, such as nproc, see no change. The other
> users of num_online_vcpus() (/proc/cpuinfo, sysfs, net-procfs) still use
> the caller's limit.
> 
> Is this change intentional? The commit message only mentions that the VE
> check is done against the current exec env, and does not mention that the
> vCPU limit is now taken from the target task.

Yes, it is intentional. I think the current approach is correct.

> 
>>   		cpumask_clear(mask);
>> +		bitmap_set(cpumask_bits(mask), 0, vcpus_avail);
> 
> [Severity: Low]
> Can this return an empty mask inside a CT?

Already replied in v2. It is fine.

> 
> vcpus_avail depends on cpumask_weight(mask), which is the weight of
> p->cpus_mask & cpu_active_mask. If that intersection is empty, vcpus_avail
> is 0 and bitmap_set(..., 0, 0) leaves the mask all zero.
> 
> One way this can happen is during a CPU hotplug window. The task's
> cpus_mask may only hold CPUs that are already cleared from
> cpu_active_mask. select_fallback_rq() does not repair the mask until the
> task next wakes.
> 
> The old CT path used num_online_vcpus(), which is task_nr_cpus(current).
> That function is clamped to at least num_online_cpus(), so it never
> returned an empty mask. In this case nproc and similar tools would report
> 0.
> 
> Would a floor of 1 be appropriate here?
> 
>>   	}
>>   
>> -	guard(raw_spinlock_irqsave)(&p->pi_lock);
>> -	cpumask_and(mask, &p->cpus_mask, cpu_active_mask);
>> -
>>   	return 0;
>>   }
> 

-- 
Best regards, Riabchun Vladimir
Linux Kernel Developer, Virtuozzo

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH hci-8.1 v3 2/2] selftests: ve: Test sched_getaffinity in VE

[Vladimir Riabchun]

Add a new selftest that verifies that sched_getaffinity respects both
cgroup cpuset and nr_cpus limits.

https://virtuozzo.atlassian.net/browse/VSTOR-141682

Signed-off-by: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
---
 tools/testing/selftests/ve/.gitignore              |   1 +
 tools/testing/selftests/ve/Makefile                |   1 +
 .../selftests/ve/ve_sched_getaffinity_test.c       | 211 +++++++++++++++++++++
 3 files changed, 213 insertions(+)

diff --git a/tools/testing/selftests/ve/.gitignore b/tools/testing/selftests/ve/.gitignore
index 8ab1bfe6f5e4..178de0ee1822 100644
--- a/tools/testing/selftests/ve/.gitignore
+++ b/tools/testing/selftests/ve/.gitignore
@@ -3,3 +3,4 @@ ve_perms_test
 ve_mount_accounting_test
 ve_cpuid_override_test
 ve_vtty_stress
+ve_sched_getaffinity_test
diff --git a/tools/testing/selftests/ve/Makefile b/tools/testing/selftests/ve/Makefile
index 51f9316beb99..d53e724205f7 100644
--- a/tools/testing/selftests/ve/Makefile
+++ b/tools/testing/selftests/ve/Makefile
@@ -6,6 +6,7 @@ TEST_GEN_PROGS += ve_ns_owner_test
 TEST_GEN_PROGS += ve_perms_test
 TEST_GEN_PROGS += ve_mount_accounting_test
 TEST_GEN_PROGS += ve_vtty_stress
+TEST_GEN_PROGS += ve_sched_getaffinity_test
 
 # CPUID faulting is x86-64 only. ARCH says "x86" for 32-bit too and lib.mk
 # picks the clang target by its emptiness, so decide by the compiler.
diff --git a/tools/testing/selftests/ve/ve_sched_getaffinity_test.c b/tools/testing/selftests/ve/ve_sched_getaffinity_test.c
new file mode 100644
index 000000000000..98acc246ede0
--- /dev/null
+++ b/tools/testing/selftests/ve/ve_sched_getaffinity_test.c
@@ -0,0 +1,211 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ve_sched_getaffinity selftests
+ *
+ * Tests to check cpuset virtualization.
+ * Not suitable for running inside CT.
+ */
+#define _GNU_SOURCE
+#include <asm/unistd.h>
+#include <linux/sched.h>
+#include <sys/syscall.h>
+#include <sys/stat.h>
+#include <errno.h>
+#include <sched.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+
+#include "../kselftest_harness.h"
+#include "ve_selftest.h"
+
+FIXTURE(ve_getaff_test)
+{
+	int cgv2_fd;
+	int ctid;
+	struct namespace_ctx ns_ctx;
+	cpu_set_t orig_cpuset;
+	int orig_cpucount;
+};
+
+static int __getaffinity_ve(void *cpuset)
+{
+	cpu_set_t *dst = cpuset;
+
+	return sched_getaffinity(getpid(), sizeof(*dst), dst);
+}
+
+static int getaffinity_ve(FIXTURE_DATA(ve_getaff_test) * self, cpu_set_t *dst)
+{
+	int ret = -1;
+	cpu_set_t *tmp = mmap(NULL, sizeof(cpu_set_t),
+		PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0);
+
+	if (tmp == MAP_FAILED) {
+		fprintf(stderr, "Failed to mmap shared region: %s\n", strerror(errno));
+		return -1;
+	}
+	CPU_ZERO(tmp);
+	CPU_ZERO(dst);
+
+	ret = run_in_ve(self->cgv2_fd, self->ctid, &self->ns_ctx,
+		CLONE_NEWVE, __getaffinity_ve, tmp);
+
+	if (!ret)
+		memcpy(dst, tmp, sizeof(cpu_set_t));
+
+	munmap(tmp, sizeof(cpu_set_t));
+	return ret;
+}
+
+static int check_cpumask_is_prefix(cpu_set_t *mask, int expected)
+{
+	int i, count = CPU_COUNT(mask);
+
+	if (expected >= 0 && expected != count) {
+		fprintf(stderr, "cpumask count mismatch: %d != %d\n", expected, count);
+		return 0;
+	}
+	for (i = 0; i < count; i++) {
+		if (!CPU_ISSET(i, mask)) {
+			fprintf(stderr, "cpumask is not a prefix: %d is OFF\n", i);
+			return 0;
+		}
+	}
+	return 1;
+}
+
+static int get_nth_cpu(cpu_set_t *mask, int n)
+{
+	int i;
+
+	for (i = 0; i < sizeof(*mask); i++) {
+		if (!CPU_ISSET(i, mask) || --n)
+			continue;
+
+		return i;
+	}
+	return -1;
+}
+
+FIXTURE_SETUP(ve_getaff_test)
+{
+	ASSERT_EQ(sched_getaffinity(getpid(), sizeof(cpu_set_t), &self->orig_cpuset), 0);
+	self->orig_cpucount = CPU_COUNT(&self->orig_cpuset);
+	if (self->orig_cpucount < 2)
+		SKIP(return, "Not enough CPUs available");
+
+	self->cgv2_fd = mount_cg2_fd();
+	ASSERT_GE(self->cgv2_fd, 0);
+
+	ASSERT_EQ(write_file_at(self->cgv2_fd, "cgroup.subtree_control",
+		  VE_CONTROLLERS), 0);
+
+	self->ctid = make_ve(self->cgv2_fd, CTID_MIN);
+	ASSERT_GE(self->ctid, 0);
+
+	ASSERT_EQ(prepare_namespaces(&self->ns_ctx, CLONE_NEWVE,
+				self->cgv2_fd, self->ctid), 0);
+}
+
+FIXTURE_TEARDOWN(ve_getaff_test)
+{
+	close_namespace_fds(&self->ns_ctx);
+	destroy_ve(self->cgv2_fd, self->ctid);
+	close(self->cgv2_fd);
+}
+
+/* Test correct processing of cpuset.cpus updates */
+TEST_F(ve_getaff_test, update_cg_cpuset)
+{
+	char path[PATH_MAX];
+	int cpu_leave;
+	cpu_set_t in_ve;
+
+	snprintf(path, sizeof(path), "%d/cpuset.cpus", self->ctid);
+	if (faccessat(self->cgv2_fd, path, F_OK, 0))
+		SKIP(return, "CT cgroup doesn't have cpuset.cpus entry");
+
+	ASSERT_EQ(getaffinity_ve(self, &in_ve), 0);
+	ASSERT_TRUE(check_cpumask_is_prefix(&in_ve, self->orig_cpucount));
+
+	/* Find two CPUs and leave the second via cgroup interface */
+	cpu_leave = get_nth_cpu(&self->orig_cpuset, 2);
+	ASSERT_GE(cpu_leave, 0);
+	ASSERT_EQ(write_u64_at(self->cgv2_fd, path, cpu_leave), 0);
+
+	/* Verify sched_getaffinity sees 1 CPU, that is the first one */
+	ASSERT_EQ(getaffinity_ve(self, &in_ve), 0);
+	ASSERT_TRUE(check_cpumask_is_prefix(&in_ve, 1));
+}
+
+/* Test correct processing of cpu.nr_cpus updates */
+TEST_F(ve_getaff_test, nr_proc_bound)
+{
+	char path[PATH_MAX];
+	cpu_set_t in_ve;
+
+	snprintf(path, sizeof(path), "%d/cpu.nr_cpus", self->ctid);
+	if (faccessat(self->cgv2_fd, path, F_OK, 0))
+		SKIP(return, "CT cgroup doesn't have cpu.nr_cpus entry");
+
+	/* We expect here CPU_COUNT >= 2 */
+	ASSERT_EQ(getaffinity_ve(self, &in_ve), 0);
+	ASSERT_TRUE(check_cpumask_is_prefix(&in_ve, self->orig_cpucount));
+
+	/* Leave one vCPU */
+	ASSERT_EQ(write_u64_at(self->cgv2_fd, path, 1), 0);
+
+	/* Verify sched_getaffinity sees only one vCPU */
+	ASSERT_EQ(getaffinity_ve(self, &in_ve), 0);
+	ASSERT_TRUE(check_cpumask_is_prefix(&in_ve, 1));
+}
+
+/* Test correct interaction of cpu.nr_cpus and cpuset.cpus */
+TEST_F(ve_getaff_test, limits_interaction)
+{
+	char path_nr[PATH_MAX], path_set[PATH_MAX], tmp[128];
+	int c1, c3;
+	cpu_set_t in_ve;
+
+	snprintf(path_nr, sizeof(path_nr), "%d/cpu.nr_cpus", self->ctid);
+	if (faccessat(self->cgv2_fd, path_nr, F_OK, 0))
+		SKIP(return, "CT cgroup doesn't have cpu.nr_cpus entry");
+
+	snprintf(path_set, sizeof(path_set), "%d/cpuset.cpus", self->ctid);
+	if (faccessat(self->cgv2_fd, path_set, F_OK, 0))
+		SKIP(return, "CT cgroup doesn't have cpuset.cpus entry");
+
+	if (self->orig_cpucount < 3)
+		SKIP(return, "This test requires at least 3 CPUs");
+
+	/* We expect here CPU_COUNT >= 3 */
+	ASSERT_EQ(getaffinity_ve(self, &in_ve), 0);
+	ASSERT_TRUE(check_cpumask_is_prefix(&in_ve, self->orig_cpucount));
+
+	/* Leave two CPUs in cpuset */
+	c1 = get_nth_cpu(&self->orig_cpuset, 1);
+	c3 = get_nth_cpu(&self->orig_cpuset, 3);
+	ASSERT_TRUE(c1 >= 0 && c3 >= 0);
+
+	snprintf(tmp, sizeof(tmp), "%d,%d", c1, c3);
+	ASSERT_EQ(write_file_at(self->cgv2_fd, path_set, tmp), 0);
+
+	/* Verify sched_getaffinity sees two vCPU */
+	ASSERT_EQ(getaffinity_ve(self, &in_ve), 0);
+	ASSERT_TRUE(check_cpumask_is_prefix(&in_ve, 2));
+
+	/* Set vCPU limit to 2 and check that we still have two vCPUs */
+	ASSERT_EQ(write_u64_at(self->cgv2_fd, path_nr, 2), 0);
+	ASSERT_EQ(getaffinity_ve(self, &in_ve), 0);
+	ASSERT_TRUE(check_cpumask_is_prefix(&in_ve, 2));
+
+	/* Set vCPU limit to 1 and check that we have one vCPU only */
+	ASSERT_EQ(write_u64_at(self->cgv2_fd, path_nr, 1), 0);
+	ASSERT_EQ(getaffinity_ve(self, &in_ve), 0);
+	ASSERT_TRUE(check_cpumask_is_prefix(&in_ve, 1));
+
+}
+
+TEST_HARNESS_MAIN

-- 
2.52.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v3 2/2] selftests: ve: Test sched_getaffinity in VE

[no-reply]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] selftests/ve: get_nth_cpu() bounds loop by sizeof(cpu_set_t) bytes, not CPU_SETSIZE
- [Medium] selftests/ve: expected CT CPU count taken from caller's affinity, not CT cpuset
- [Low] selftests/ve: FIXTURE_SETUP leaks CT cgroup if prepare_namespaces() ASSERT fails
--

commit 55fc819ea7f7954caf6a8e968697a2cc48fd1982
Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>

selftests: ve: Test sched_getaffinity in VE

This adds a selftest that checks sched_getaffinity() inside a VE respects
both the cgroup cpuset and the cpu.nr_cpus limit. It adds three test cases
and registers the binary in the Makefile and .gitignore.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-141682

> diff --git a/tools/testing/selftests/ve/ve_sched_getaffinity_test.c b/tools/testing/selftests/ve/ve_sched_getaffinity_test.c
> new file mode 100644
> index 000000000000..98acc246ede0
> --- /dev/null
> +++ b/tools/testing/selftests/ve/ve_sched_getaffinity_test.c

[ ... ]

> +static int get_nth_cpu(cpu_set_t *mask, int n)
> +{
> +	int i;
> +
> +	for (i = 0; i < sizeof(*mask); i++) {
> +		if (!CPU_ISSET(i, mask) || --n)
> +			continue;
> +
> +		return i;
> +	}
> +	return -1;
> +}

[Severity: Medium]
Is the loop bound in get_nth_cpu() correct?  sizeof(*mask) is the size of
cpu_set_t in bytes (128), while CPU_ISSET() takes a bit index and the set
holds CPU_SETSIZE (1024) bits:

    for (i = 0; i < sizeof(*mask); i++) {
        if (!CPU_ISSET(i, mask) || --n)

The scan stops at CPU 127.  If the 2nd or 3rd allowed CPU in orig_cpuset
is numbered 128 or higher (for example under taskset, a restricted cpuset,
or a large sparse host), get_nth_cpu() returns -1.

update_cg_cpuset then fails ASSERT_GE(cpu_leave, 0), and limits_interaction
fails ASSERT_TRUE(c1 >= 0 && c3 >= 0), without sched_getaffinity() in the
VE being exercised.

The loop also compares a signed int with a size_t.  Should the bound be
CPU_SETSIZE?

> +FIXTURE_SETUP(ve_getaff_test)
> +{
> +	ASSERT_EQ(sched_getaffinity(getpid(), sizeof(cpu_set_t), &self->orig_cpuset), 0);
> +	self->orig_cpucount = CPU_COUNT(&self->orig_cpuset);

[Severity: Medium]
Is orig_cpucount the right expected value for the CT?  It is taken from the
runner's own affinity mask, not from the CT cgroup's effective cpuset.

In getaffinity_ve(), run_in_ve() forks a child that does:

    enter_cgroup(cgv2_fd, ctid);
    enter_namespaces(ctx, flags);
    ret = fn(arg);

Nothing in the child restores the parent's affinity after joining the
cgroup.  So the mask that __getaffinity_ve() reports derives from the CT
cgroup's cpuset, which is inherited from the parent cgroup.

I did not read cpuset_attach() in this tree.  From memory of mainline, it
resets cpus_mask to the cgroup's effective_cpus on attach, but that is
unverified.

If the runner was restricted, for example with taskset -c 0,1 on a host
with more CPUs, the CT may report more CPUs than orig_cpucount.  Then this
fails in update_cg_cpuset, nr_proc_bound and limits_interaction:

    ASSERT_TRUE(check_cpumask_is_prefix(&in_ve, self->orig_cpucount));

Would it be better to derive the expected count from the CT cgroup's
effective cpuset, or to skip when the two differ?

Separately, sched_getaffinity() with a fixed sizeof(cpu_set_t) returns
EINVAL when nr_cpu_ids is greater than 1024.  FIXTURE_SETUP asserts on that
call rather than skipping.

[ ... ]

> +	self->ctid = make_ve(self->cgv2_fd, CTID_MIN);
> +	ASSERT_GE(self->ctid, 0);
> +
> +	ASSERT_EQ(prepare_namespaces(&self->ns_ctx, CLONE_NEWVE,
> +				self->cgv2_fd, self->ctid), 0);
> +}

[Severity: Low]
Can this leak the CT cgroup created by make_ve()?  If prepare_namespaces()
fails, the ASSERT_EQ() aborts FIXTURE_SETUP before setup_completed is set.

In kselftest_harness.h, __TEST_F_IMPL only calls the teardown when
_metadata->setup_completed is true:

    if (_metadata->setup_completed && !fixture_name##_teardown_parent && \
            __sync_bool_compare_and_swap(teardown, false, true)) \
        fixture_name##_teardown(_metadata, self, variant->data); \

So FIXTURE_TEARDOWN(ve_getaff_test) never runs.  close_namespace_fds(),
destroy_ve() and close(cgv2_fd) are all skipped.

I inferred, but did not verify in this tree, that the cgroup persists after
the process exits.  If so, each leaked cgroup keeps the first free id in
the CTID_MIN..CTID_MAX range that make_ve() scans:

    for (id = from; id < CTID_MAX; id++) {
        snprintf(name, sizeof(name), "%d", id);
        if (faccessat(cgv2_fd, name, F_OK, 0) != 0 && errno == ENOENT)
            break;
    }

This only happens on a failure path.  The same pattern exists in other ve
selftests such as ve_ns_owner_test and ve_perms_test, so this patch adds
another instance rather than a new class of problem.

Could the cleanup be done on that failure path, for example by destroying
the CT before asserting?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261029-ct-invalid-nrpoc-v3-0-89d4b0b7d520@virtuozzo.com?part=2

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v3 2/2] selftests: ve: Test sched_getaffinity in VE

[Vladimir Riabchun]

On 10/29/26 11:22, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Medium] selftests/ve: get_nth_cpu() bounds loop by sizeof(cpu_set_t) bytes, not CPU_SETSIZE

Replied in v2.

> - [Medium] selftests/ve: expected CT CPU count taken from caller's affinity, not CT cpuset

This is not a CT. This test is a root-only, no need for complications.

> - [Low] selftests/ve: FIXTURE_SETUP leaks CT cgroup if prepare_namespaces() ASSERT fails

This is fine IMO. With skip it was correct to move it to the top.
Here if something fails we should leave all resources, to simplify
debugging.

> --
> 
> commit 55fc819ea7f7954caf6a8e968697a2cc48fd1982
> Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> 
> selftests: ve: Test sched_getaffinity in VE
> 
> This adds a selftest that checks sched_getaffinity() inside a VE respects
> both the cgroup cpuset and the cpu.nr_cpus limit. It adds three test cases
> and registers the binary in the Makefile and .gitignore.
> 
> Link: https://virtuozzo.atlassian.net/browse/VSTOR-141682
> 
>> diff --git a/tools/testing/selftests/ve/ve_sched_getaffinity_test.c b/tools/testing/selftests/ve/ve_sched_getaffinity_test.c
>> new file mode 100644
>> index 000000000000..98acc246ede0
>> --- /dev/null
>> +++ b/tools/testing/selftests/ve/ve_sched_getaffinity_test.c
> 
> [ ... ]
> 
>> +static int get_nth_cpu(cpu_set_t *mask, int n)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < sizeof(*mask); i++) {
>> +		if (!CPU_ISSET(i, mask) || --n)
>> +			continue;
>> +
>> +		return i;
>> +	}
>> +	return -1;
>> +}
> 
> [Severity: Medium]
> Is the loop bound in get_nth_cpu() correct?  sizeof(*mask) is the size of
> cpu_set_t in bytes (128), while CPU_ISSET() takes a bit index and the set
> holds CPU_SETSIZE (1024) bits:
> 
>      for (i = 0; i < sizeof(*mask); i++) {
>          if (!CPU_ISSET(i, mask) || --n)
> 
> The scan stops at CPU 127.  If the 2nd or 3rd allowed CPU in orig_cpuset
> is numbered 128 or higher (for example under taskset, a restricted cpuset,
> or a large sparse host), get_nth_cpu() returns -1.
> 
> update_cg_cpuset then fails ASSERT_GE(cpu_leave, 0), and limits_interaction
> fails ASSERT_TRUE(c1 >= 0 && c3 >= 0), without sched_getaffinity() in the
> VE being exercised.
> 
> The loop also compares a signed int with a size_t.  Should the bound be
> CPU_SETSIZE?
> 
>> +FIXTURE_SETUP(ve_getaff_test)
>> +{
>> +	ASSERT_EQ(sched_getaffinity(getpid(), sizeof(cpu_set_t), &self->orig_cpuset), 0);
>> +	self->orig_cpucount = CPU_COUNT(&self->orig_cpuset);
> 
> [Severity: Medium]
> Is orig_cpucount the right expected value for the CT?  It is taken from the
> runner's own affinity mask, not from the CT cgroup's effective cpuset.
> 
> In getaffinity_ve(), run_in_ve() forks a child that does:
> 
>      enter_cgroup(cgv2_fd, ctid);
>      enter_namespaces(ctx, flags);
>      ret = fn(arg);
> 
> Nothing in the child restores the parent's affinity after joining the
> cgroup.  So the mask that __getaffinity_ve() reports derives from the CT
> cgroup's cpuset, which is inherited from the parent cgroup.
> 
> I did not read cpuset_attach() in this tree.  From memory of mainline, it
> resets cpus_mask to the cgroup's effective_cpus on attach, but that is
> unverified.
> 
> If the runner was restricted, for example with taskset -c 0,1 on a host
> with more CPUs, the CT may report more CPUs than orig_cpucount.  Then this
> fails in update_cg_cpuset, nr_proc_bound and limits_interaction:
> 
>      ASSERT_TRUE(check_cpumask_is_prefix(&in_ve, self->orig_cpucount));
> 
> Would it be better to derive the expected count from the CT cgroup's
> effective cpuset, or to skip when the two differ?
> 
> Separately, sched_getaffinity() with a fixed sizeof(cpu_set_t) returns
> EINVAL when nr_cpu_ids is greater than 1024.  FIXTURE_SETUP asserts on that
> call rather than skipping.
> 
> [ ... ]
> 
>> +	self->ctid = make_ve(self->cgv2_fd, CTID_MIN);
>> +	ASSERT_GE(self->ctid, 0);
>> +
>> +	ASSERT_EQ(prepare_namespaces(&self->ns_ctx, CLONE_NEWVE,
>> +				self->cgv2_fd, self->ctid), 0);
>> +}
> 
> [Severity: Low]
> Can this leak the CT cgroup created by make_ve()?  If prepare_namespaces()
> fails, the ASSERT_EQ() aborts FIXTURE_SETUP before setup_completed is set.
> 
> In kselftest_harness.h, __TEST_F_IMPL only calls the teardown when
> _metadata->setup_completed is true:
> 
>      if (_metadata->setup_completed && !fixture_name##_teardown_parent && \
>              __sync_bool_compare_and_swap(teardown, false, true)) \
>          fixture_name##_teardown(_metadata, self, variant->data); \
> 
> So FIXTURE_TEARDOWN(ve_getaff_test) never runs.  close_namespace_fds(),
> destroy_ve() and close(cgv2_fd) are all skipped.
> 
> I inferred, but did not verify in this tree, that the cgroup persists after
> the process exits.  If so, each leaked cgroup keeps the first free id in
> the CTID_MIN..CTID_MAX range that make_ve() scans:
> 
>      for (id = from; id < CTID_MAX; id++) {
>          snprintf(name, sizeof(name), "%d", id);
>          if (faccessat(cgv2_fd, name, F_OK, 0) != 0 && errno == ENOENT)
>              break;
>      }
> 
> This only happens on a failure path.  The same pattern exists in other ve
> selftests such as ve_ns_owner_test and ve_perms_test, so this patch adds
> another instance rather than a new class of problem.
> 
> Could the cleanup be done on that failure path, for example by destroying
> the CT before asserting?
> 

-- 
Best regards, Riabchun Vladimir
Linux Kernel Developer, Virtuozzo

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel