linux-kernel - Re: [PATCH v2 05/18] x86/sgx: Track epc pages on reclaimable or unreclaimable lists

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: <1dccd2ec-cad8-b9d2-d66b-aebad21cdb44@intel.com>
Date:   Fri, 2 Dec 2022 14:13:19 -0800
From:   Dave Hansen <dave.hansen@...el.com>
To:     Kristen Carlson Accardi <kristen@...ux.intel.com>,
        jarkko@...nel.org, dave.hansen@...ux.intel.com, tj@...nel.org,
        linux-kernel@...r.kernel.org, linux-sgx@...r.kernel.org,
        cgroups@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>
Cc:     zhiquan1.li@...el.com, Sean Christopherson <seanjc@...gle.com>
Subject: Re: [PATCH v2 05/18] x86/sgx: Track epc pages on reclaimable or
 unreclaimable lists

On 12/2/22 10:36, Kristen Carlson Accardi wrote:
> Replace functions sgx_mark_page_reclaimable() and
> sgx_unmark_page_reclaimable() with sgx_record_epc_page() and
> sgx_drop_epc_page(). sgx_record_epc_page() wil add the epc_page
> to the correct "reclaimable" or "unreclaimable" list in the
> sgx_epc_lru_lists struct. sgx_drop_epc_page() will delete the page
> from the LRU list. Tracking pages that are not tracked by
> the reclaimer in the sgx_epc_lru_lists "unreclaimable" list allows
> an OOM event to cause all the pages in use by an enclave to be freed,
> regardless of whether they were reclaimable pages or not.

This might be more a comment about Sean's stuff, but could you please
start using paragraphs in these changelogs?

Also, on the content, I really prefer that patches start off talking in
English as much as possible and not just talk about the code.

	Right now, SGX has a single LRU list.  The code is transitioning
	over to use multiple LRU lists.

I'd also prefer that _this_ patch do the:

> -	sgx_mark_page_reclaimable(entry->epc_page);
> +	sgx_record_epc_page(entry->epc_page, SGX_EPC_PAGE_RECLAIMER_TRACKED);

bits and then *another* patch do the unreclaimable side.  This patch
could be a straight replacement which is easy to audit.  The
unreclaimable one needs more thought.

I also think this ends up looking a bit weird:

> -	sgx_epc_push_reclaimable(&sgx_global_lru, page);
> +	WARN_ON(page->flags & SGX_EPC_PAGE_RECLAIMER_TRACKED);
> +	page->flags |= flags;
> +	if (flags & SGX_EPC_PAGE_RECLAIMER_TRACKED)
> +		sgx_epc_push_reclaimable(&sgx_global_lru, page);
> +	else
> +		sgx_epc_push_unreclaimable(&sgx_global_lru, page);
>  	spin_unlock(&sgx_global_lru.lock);
>  }

I think that would be better with a single "push" helper and then let
the callers specify the list:

	if (flags & SGX_EPC_PAGE_RECLAIMER_TRACKED)
		sgx_lru_push(&sgx_global_lru.reclaimable, page);
	else
		sgx_lru_push(&sgx_global_lru.unreclaimable, page);