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

[no-reply@sashiko.virtuozzo.com]

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