linux-kernel - Re: [PATCH v4 1/3] x86/sgx: Use sgx_free_epc_page() in sgx_reclaim

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]

Date:   Mon, 15 Mar 2021 08:32:13 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Jarkko Sakkinen <jarkko@...nel.org>, linux-sgx@...r.kernel.org
Cc:     Dave Hansen <dave.hansen@...ux.intel.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/3] x86/sgx: Use sgx_free_epc_page() in
 sgx_reclaim_pages()

On 3/13/21 8:01 AM, Jarkko Sakkinen wrote:
> Replace the ad-hoc code with a sgx_free_epc_page(), in order to make sure
> that all the relevant checks and book keeping is done, while freeing a
> borrowed EPC page, and remove redundant code. EREMOVE inside
> sgx_free_epc_page() does not change the semantics, as EREMOVE to an
> uninitialize pages is a nop.

  ^ uninitialized

I know this is a short patch, but this changelog still falls a bit short
for me.

Why is this patch a part of _this_ series?  What *problem* does it
solve, related to this series?

It would also be nice to remind me why the EREMOVE is redundant.  Why
didn't we need one before?  What put the page in the uninitialized
state?  Is EREMOVE guaranteed to do nothing?  How expensive is it?



> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 8df81a3ed945..65004fb8a91f 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -305,7 +305,6 @@ static void sgx_reclaim_pages(void)
>  {
>  	struct sgx_epc_page *chunk[SGX_NR_TO_SCAN];
>  	struct sgx_backing backing[SGX_NR_TO_SCAN];
> -	struct sgx_epc_section *section;
>  	struct sgx_encl_page *encl_page;
>  	struct sgx_epc_page *epc_page;
>  	pgoff_t page_index;
> @@ -378,11 +377,7 @@ static void sgx_reclaim_pages(void)
>  		kref_put(&encl_page->encl->refcount, sgx_encl_release);
>  		epc_page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
>  
> -		section = &sgx_epc_sections[epc_page->section];
> -		spin_lock(&section->lock);
> -		list_add_tail(&epc_page->list, &section->page_list);
> -		section->free_cnt++;
> -		spin_unlock(&section->lock);
> +		sgx_free_epc_page(epc_page);
>  	}
>  }
>  
>