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

[Devel] [PATCH hci-8.1 v2 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 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       | 213 +++++++++++++++++++++
 4 files changed, 228 insertions(+), 5 deletions(-)
---
base-commit: a1c0fd720976500f854fd5a4c946a54e65c82d78
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 v2 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.

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..265d3a96d227 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(num_online_vcpus(), 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

[Devel] [PATCH hci-8.1 v2 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       | 213 +++++++++++++++++++++
 3 files changed, 215 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..376b65a215f1
--- /dev/null
+++ b/tools/testing/selftests/ve/ve_sched_getaffinity_test.c
@@ -0,0 +1,213 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ve_mount_accounting 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)
+{
+	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);
+
+
+	ASSERT_EQ(sched_getaffinity(getpid(), sizeof(cpu_set_t), &self->orig_cpuset), 0);
+	if (CPU_COUNT(&self->orig_cpuset) < 2)
+		SKIP(return, "Not enough CPUs available");
+
+	self->orig_cpucount = CPU_COUNT(&self->orig_cpuset);
+};
+
+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 v2 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() mixes current's vCPU limit with target task's mask weight
- [Low] sched: sched_getaffinity() in CT can report an empty mask with success
--

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

sched: Fix sched_getaffinity syscall

This patch changes sched_getaffinity() so that inside a container it
computes the intersection of the target task's cpus_mask and
cpu_active_mask, then reports the first min(num_online_vcpus(),
weight) bits as set. The host path 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(num_online_vcpus(), cpumask_weight(mask));

[Severity: Low]
Can this combine values from two different tasks?

num_online_vcpus() is defined as task_nr_cpus(current), while
cpumask_weight(mask) is computed from the cpus_mask of the task p that
was looked up by pid.

task_nr_cpus() reads task_group(p)->nr_cpus, which is the task's own
cgroup and not the CT root group. nr_cpus_write_u64() is registered for
every non-root cgroup, so sub-cgroups inside one CT can have different
nr_cpus limits.

If a process in a CT calls sched_getaffinity() with pid != 0 on a task in
a different sub-cgroup with a different cpu.nr_cpus value, the count
returned for p depends on the caller's limit and not on p's.

Should the vCPU limit be taken from p instead of current? The old code
also used only current's limit, so this is not a new dependency.

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

[Severity: Low]
Can this return success with an empty mask?

cpumask_and() can produce an empty intersection transiently, for example
when p->cpus_mask has only CPUs just cleared from cpu_active_mask, before
balance_push or select_fallback_rq has rewritten the mask. A cpuset whose
CPUs all go offline before the cpuset hotplug work updates task masks
gives the same result.

In that window cpumask_weight(mask) is 0, so:

    vcpus_avail = min(num_online_vcpus(), 0) = 0;
    bitmap_set(cpumask_bits(mask), 0, 0);

bitmap_set() is then a no-op and the function returns 0 with an all-zero
mask. num_online_vcpus() is not the cause, since task_nr_cpus() clamps
the result to at least 1.

The old CT branch never read p->cpus_mask and always set at least one
bit. Callers such as nproc would count 0 CPUs here. Should there be a
fallback to at least one bit?

>  	}
>  
> -	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/20261028-ct-invalid-nrpoc-v2-0-968fe985ce27@virtuozzo.com?part=1

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

Re: [Devel] [PATCH hci-8.1 v2 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: FIXTURE_SETUP skips teardown, leaking VE cgroup and fds
- [Medium] selftests: ve: get_nth_cpu() loop bound uses sizeof(*mask) instead of CPU_SETSIZE
- [Low] selftests: ve: copy-pasted header comment and stray semicolon after FIXTURE_SETUP
--

commit 72cb69c9cdabbcdd75bce8639364f25d3d6973b0
Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>

selftests: ve: Test sched_getaffinity in VE

This adds a new selftest, ve_sched_getaffinity_test, which checks that
sched_getaffinity() inside a VE respects both the cgroup cpuset and the
cpu.nr_cpus limits. It also registers the test 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 0000000000000..376b65a215f12
> --- /dev/null
> +++ b/tools/testing/selftests/ve/ve_sched_getaffinity_test.c
> @@ -0,0 +1,213 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ve_mount_accounting selftests
> + *

[Severity: Low]
This isn't a bug, but the header comment names ve_mount_accounting, which
looks like it was copied from another test. This file tests
sched_getaffinity() and the cpuset and nr_cpus virtualization.

[ ... ]

> +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), but CPU_ISSET() takes a bit index up to
CPU_SETSIZE (1024).

As written, only CPU ids 0..127 are scanned. If the nth allowed CPU has an
id of 128 or more, for example on a large host or under taskset, the
function returns -1.

In that case update_cg_cpuset fails on ASSERT_GE(cpu_leave, 0). The
limits_interaction test fails on ASSERT_TRUE(c1 >= 0 && c3 >= 0).

Would "i < CPU_SETSIZE" be the intended bound? The loop also compares a
signed int against a size_t.

> +FIXTURE_SETUP(ve_getaff_test)
> +{
> +	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);
> +
> +
> +	ASSERT_EQ(sched_getaffinity(getpid(), sizeof(cpu_set_t), &self->orig_cpuset), 0);
> +	if (CPU_COUNT(&self->orig_cpuset) < 2)
> +		SKIP(return, "Not enough CPUs available");
> +
> +	self->orig_cpucount = CPU_COUNT(&self->orig_cpuset);
> +};

[Severity: Medium]
Can this leak the VE cgroup and the namespace and cgroup fds? The
sched_getaffinity() ASSERT and the SKIP(return, "Not enough CPUs
available") both run after mount_cg2_fd(), make_ve() and
prepare_namespaces() have already succeeded.

In kselftest_harness.h, __TEST_F_IMPL() does this after the setup call:

    if (_metadata->exit_code)
        _exit(0);
    _metadata->setup_completed = true;

When setup exits early, setup_completed is never set. The teardown call is
guarded by setup_completed, so FIXTURE_TEARDOWN never runs.
close_namespace_fds(), destroy_ve() and close() are all skipped.

On a host with fewer than 2 allowed CPUs, each run would leave a CT cgroup
behind and use up a CTID.

Could the CPU count check be moved before any resources are created, or
could the resources be cleaned up explicitly before the SKIP and the
failing ASSERT paths?

Also, FIXTURE_SETUP ends with a stray semicolon ("};"), and there are
extra blank lines in this function. mmap() and munmap() are used without
including <sys/mman.h>, which currently only comes in transitively through
kselftest_harness.h.

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261028-ct-invalid-nrpoc-v2-0-968fe985ce27@virtuozzo.com?part=2

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

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

[Pavel Tikhomirov]

Looks good. (except that sashiko reveiw should be handled)

Reviewed-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

On 10/28/26 13:46, Vladimir Riabchun wrote:
> 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 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       | 213 +++++++++++++++++++++
>  4 files changed, 228 insertions(+), 5 deletions(-)
> ---
> base-commit: a1c0fd720976500f854fd5a4c946a54e65c82d78
> change-id: 20261027-ct-invalid-nrpoc-f183b3ba691d
> 
> Best regards,
> --  
> Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>

-- 
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.

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

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

[Vladimir Riabchun]

On 10/28/26 13:50, 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() mixes current's vCPU limit with target task's mask weight
> - [Low] sched: sched_getaffinity() in CT can report an empty mask with success
> --
> 
> commit c2d8d2fc58cee7091fb732ace961d0d72a6ccb84
> Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> 
> sched: Fix sched_getaffinity syscall
> 
> This patch changes sched_getaffinity() so that inside a container it
> computes the intersection of the target task's cpus_mask and
> cpu_active_mask, then reports the first min(num_online_vcpus(),
> weight) bits as set. The host path 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(num_online_vcpus(), cpumask_weight(mask));
> 
> [Severity: Low]
> Can this combine values from two different tasks?
> 
> num_online_vcpus() is defined as task_nr_cpus(current), while
> cpumask_weight(mask) is computed from the cpus_mask of the task p that
> was looked up by pid.
> 
> task_nr_cpus() reads task_group(p)->nr_cpus, which is the task's own
> cgroup and not the CT root group. nr_cpus_write_u64() is registered for
> every non-root cgroup, so sub-cgroups inside one CT can have different
> nr_cpus limits.
> 
> If a process in a CT calls sched_getaffinity() with pid != 0 on a task in
> a different sub-cgroup with a different cpu.nr_cpus value, the count
> returned for p depends on the caller's limit and not on p's.
> 
> Should the vCPU limit be taken from p instead of current? The old code
> also used only current's limit, so this is not a new dependency.

Correct, will fix in v3.

> 
>>   		cpumask_clear(mask);
>>   		bitmap_set(cpumask_bits(mask), 0, vcpus_avail);
> 
> [Severity: Low]
> Can this return success with an empty mask?
> 
> cpumask_and() can produce an empty intersection transiently, for example
> when p->cpus_mask has only CPUs just cleared from cpu_active_mask, before
> balance_push or select_fallback_rq has rewritten the mask. A cpuset whose
> CPUs all go offline before the cpuset hotplug work updates task masks
> gives the same result.
> 
> In that window cpumask_weight(mask) is 0, so:
> 
>      vcpus_avail = min(num_online_vcpus(), 0) = 0;
>      bitmap_set(cpumask_bits(mask), 0, 0);
> 
> bitmap_set() is then a no-op and the function returns 0 with an all-zero
> mask. num_online_vcpus() is not the cause, since task_nr_cpus() clamps
> the result to at least 1.
> 
> The old CT branch never read p->cpus_mask and always set at least one
> bit. Callers such as nproc would count 0 CPUs here. Should there be a
> fallback to at least one bit?

I see no problem in returning a zero mask. If the task has no CPUs
to run because of user config - a pity, but not ours.

> 
>>   	}
>>   
>> -	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

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

[Vladimir Riabchun]

On 10/28/26 13:51, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Medium] selftests: ve: FIXTURE_SETUP skips teardown, leaking VE cgroup and fds
> - [Medium] selftests: ve: get_nth_cpu() loop bound uses sizeof(*mask) instead of CPU_SETSIZE
> - [Low] selftests: ve: copy-pasted header comment and stray semicolon after FIXTURE_SETUP
> --
> 
> commit 72cb69c9cdabbcdd75bce8639364f25d3d6973b0
> Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> 
> selftests: ve: Test sched_getaffinity in VE
> 
> This adds a new selftest, ve_sched_getaffinity_test, which checks that
> sched_getaffinity() inside a VE respects both the cgroup cpuset and the
> cpu.nr_cpus limits. It also registers the test 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 0000000000000..376b65a215f12
>> --- /dev/null
>> +++ b/tools/testing/selftests/ve/ve_sched_getaffinity_test.c
>> @@ -0,0 +1,213 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * ve_mount_accounting selftests
>> + *
> 
> [Severity: Low]
> This isn't a bug, but the header comment names ve_mount_accounting, which
> looks like it was copied from another test. This file tests
> sched_getaffinity() and the cpuset and nr_cpus virtualization.

A shame, didn't notice. Will fix in v3.

> 
> [ ... ]
> 
>> +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), but CPU_ISSET() takes a bit index up to
> CPU_SETSIZE (1024).
> 
> As written, only CPU ids 0..127 are scanned. If the nth allowed CPU has an
> id of 128 or more, for example on a large host or under taskset, the
> function returns -1.
> 
> In that case update_cg_cpuset fails on ASSERT_GE(cpu_leave, 0). The
> limits_interaction test fails on ASSERT_TRUE(c1 >= 0 && c3 >= 0).
> 
> Would "i < CPU_SETSIZE" be the intended bound? The loop also compares a
> signed int against a size_t.

Wouldn't bother:
1. We don't run our tests on hosts with 128+ CPUs.
2. This test is supposed to be run as root, so I, as a test author,
    really expect to see all CPUs available. If someone disabled first
    128 CPUs and left on 129..., then something is going terribly wrong.

> 
>> +FIXTURE_SETUP(ve_getaff_test)
>> +{
>> +	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);
>> +
>> +
>> +	ASSERT_EQ(sched_getaffinity(getpid(), sizeof(cpu_set_t), &self->orig_cpuset), 0);
>> +	if (CPU_COUNT(&self->orig_cpuset) < 2)
>> +		SKIP(return, "Not enough CPUs available");
>> +
>> +	self->orig_cpucount = CPU_COUNT(&self->orig_cpuset);
>> +};
> 
> [Severity: Medium]
> Can this leak the VE cgroup and the namespace and cgroup fds? The
> sched_getaffinity() ASSERT and the SKIP(return, "Not enough CPUs
> available") both run after mount_cg2_fd(), make_ve() and
> prepare_namespaces() have already succeeded.
> 
> In kselftest_harness.h, __TEST_F_IMPL() does this after the setup call:
> 
>      if (_metadata->exit_code)
>          _exit(0);
>      _metadata->setup_completed = true;
> 
> When setup exits early, setup_completed is never set. The teardown call is
> guarded by setup_completed, so FIXTURE_TEARDOWN never runs.
> close_namespace_fds(), destroy_ve() and close() are all skipped.
> 
> On a host with fewer than 2 allowed CPUs, each run would leave a CT cgroup
> behind and use up a CTID.
> 
> Could the CPU count check be moved before any resources are created, or
> could the resources be cleaned up explicitly before the SKIP and the
> failing ASSERT paths?

Cool, this is true. I'll move cpu check to the beginning.

> 
> Also, FIXTURE_SETUP ends with a stray semicolon ("};"), and there are
> extra blank lines in this function. mmap() and munmap() are used without
> including <sys/mman.h>, which currently only comes in transitively through
> kselftest_harness.h.
> 

Also true, will fix in v3.

> [ ... ]
> 

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

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