Re: [PATCH v12 05/14] x86/sgx: Implement basic EPC misc cgroup functionality

From: Haitao Huang
Date: Fri Apr 19 2024 - 14:15:28 EST


On Thu, 18 Apr 2024 18:29:53 -0500, Huang, Kai <kai.huang@xxxxxxxxx> wrote:



--- /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

My test confirms this is does not cause NULL cgroup for the tasks.
It actually works the same way as commandline disable except for that this only disables misc in subtree and does not show any misc.* files or allow creating such files in the subtree.

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.


I think it's just being defensive as this function is public API called by other parts of kernel. Documentation of task_get_css() says it always returns a valid css. This function is used by get_current_misc_cg() to get the css refernce.


/**
* task_get_css - find and get the css for (task, subsys)
* @task: the target task
* @subsys_id: the target subsystem ID
*
* Find the css for the (@task, @subsys_id) combination, increment a
* reference on and return it. This function is guaranteed to return a
* valid css. The returned css may already have been offlined.
*/
static inline struct cgroup_subsys_state *
task_get_css(struct task_struct *task, int subsys_id)


If you look at the code of this function, you will see it does not check NULL either for task_css().

So I think we are pretty sure here it's confirmed by this documentation and testing.
Thanks
Haitao