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

[Haitao Huang]

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