linux-kernel - Re: [PATCH v25 07/21] x86/sgx: Enumerate and track EPC sections

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:   Thu, 6 Feb 2020 17:34:06 +0200
From:   Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
To:     Sean Christopherson <sean.j.christopherson@...el.com>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org,
        linux-sgx@...r.kernel.org, akpm@...ux-foundation.org,
        dave.hansen@...el.com, nhorman@...hat.com, npmccallum@...hat.com,
        haitao.huang@...el.com, andriy.shevchenko@...ux.intel.com,
        tglx@...utronix.de, kai.svahn@...el.com, bp@...en8.de,
        josh@...htriplett.org, luto@...nel.org, kai.huang@...el.com,
        rientjes@...gle.com, cedric.xing@...el.com, puiterwijk@...hat.com,
        Serge Ayoun <serge.ayoun@...el.com>
Subject: Re: [PATCH v25 07/21] x86/sgx: Enumerate and track EPC sections

On Thu, Feb 06, 2020 at 01:11:47AM +0200, Jarkko Sakkinen wrote:
> On Wed, Feb 05, 2020 at 11:57:00AM -0800, Sean Christopherson wrote:
> > > +		ret = __eremove(sgx_epc_addr(page));
> > > +		if (!WARN_ON_ONCE(ret)) {
> > 
> > Sadly, this WARN can fire after kexec() on systems with multiple EPC
> > sections if the SECS has child pages in another section.
> 
> What causes this?

Ah obviously this can happen given that the final loop is done per
section before other sections are processed.

Lets fix the code first rather than change the approach based on code
that has an underlying regression. Performance can be fine tuned even
after upstreaming. Especially if the performance increases complexity it
is better to work that after there is a mainline code base.

Given that the loop is done in separate thread anyway, I'm not sure how
bad performance issue there is anyway. Performance based changes should
be always done based on a workloads and statistics.

I think you'd fix this issue by first changing the functions as:

static void sgx_sanitize_section(struct sgx_epc_section *section)
{
	struct sgx_epc_page *page, *tmp;
	LIST_HEAD(secs_list);
	int ret;

	while (!list_empty(&section->unsanitized_page_list)) {
		if (kthread_should_stop())
			return;

		spin_lock(&section->lock);

		page = list_first_entry(&section->unsanitized_page_list,
					struct sgx_epc_page, list);

		ret = __eremove(sgx_epc_addr(page));
		if (!ret)
			list_move(&page->list, &section->page_list);
		else
			list_move_tail(&page->list, &secs_list);

		spin_unlock(&section->lock);

		cond_resched();
	}

	list_move_tail(&secs_list, &section->unsanitized_list);
}

Then in ksgxswapd() you'd

for (i = 0; i < sgx_nr_epc_sections; i++)
	sgx_sanitize_section(&sgx_epc_sections[i]);

/* 2nd round for SECS */
for (i = 0; i < sgx_nr_epc_sections; i++)
	sgx_sanitize_section(&sgx_epc_sections[i]);

Finally you'd:

for (i = 0; i < sgx_nr_epc_sections; i++)
	WARN_ONCE(!list_empty(&sgx_epc_sections[i]->unsanitized_list));

/Jarkko