linux-kernel - Re: [PATCH] mm/mm_init.c: remove spinlock in early_pfn_to

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2023061431-litigate-upchuck-7ed1@gregkh>
Date:   Wed, 14 Jun 2023 13:09:45 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Yajun Deng <yajun.deng@...ux.dev>
Cc:     rafael@...nel.org, rppt@...nel.org, akpm@...ux-foundation.org,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH] mm/mm_init.c: remove spinlock in early_pfn_to_nid()

On Wed, Jun 14, 2023 at 07:03:24PM +0800, Yajun Deng wrote:
> When the system boots, only one cpu is enabled before smp_init().
> So the spinlock is not needed in most cases, remove it.
> 
> Add spinlock in get_nid_for_pfn() because it is after smp_init().

So this is two different things at once in the same patch?

Or are they the same problem and both need to go in to solve it?

And if a spinlock is not needed at early boot, is it really causing any
problems?

> 
> Signed-off-by: Yajun Deng <yajun.deng@...ux.dev>
> ---
>  drivers/base/node.c | 11 +++++++++--
>  mm/mm_init.c        | 18 +++---------------
>  2 files changed, 12 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/base/node.c b/drivers/base/node.c
> index 9de524e56307..844102570ff2 100644
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -748,8 +748,15 @@ int unregister_cpu_under_node(unsigned int cpu, unsigned int nid)
>  static int __ref get_nid_for_pfn(unsigned long pfn)
>  {
>  #ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT
> -	if (system_state < SYSTEM_RUNNING)
> -		return early_pfn_to_nid(pfn);
> +	static DEFINE_SPINLOCK(early_pfn_lock);
> +	int nid;
> +
> +	if (system_state < SYSTEM_RUNNING) {
> +		spin_lock(&early_pfn_lock);
> +		nid = early_pfn_to_nid(pfn);
> +		spin_unlock(&early_pfn_lock);

Adding an external lock for when you call a function is VERY dangerous
as you did not document this anywhere, and there's no way to enforce it
properly at all.

Does your change actually result in any boot time changes?  How was this
tested?

thanks,

greg k-h