Received-SPF: pass (google.com: domain of linux-kernel+bounces-138605-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Sender: Ingo Molnar <mingo.kernel.org@gmail.com>
Date: Wed, 10 Apr 2024 15:30:12 +0200
From: Ingo Molnar <mingo@kernel.org>
To: Nikita Kiryushin <kiryushin@ancud.ru>
Cc: Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
	Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
	"H. Peter Anvin" <hpa@zytor.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Ashok Raj <ashok.raj@intel.com>,
	David Woodhouse <dwmw@amazon.co.uk>, linux-kernel@vger.kernel.org,
	lvc-project@linuxtesting.org
Subject: Re: [PATCH] x86/smpboot: Add map vars allocation check in
 smp_prepare_cpus_common
Message-ID: <ZhaUZFY5MUyy6hWK@gmail.com>
References: <20240409182940.664482-1-kiryushin@ancud.ru>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240409182940.664482-1-kiryushin@ancud.ru>


* 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