[PATCH] x86/smpboot: Add map vars allocation check in smp_prepare_cpus_common

[PATCH] x86/smpboot: Add map vars allocation check in smp_prepare_cpus_common

[Nikita Kiryushin]

As of now, zalloc_cpumask_var for various maps in smp_prepare_cpus_common
is not checked.

If allocation fails, it will not be known, unless the not-allocated map
will be accessed. The situation seems not very realistic now, but could
get more relevant in the future, as number of cores (and amount of
allocated resources) grows.

Add a cumulative status for all zalloc_cpumask_var() calls in
smp_prepare_cpus_common() and error message in case the status signals
that any of the map var allocations failed (per cpu).

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Nikita Kiryushin <kiryushin@ancud.ru>
---
 arch/x86/kernel/smpboot.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 76bb65045c64..3b24c2e1fa3b 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1042,11 +1042,16 @@ void __init smp_prepare_cpus_common(void)
 	}
 
 	for_each_possible_cpu(i) {
-		zalloc_cpumask_var(&per_cpu(cpu_sibling_map, i), GFP_KERNEL);
-		zalloc_cpumask_var(&per_cpu(cpu_core_map, i), GFP_KERNEL);
-		zalloc_cpumask_var(&per_cpu(cpu_die_map, i), GFP_KERNEL);
-		zalloc_cpumask_var(&per_cpu(cpu_llc_shared_map, i), GFP_KERNEL);
-		zalloc_cpumask_var(&per_cpu(cpu_l2c_shared_map, i), GFP_KERNEL);
+		bool ret = true;
+
+		ret &= zalloc_cpumask_var(&per_cpu(cpu_sibling_map, i), GFP_KERNEL);
+		ret &= zalloc_cpumask_var(&per_cpu(cpu_core_map, i), GFP_KERNEL);
+		ret &= zalloc_cpumask_var(&per_cpu(cpu_die_map, i), GFP_KERNEL);
+		ret &= zalloc_cpumask_var(&per_cpu(cpu_llc_shared_map, i), GFP_KERNEL);
+		ret &= zalloc_cpumask_var(&per_cpu(cpu_l2c_shared_map, i), GFP_KERNEL);
+
+		if (!ret)
+			pr_err("Failed to allocate map for CPU%u\n", i);
 	}
 
 	set_cpu_sibling_map(0);
-- 
2.34.1

Re: [PATCH] x86/smpboot: Add map vars allocation check in smp_prepare_cpus_common

[Ingo Molnar]

* Nikita Kiryushin <kiryushin@ancud.ru> wrote:

> As of now, zalloc_cpumask_var for various maps in smp_prepare_cpus_common
> is not checked.
> 
> If allocation fails, it will not be known, unless the not-allocated map
> will be accessed. The situation seems not very realistic now, but could
> get more relevant in the future, as number of cores (and amount of
> allocated resources) grows.
> 
> Add a cumulative status for all zalloc_cpumask_var() calls in
> smp_prepare_cpus_common() and error message in case the status signals
> that any of the map var allocations failed (per cpu).
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Nikita Kiryushin <kiryushin@ancud.ru>
> ---
>  arch/x86/kernel/smpboot.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 76bb65045c64..3b24c2e1fa3b 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1042,11 +1042,16 @@ void __init smp_prepare_cpus_common(void)
>  	}
>  
>  	for_each_possible_cpu(i) {
> -		zalloc_cpumask_var(&per_cpu(cpu_sibling_map, i), GFP_KERNEL);
> -		zalloc_cpumask_var(&per_cpu(cpu_core_map, i), GFP_KERNEL);
> -		zalloc_cpumask_var(&per_cpu(cpu_die_map, i), GFP_KERNEL);
> -		zalloc_cpumask_var(&per_cpu(cpu_llc_shared_map, i), GFP_KERNEL);
> -		zalloc_cpumask_var(&per_cpu(cpu_l2c_shared_map, i), GFP_KERNEL);
> +		bool ret = true;
> +
> +		ret &= zalloc_cpumask_var(&per_cpu(cpu_sibling_map, i), GFP_KERNEL);
> +		ret &= zalloc_cpumask_var(&per_cpu(cpu_core_map, i), GFP_KERNEL);
> +		ret &= zalloc_cpumask_var(&per_cpu(cpu_die_map, i), GFP_KERNEL);
> +		ret &= zalloc_cpumask_var(&per_cpu(cpu_llc_shared_map, i), GFP_KERNEL);
> +		ret &= zalloc_cpumask_var(&per_cpu(cpu_l2c_shared_map, i), GFP_KERNEL);
> +
> +		if (!ret)
> +			pr_err("Failed to allocate map for CPU%u\n", i);

So:

 - That doesn't really solve anything, nor does it propagate the error 
   further up. Plus memory allocation failures within __init functions for 
   key CPU data structures are invariably fatal. While there might be
   more cores in the future - but there will be even more RAM. This error 
   condition will never be realistic.

 - The canonical arch behavior for __init functions is to return -ENOMEM 
   and not printk anything. But that's not really an option for 
   smp_prepare_cpus_common(), which feeds back into the 
   ::smp_prepare_cpus() callback that doesn't really expect failure either.

My suggestion would be to simply pass in __GFP_NOFAIL to document that 
there's no reasonable allocation failure policy here. That's better than 
needlessly complicating this code.

Also note that this code has changed in the latest x86 tree (tip:master).

Thanks,

	Ingo

Re: [PATCH] x86/smpboot: Add map vars allocation check in smp_prepare_cpus_common

[Nikita Kiryushin]

Thank you for the feedback!
I still have a couple of questions, if you could elaborate further?

>   - That doesn't really solve anything, nor does it propagate the error
>     further up.
This patch indeed is not fixing the problem, but tries to, at least
notify about it. Not sure, if it makes any sense in the context, as
noted that

> Plus memory allocation failures within __init functions for
>     key CPU data structures are invariably fatal.
If allocation errors in this case are indeed fatal, no one will be able to
get the printed warning (so it is indeed pointless).

But I do not understand, if all the possible allocation failures here are fatal?
For cpu 0 it is obvious (as set_cpu_sibling_map(0) right here dereferences
the allocated), but is it as fatal for the other cores?

> While there might be
>     more cores in the future - but there will be even more RAM. This error
>     condition will never be realistic.
In general case it is true. But there are some cases, for which we can not presume, that there will be enough resources: for example, building a new system with large number of cores and test-running it with one small stick of ram, or (more realistically) some quirky hardware/firmware fault. I myself had experience in the past with a buggy BIOS, that made memory resource available for the system usage so small, it led to allocation failures in places it was presumed impossible before.
>   - The canonical arch behavior for __init functions is to return -ENOMEM
>     and not printk anything.
Thank you for this observation! Is there any documentation I should read
to educate myself about conventional rules like that?
I was making my assumptions that printk is an OK solution in this case,
based on surrounding code (like arch_cpuhp_init_parallel_bringup,
disable_smp and native_smp_prepare_cpus, all of which do printk).
I guess it may be specific to this specific part of code that, as you
mentioned, are not meant to fail.
>   But that's not really an option for
>     smp_prepare_cpus_common(), which feeds back into the
>     ::smp_prepare_cpus() callback that doesn't really expect failure either.

> My suggestion would be to simply pass in __GFP_NOFAIL to document that
> there's no reasonable allocation failure policy here.
Thank you, seems like a more elegant solution, than adding new state
flags to the code!
But I have some questions considering __GFP_NOFAIL. It clearly shows, that allocation will not fail/is not expected to fail, does it not try making allocation until it succeeds? Would not it make the system hang in a problematic case? If all the allocations for all the cpus in this block are critical, we would be trading a possible crash for a possible hang (all in early startup), not sure which is better to debug. If some of the allocations are not that critical, with __GFP_NOFAIL the would still hang, is it OK?
> Also note that this code has changed in the latest x86 tree (tip:master).
Thank you for bringing that to my attention! Are there any guides to read
about the preferred workflow for patches for x86 subsystem? I was developing
the code on top of mainline kernel, not the x86 tree, and would like to know
if I should have done it differently for the future.

Re: [PATCH] x86/smpboot: Add map vars allocation check in smp_prepare_cpus_common

[Ingo Molnar]

* Nikita Kiryushin <kiryushin@ancud.ru> wrote:

> But I have some questions considering __GFP_NOFAIL. It clearly shows, 
> that allocation will not fail/is not expected to fail, does it not try 
> making allocation until it succeeds? Would not it make the system hang in 
> a problematic case?

Yes, which is better than undefined behavior. This case should only happen 
on hopelessly buggy kernels or hopelessly buggy hardware - which will very 
likely crash way sooner before they get into this part of the init 
sequence.

This whole problem is a non-issue.

Thanks,

	Ingo