linux-kernel - Re: [PATCH v12 05/14] 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: <d401e7a1-44fa-44b6-9c6e-c0abdabd5111@intel.com>
Date: Fri, 19 Apr 2024 11:29:53 +1200
From: "Huang, Kai" <kai.huang@...el.com>
To: Haitao Huang <haitao.huang@...ux.intel.com>, "hpa@...or.com"
	<hpa@...or.com>, "tim.c.chen@...ux.intel.com" <tim.c.chen@...ux.intel.com>,
	"linux-sgx@...r.kernel.org" <linux-sgx@...r.kernel.org>, "x86@...nel.org"
	<x86@...nel.org>, "dave.hansen@...ux.intel.com"
	<dave.hansen@...ux.intel.com>, "jarkko@...nel.org" <jarkko@...nel.org>,
	"cgroups@...r.kernel.org" <cgroups@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"mkoutny@...e.com" <mkoutny@...e.com>, "tglx@...utronix.de"
	<tglx@...utronix.de>, "Mehta, Sohil" <sohil.mehta@...el.com>, "tj@...nel.org"
	<tj@...nel.org>, "mingo@...hat.com" <mingo@...hat.com>, "bp@...en8.de"
	<bp@...en8.de>
CC: "mikko.ylinen@...ux.intel.com" <mikko.ylinen@...ux.intel.com>,
	"seanjc@...gle.com" <seanjc@...gle.com>, "anakrish@...rosoft.com"
	<anakrish@...rosoft.com>, "Zhang, Bo" <zhanb@...rosoft.com>,
	"kristen@...ux.intel.com" <kristen@...ux.intel.com>, "yangjie@...rosoft.com"
	<yangjie@...rosoft.com>, "Li, Zhiquan1" <zhiquan1.li@...el.com>,
	"chrisyan@...rosoft.com" <chrisyan@...rosoft.com>
Subject: Re: [PATCH v12 05/14] x86/sgx: Implement basic EPC misc cgroup
 functionality


> Was requested by Jarkko:
> https://lore.kernel.org/lkml/CYU504RLY7QU.QZY9LWC076NX@suppilovahvero/#t
> 
>> [...]

Ah I missed that.  No problem to me.

>>
>>>
>>> --- /dev/null
>>> +++ b/arch/x86/kernel/cpu/sgx/epc_cgroup.h
>>> @@ -0,0 +1,72 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +#ifndef _SGX_EPC_CGROUP_H_
>>> +#define _SGX_EPC_CGROUP_H_
>>> +
>>> +#include <asm/sgx.h>
>>
>> I don't see why you need <asm/sgx.h> here.  Also, ...
>>
>>> +#include <linux/cgroup.h>
>>> +#include <linux/misc_cgroup.h>
>>> +
>>> +#include "sgx.h"
>>
>> ... "sgx.h" already includes <asm/sgx.h>
>>
>> [...]
>>
> right
> 
>>>
>>> +static inline struct sgx_cgroup *sgx_get_current_cg(void)
>>> +{
>>> +    /* get_current_misc_cg() never returns NULL when Kconfig enabled */
>>> +    return sgx_cgroup_from_misc_cg(get_current_misc_cg());
>>> +}
>>
>> I spent some time looking into this.  And yes if I was reading code
>> correctly the get_current_misc_cg() should never return NULL when Kconfig
>> is on.
>>
>> I typed my analysis below in [*].  And it would be helpful if any cgroup
>> expert can have a second eye on this.
>>
>> [...]
>>
> Thanks for checking this and I did similar and agree with the 
> conclusion. I think this is confirmed also by Michal's description AFAICT:
> "
> The current implementation creates root css object (see cgroup_init(),
> cgroup_ssid_enabled() check is after cgroup_init_subsys()).
> I.e. it will look like all tasks are members of root cgroup wrt given
> controller permanently and controller attribute files won't exist."

After looking I believe we can even disable MISC cgroup at runtime for a 
particular cgroup (haven't actually verified on real machine, though):

  # echo "-misc" > /sys/fs/cgroup/my_group/cgroup.subtree_control

And if you look at the MISC cgroup core code, many functions actually 
handle a NULL css, e.g., misc_cg_try_charge():

	int misc_cg_try_charge(enum misc_res_type type,
				struct misc_cg *cg, u64 amount)
	{
		...

         	if (!(valid_type(type) && cg &&
				READ_ONCE(misc_res_capacity[type])))
                 	return -EINVAL;

		...
	}

That's why I am still a little bit worried about this.  And it's better 
to have cgroup expert(s) to confirm here.

Btw, AMD SEV doesn't need to worry because it doesn't dereference @css 
but just pass it to MISC cgroup core functions like 
misc_cg_try_charge().  But for SGX, we actually dereference it directly.