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
