linux-kernel - RE: [PATCH v8 04/15] x86/sgx: Implement basic EPC misc cgroup functionality

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: <BL1PR11MB59784620554CDAD5E2C2BF45F77D2@BL1PR11MB5978.namprd11.prod.outlook.com>
Date: Tue, 30 Jan 2024 15:22:14 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: Haitao Huang <haitao.huang@...ux.intel.com>, "jarkko@...nel.org"
	<jarkko@...nel.org>, "dave.hansen@...ux.intel.com"
	<dave.hansen@...ux.intel.com>, "tj@...nel.org" <tj@...nel.org>,
	"mkoutny@...e.com" <mkoutny@...e.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-sgx@...r.kernel.org"
	<linux-sgx@...r.kernel.org>, "x86@...nel.org" <x86@...nel.org>,
	"cgroups@...r.kernel.org" <cgroups@...r.kernel.org>, "tglx@...utronix.de"
	<tglx@...utronix.de>, "mingo@...hat.com" <mingo@...hat.com>, "bp@...en8.de"
	<bp@...en8.de>, "hpa@...or.com" <hpa@...or.com>, "Mehta, Sohil"
	<sohil.mehta@...el.com>
CC: "Li, Zhiquan1" <zhiquan1.li@...el.com>, "kristen@...ux.intel.com"
	<kristen@...ux.intel.com>, "seanjc@...gle.com" <seanjc@...gle.com>, "Zhang,
 Bo" <zhanb@...rosoft.com>, "anakrish@...rosoft.com" <anakrish@...rosoft.com>,
	"mikko.ylinen@...ux.intel.com" <mikko.ylinen@...ux.intel.com>,
	"yangjie@...rosoft.com" <yangjie@...rosoft.com>, "chrisyan@...rosoft.com"
	<chrisyan@...rosoft.com>
Subject: RE: [PATCH v8 04/15] x86/sgx: Implement basic EPC misc cgroup
 functionality

>  struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim)  {
> +	struct sgx_epc_cgroup *epc_cg;
>  	struct sgx_epc_page *page;
> +	int ret;
> +
> +	epc_cg = sgx_get_current_epc_cg();
> +	ret = sgx_epc_cgroup_try_charge(epc_cg);
> +	if (ret) {
> +		sgx_put_epc_cg(epc_cg);
> +		return ERR_PTR(ret);
> +	}
> 
>  	for ( ; ; ) {
>  		page = __sgx_alloc_epc_page();
> @@ -567,8 +578,10 @@ struct sgx_epc_page *sgx_alloc_epc_page(void
> *owner, bool reclaim)
>  			break;
>  		}
> 
> -		if (list_empty(&sgx_active_page_list))
> -			return ERR_PTR(-ENOMEM);
> +		if (list_empty(&sgx_active_page_list)) {
> +			page = ERR_PTR(-ENOMEM);
> +			break;
> +		}

(Sorry for replying from Outlook because I am in travel for Chinese New Year.)

Perhaps I am missing something but I don't understand this change.

An empty sgx_active_page_list means you cannot reclaim any page from it, so why need to break?

> 
>  		if (!reclaim) {
>  			page = ERR_PTR(-EBUSY);
> @@ -580,10 +593,25 @@ struct sgx_epc_page *sgx_alloc_epc_page(void
> *owner, bool reclaim)
>  			break;
>  		}
> 
> +		/*
> +		 * Need to do a global reclamation if cgroup was not full but
> free
> +		 * physical pages run out, causing __sgx_alloc_epc_page() to
> fail.
> +		 */
>  		sgx_reclaim_pages();
>  		cond_resched();
>  	}

And why adding this comment, especially in this patch?

I don't see it brings additional clarity because there's only global reclaim now, no matter whether cgroup is enabled or not.