[BUG] v6.3-rc2 regresses sched_getaffinity() for arm64

[BUG] v6.3-rc2 regresses sched_getaffinity() for arm64

[Ryan Roberts]

Hi Linus,

I need to report a regression in v6.3-rc2 where sched_getaffinity() returns an
incorrect cpu_set, at least when running on arm64. Git bisect shows this patch
as the culprit, authored by you:

  596ff4a09b89 cpumask: re-introduce constant-sized cpumask optimizations

Apologies if this is the wrong channel for reporting this - I couldn't find a
suitable mail on the list for this patch to reply to. Happy to direct it
somewhere else if appropriate.


Details:

I'm running v6.3-rc2 kernel in a VM on Ampere Altra (arm64 system). The VM is
assigned 8 vCPUs. The kernel is defconfig and I'm booting into an Ubuntu
user-space. `nproc` returns a value that fluctuates from call to call in the
range ~80-100. If I run with v6.2, nproc always returns 8, as expected.

nproc is calling sched_getaffinity() with a 1024 entry cpu_set mask, then adds
up all the set bits to find the number of CPUs. I wrote a test program and can
see that the first 8 bits are always correctly set and most of the other bits
are always correctly 0. But bits ~64-224 are randomly set/clear from call to call.


Test program:

#define _GNU_SOURCE             /* See feature_test_macros(7) */
#include <sched.h>
#include <stdio.h>

#define SET_SIZE 1024

static void print_cpu_set(cpu_set_t *cpu_set)
{
	int ret, i, j, k;

	printf("cpu_count=%d\n", CPU_COUNT(cpu_set));
	for (i = 0; i < SET_SIZE;) {
		printf("[%03d]: ", i);
		for (k = 0; k < 8; k++) {
			for (j = 0; j < 8; j++, i++) {
				printf("%d", CPU_ISSET(i, cpu_set));
			}
			printf("  ");
		}
		printf("\n");
	}
}

int main()
{
	int ret;
	cpu_set_t *cpu_set;
	size_t size;

	cpu_set = CPU_ALLOC(SET_SIZE);
	size = CPU_ALLOC_SIZE(SET_SIZE);
	CPU_ZERO(cpu_set);

	printf("before:\n");
	print_cpu_set(cpu_set);
	ret = sched_getaffinity(0, size, cpu_set);
	printf("ret=%d\n", ret);
	printf("after:\n");
	print_cpu_set(cpu_set);

	return 0;
}


Broken output on v6.3-rc2:

before:
cpu_count=0
[000]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[064]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[128]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[192]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[256]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[320]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[384]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[448]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[512]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[576]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[640]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[704]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[768]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[832]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[896]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[960]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
ret=0
after:
cpu_count=82
[000]: 11111111 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[064]: 00000100 10110111 00110010 01101001 11111111 11111111 00000000 00000000
[128]: 00010101 00001101 11011111 10001110 11110001 10100101 11111111 11111111
[192]: 00000000 00001000 00000000 00000100 00000000 00000000 00000000 00000000
[256]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[320]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[384]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[448]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[512]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[576]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[640]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[704]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[768]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[832]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[896]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[960]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000


Correct output in v6.2:

before:
cpu_count=0
[000]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[064]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[128]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[192]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[256]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[320]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[384]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[448]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[512]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[576]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[640]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[704]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[768]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[832]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[896]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[960]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
ret=0
after:
cpu_count=8
[000]: 11111111 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[064]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[128]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[192]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[256]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[320]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[384]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[448]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[512]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[576]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[640]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[704]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[768]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[832]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[896]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[960]: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000


Thanks,
Ryan

Re: [BUG] v6.3-rc2 regresses sched_getaffinity() for arm64

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1762 bytes --]

On Tue, Mar 14, 2023 at 3:41 PM Ryan Roberts <ryan.roberts@arm.com> wrote:
>
> Apologies if this is the wrong channel for reporting this - I couldn't find a
> suitable mail on the list for this patch to reply to. Happy to direct it
> somewhere else if appropriate.

No, this is good.

> nproc is calling sched_getaffinity() with a 1024 entry cpu_set mask, then adds
> up all the set bits to find the number of CPUs. I wrote a test program and can
> see that the first 8 bits are always correctly set and most of the other bits
> are always correctly 0. But bits ~64-224 are randomly set/clear from call to call.

Ahh.

Yes, I see what's happening. The code does

                unsigned int retlen = min(len, cpumask_size());

and our cpu mask allocation size is set to 4 words - but since your
'nr_cpu_ids' is just 8, only the first word has actually been filled
with valid data.

That "cpumask_size()" thing is meant to be how big the allocation size
is, but clearly there is at least one user that has then taken it to
mean how much data it contains.

Interestingly, that same code already actually checks the length
against the right thing (nr_cpu_ids) elsewhere, but does it kind of
stupidly - first testing that the result fits in the bytes, then
checks that the thing is long-word aligned.

The immediate fix for your issue is likely the attached patch, but I'm
not particularly happy with it. I'd need to at the very least also fix
the same issue in the compat code, but there might be other cases of
this too, where people use the "allocation size" as the "valid bits
size".

Let me think about it some more, but in the meantime you can test if
this patch does indeed fix things for you.

             Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 822 bytes --]

 kernel/sched/core.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index af017e038b48..fdbe7f3b55f0 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -8413,18 +8413,17 @@ SYSCALL_DEFINE3(sched_getaffinity, pid_t, pid, unsigned int, len,
 		return -EINVAL;
 	if (len & (sizeof(unsigned long)-1))
 		return -EINVAL;
+	len = BITS_TO_LONGS(nr_cpu_ids) * sizeof(unsigned long);
 
 	if (!alloc_cpumask_var(&mask, GFP_KERNEL))
 		return -ENOMEM;
 
 	ret = sched_getaffinity(pid, mask);
 	if (ret == 0) {
-		unsigned int retlen = min(len, cpumask_size());
-
-		if (copy_to_user(user_mask_ptr, mask, retlen))
+		if (copy_to_user(user_mask_ptr, mask, len))
 			ret = -EFAULT;
 		else
-			ret = retlen;
+			ret = len;
 	}
 	free_cpumask_var(mask);
 

Re: [BUG] v6.3-rc2 regresses sched_getaffinity() for arm64

[Linus Torvalds]

On Tue, Mar 14, 2023 at 5:51 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> The immediate fix for your issue is likely the attached patch, but I'm
> not particularly happy with it. I'd need to at the very least also fix
> the same issue in the compat code, but there might be other cases of
> this too, where people use the "allocation size" as the "valid bits
> size".

It does look like all other users of cpumask_size() get it right and
treat it as an allocation size (and will explicitly clear the cpumask
if they then also use the size-in-bytes later for other things)

So this does look  like purely a sched_getaffinity() thing (including
the compat handling for same).

And I can see why sched_getaffinity() uses cpumask_size(): we have no
other good helper for this.

It looks like we have never actually done a "what is the size of a
bitmap of X bits" helper function. We have that

        unsigned int len = BITS_TO_LONGS(nbits) * sizeof(unsigned long);

expanded many times by hand, but there is no simple helper for that
rather common expression.

We've got a few places that clearly got tired of not having said
helper, so drivers/md/dm-clone-metadata.c has that "bitmap_size()" as
an inline function, and lib/math/prime_numbers.c has it as a macro.

So I guess I can't blame the getaffinity() code for then using the
allocation size helper, since it was there and it worked until it
didn't. The setaffinity() code actually gets it right, and uses it
basically as a "this is the allocation size" thing, and then fills it
up correctly.

And the reason this hits mainly on arm64 is presumably that on x86-64,
people either use MAXSMP (ugh) or have smaller cpu masks, and you
really need to hit that "64 < NR_CPU <= 256" case to get the
problematic situation.

            Linus

Re: [BUG] v6.3-rc2 regresses sched_getaffinity() for arm64

[Linus Torvalds]

On Tue, Mar 14, 2023 at 6:35 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So this does look  like purely a sched_getaffinity() thing (including
> the compat handling for same).
>
> And I can see why sched_getaffinity() uses cpumask_size(): we have no
> other good helper for this.

I decided that the cleanest fix is to just keep the cpumask_size() use
as-is, and just use zalloc_cpumask_var() to make sure the cpumask is
fully initialized.

Yes, we could play games with the exact size, but there just isn't any
excuse for it. Either it's a small on-stack allocation that gets
copied to user space (in which case we really are better off just
initializing it instead of doing anything clever), or it's an explicit
allocation due to the x86-64 MAXSMP case (in which case zeroing the
allocation is the least of our problems).

And zeroing the cpumask was what other somewhat similar cases seemed
to be doing, so it's consistent.

I've pushed out my fix. It looks ObviouslyCorrect(tm), but it would be
good to get verification that it does indeed fix things for you.

Because sometimes things look a bit more obvious than they actually are ;)

              Linus

Re: [BUG] v6.3-rc2 regresses sched_getaffinity() for arm64

[Ryan Roberts]

On 15/03/2023 02:48, Linus Torvalds wrote:
> On Tue, Mar 14, 2023 at 6:35 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> So this does look  like purely a sched_getaffinity() thing (including
>> the compat handling for same).
>>
>> And I can see why sched_getaffinity() uses cpumask_size(): we have no
>> other good helper for this.
> 
> I decided that the cleanest fix is to just keep the cpumask_size() use
> as-is, and just use zalloc_cpumask_var() to make sure the cpumask is
> fully initialized.
> 
> Yes, we could play games with the exact size, but there just isn't any
> excuse for it. Either it's a small on-stack allocation that gets
> copied to user space (in which case we really are better off just
> initializing it instead of doing anything clever), or it's an explicit
> allocation due to the x86-64 MAXSMP case (in which case zeroing the
> allocation is the least of our problems).
> 
> And zeroing the cpumask was what other somewhat similar cases seemed
> to be doing, so it's consistent.

Thanks for the fast response and clear explanation! FWIW, the fix you committed
looks sensible to me.

> 
> I've pushed out my fix. It looks ObviouslyCorrect(tm), but it would be
> good to get verification that it does indeed fix things for you.

I tested at 6015b1aca1a233379625385feb01dd014aca60b5 and all looks good now, so:

Tested-by: Ryan Roberts <ryan.roberts@arm.com>

> 
> Because sometimes things look a bit more obvious than they actually are ;)
> 
>               Linus