linux-kernel - Re: [PATCH v2 1/2] x86/sgx: Fix deadlock in SGX NUMA node search

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: <D3YF1PO93RI0.2V31UQ4D620TT@kernel.org>
Date: Thu, 05 Sep 2024 17:21:54 +0300
From: "Jarkko Sakkinen" <jarkko@...nel.org>
To: "Aaron Lu" <aaron.lu@...el.com>, "Dave Hansen"
 <dave.hansen@...ux.intel.com>, "Kai Huang" <kai.huang@...el.com>
Cc: "Molina Sabido, Gerardo" <gerardo.molina.sabido@...el.com>, "Zhimin Luo"
 <zhimin.luo@...el.com>, <linux-sgx@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, <x86@...nel.org>
Subject: Re: [PATCH v2 1/2] x86/sgx: Fix deadlock in SGX NUMA node search

On Thu Sep 5, 2024 at 11:08 AM EEST, Aaron Lu wrote:
> When the current node doesn't have an EPC section configured by firmware
> and all other EPC sections are used up, CPU can get stuck inside the
> while loop that looks for an available EPC page from remote nodes
> indefinitely, leading to a soft lockup. Note how nid_of_current will
> never be equal to nid in that while loop because nid_of_current is not
> set in sgx_numa_mask.
>
> Also worth mentioning is that it's perfectly fine for the firmware not
> to setup an EPC section on a node. While setting up an EPC section on
> each node can enhance performance, it is not a requirement for
> functionality.
>
> Rework the loop to start and end on *a* node that has SGX memory. This
> avoids the deadlock looking for the current SGX-lacking node to show up
> in the loop when it never will.
>
> Fixes: 901ddbb9ecf5 ("x86/sgx: Add a basic NUMA allocation scheme to sgx_alloc_epc_page()")
> Reported-by: "Molina Sabido, Gerardo" <gerardo.molina.sabido@...el.com>
> Tested-by: Zhimin Luo <zhimin.luo@...el.com>
> Reviewed-by: Kai Huang <kai.huang@...el.com>
> Acked-by: Dave Hansen <dave.hansen@...ux.intel.com>
> Signed-off-by: Aaron Lu <aaron.lu@...el.com>
> ---
>  arch/x86/kernel/cpu/sgx/main.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 1a000acd933a..694fcf7a5e3a 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -475,24 +475,25 @@ struct sgx_epc_page *__sgx_alloc_epc_page(void)
>  {
>  	struct sgx_epc_page *page;
>  	int nid_of_current = numa_node_id();
> -	int nid = nid_of_current;
> +	int nid_start, nid;
>  
> -	if (node_isset(nid_of_current, sgx_numa_mask)) {
> -		page = __sgx_alloc_epc_page_from_node(nid_of_current);
> -		if (page)
> -			return page;
> -	}
> -
> -	/* Fall back to the non-local NUMA nodes: */
> -	while (true) {
> -		nid = next_node_in(nid, sgx_numa_mask);
> -		if (nid == nid_of_current)
> -			break;
> +	/*
> +	 * Try local node first. If it doesn't have an EPC section,
> +	 * fall back to the non-local NUMA nodes.
> +	 */
> +	if (node_isset(nid_of_current, sgx_numa_mask))
> +		nid_start = nid_of_current;
> +	else
> +		nid_start = next_node_in(nid_of_current, sgx_numa_mask);
>  
> +	nid = nid_start;
> +	do {
>  		page = __sgx_alloc_epc_page_from_node(nid);
>  		if (page)
>  			return page;
> -	}
> +
> +		nid = next_node_in(nid, sgx_numa_mask);
> +	} while (nid != nid_start);
>  
>  	return ERR_PTR(-ENOMEM);
>  }

Looks good to me:

Reviewed-by: Jarkko Sakkinen <jarkko@...nel.org>

BR, Jarkko