linux-kernel - Re: [PATCH v2 1/2] x86/sgx: Do not update sgx_nr_free_pages in sgx_setup_epc

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: <YG3X454GI4U2BZVU@kernel.org>
Date:   Wed, 7 Apr 2021 19:03:47 +0300
From:   Jarkko Sakkinen <jarkko@...nel.org>
To:     Borislav Petkov <bp@...en8.de>
Cc:     linux-sgx@...r.kernel.org,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, x86@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] x86/sgx: Do not update sgx_nr_free_pages in
 sgx_setup_epc_section()

On Wed, Apr 07, 2021 at 05:49:34PM +0200, Borislav Petkov wrote:
> On Tue, Apr 06, 2021 at 02:26:52AM +0300, Jarkko Sakkinen wrote:
> > Now that the sanitization process will make pages available by calling
> > sgx_free_epc_page(), sgx_setup_epc_section() should not touch to
> > sgx_nr_free_pages. This will result sgx_nr_free_pages to contain 2x the
> > number of actual free pages. Simply, remove the statement.
> > 
> > Fixes: 51ab30eb2ad4 ("x86/sgx: Replace section->init_laundry_list with sgx_dirty_page_list")
> > Signed-off-by: Jarkko Sakkinen <jarkko@...nel.org>
> > ---
> >  arch/x86/kernel/cpu/sgx/main.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > index 13a7599ce7d4..7df7048cb1c9 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -657,7 +657,6 @@ static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size,
> >  		list_add_tail(&section->pages[i].list, &sgx_dirty_page_list);
> >  	}
> >  
> > -	sgx_nr_free_pages += nr_pages;
> >  	return true;
> >  }
> >  
> 
> First of all, I don't know how I didn't catch this:
> 
> /* The free page list lock protected variables prepend the lock. */
> static unsigned long sgx_nr_free_pages;
> 
> I need some sort of translator to understand what this comment means. I
> can guess what is trying to tell me.
> 
> Which leads to my question: what is sgx_nr_free_pages supposed to denote?
> 
> Because I understand the callpath
> 
> sgx_page_cache_init
> ...
> for (i = 0; i < ARRAY_SIZE(sgx_epc_sections); i++) {
> 	...
> 	sgx_setup_epc_section
> 	...
> 		sgx_nr_free_pages += nr_pages;
> 
> as adding the number of pages of each new EPC section to the total
> number of the free pages. Unless that variable accounts something else.
> 
> So what does this variable actually mean?

It's used for only to trigger watermark for reclaiming. I.e. causes
ksgxd to trigger. And it gives the number of total free EPC pages in
all NUMA nodes.

/Jarkko