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

[Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>]

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