Re: [PATCH v12 09/14] x86/sgx: Implement async reclamation for cgroup

Huang, Kai
2024-04-22T22:16:46Z

On Mon, 2024-04-22 at 11:17 -0500, Haitao Huang wrote:
> On Sun, 21 Apr 2024 19:22:27 -0500, Huang, Kai <kai.huang@intel.com> wrote:
>
> > On Fri, 2024-04-19 at 20:14 -0500, Haitao Huang wrote:
> > > > > I think we can add support for "sgx_cgroup=disabled" in future if
> > > indeed
> > > > > needed. But just for init failure, no?
> > > > >
> > > >
> > > > It's not about the commandline, which we can add in the future when
> > > > needed. It's about we need to have a way to handle SGX cgroup being
> > > > disabled at boot time nicely, because we already have a case where we
> > > > need
> > > > to do so.
> > > >
> > > > Your approach looks half-way to me, and is not future extendible. If
> > > we
> > > > choose to do it, do it right -- that is, we need a way to disable it
> > > > completely in both kernel and userspace so that userspace won't be
> > > able> to
> > > > see it.
> > >
> > > That would need more changes in misc cgroup implementation to support
> > > sgx-disable. Right now misc does not have separate files for different
> > > resource types. So we can only block echo "sgx_epc..." to those
> > > interfacefiles, can't really make files not visible.
> >
> > "won't be able to see" I mean "only for SGX EPC resource", but not the
> > control files for the entire MISC cgroup.
> >
> > I replied at the beginning of the previous reply:
> >
> > "
> > Given SGX EPC is just one type of MISC cgroup resources, we cannot just
> > disable MISC cgroup as a whole.
> > "
> >
> Sorry I missed this point. below.
>
> > You just need to set the SGX EPC "capacity" to 0 to disable SGX EPC. See
> > the comment of @misc_res_capacity:
> >
> > * Miscellaneous resources capacity for the entire machine. 0 capacity
> > * means resource is not initialized or not present in the host.
> >
>
> IIUC I don't think the situation we have is either of those cases. For our
> case, resource is inited and present on the host but we have allocation
> error for sgx cgroup infra.

You have calculated the "capacity", but later you failed something and
then reset the "capacity" to 0, i.e., cleanup. What's wrong with that?

>
> > And "blocking echo sgx_epc ... to those control files" is already
> > sufficient for the purpose of not exposing SGX EPC to userspace, correct?
> >
> > E.g., if SGX cgroup is enabled, you can see below when you read "max":
> >
> > # cat /sys/fs/cgroup/my_group/misc.max
> > # <resource1> <max1>
> > sgx_epc ...
> > ...
> >
> > Otherwise you won't be able to see "sgx_epc":
> >
> > # cat /sys/fs/cgroup/my_group/misc.max
> > # <resource1> <max1>
> > ...
> >
> > And when you try to write the "max" for "sgx_epc", you will hit error:
> >
> > # echo "sgx_epc 100" > /sys/fs/cgroup/my_group/misc.max
> > # ... echo: write error: Invalid argument
> >
> > The above applies to all the control files. To me this is pretty much
> > means "SGX EPC is disabled" or "not supported" for userspace.
> >
> You are right, capacity == 0 does block echoing max and users see an error
> if they do that. But 1) doubt you literately wanted "SGX EPC is disabled"
> and make it unsupported in this case,
>

I don't understand. Something failed during SGX cgroup initialization,
you _literally_ cannot continue to support it.

> 2) even if we accept this is "sgx
> cgroup disabled" I don't see how it is much better user experience than
> current solution or really helps user better.

In your way, the userspace is still able to see "sgx_epc" in control files
and is able to update them. So from userspace's perspective SGX cgroup is
enabled, but obviously updating to "max" doesn't have any impact. This
will confuse userspace.

>
> Also to implement this approach, as you mentioned, we need workaround the
> fact that misc_try_charge() fails when capacity set to zero, and adding
> code to return root always?
>

Why this is a problem?

> So it seems like more workaround code to just
> make it work for a failing case no one really care much and end result is
> not really much better IMHO.

It's not workaround, it's the right thing to do.

The result is userspace will see it being disabled when kernel disables
it.