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

[Huang, Kai]

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