linux-kernel - Re: [PATCH v16 05/16] x86/sgx: Implement basic EPC misc cgroup functionality

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <41ac0f4d-064c-4ece-9c23-3618f1a7bdf5@intel.com>
Date: Wed, 28 Aug 2024 12:01:57 +1200
From: "Huang, Kai" <kai.huang@...el.com>
To: Haitao Huang <haitao.huang@...ux.intel.com>, <jarkko@...nel.org>,
	<dave.hansen@...ux.intel.com>, <tj@...nel.org>, <mkoutny@...e.com>,
	<chenridong@...wei.com>, <linux-kernel@...r.kernel.org>,
	<linux-sgx@...r.kernel.org>, <x86@...nel.org>, <cgroups@...r.kernel.org>,
	<tglx@...utronix.de>, <mingo@...hat.com>, <bp@...en8.de>, <hpa@...or.com>,
	<sohil.mehta@...el.com>, <tim.c.chen@...ux.intel.com>
CC: <zhiquan1.li@...el.com>, <kristen@...ux.intel.com>, <seanjc@...gle.com>,
	<zhanb@...rosoft.com>, <anakrish@...rosoft.com>,
	<mikko.ylinen@...ux.intel.com>, <yangjie@...rosoft.com>,
	<chrisyan@...rosoft.com>
Subject: Re: [PATCH v16 05/16] x86/sgx: Implement basic EPC misc cgroup
 functionality



On 28/08/2024 11:11 am, Huang, Kai wrote:
>> +static void sgx_cgroup_misc_init(struct misc_cg *cg, struct 
>> sgx_cgroup *sgx_cg)
>> +{
>> +    cg->res[MISC_CG_RES_SGX_EPC].priv = sgx_cg;
>> +    sgx_cg->cg = cg;
>> +}
>> +
> 
> [...]
> 
>> +int __init sgx_cgroup_init(void)
>> +{
>> +    sgx_cgroup_misc_init(misc_cg_root(), &sgx_cg_root);
>> +
>> +    return 0;
>> +} > +
>> +/**
>> + * Register capacity and ops for SGX cgroup.
>> + * Only called at the end of sgx_init() when SGX is ready to handle 
>> the ops
>> + * callbacks.
>> + */
>> +void __init sgx_cgroup_register(void)
>> +{
>> +    unsigned int nid = first_node(sgx_numa_mask);
>> +    unsigned int first = nid;
>> +    u64 capacity = 0;
>> +
>> +    misc_cg_set_ops(MISC_CG_RES_SGX_EPC, &sgx_cgroup_ops);
>> +
>> +    /* sgx_numa_mask is not empty when this is called */
>> +    do {
>> +        capacity += sgx_numa_nodes[nid].size;
>> +        nid = next_node_in(nid, sgx_numa_mask);
>> +    } while (nid != first);
>> +    misc_cg_set_capacity(MISC_CG_RES_SGX_EPC, capacity);
>> +}
> 
> [...]
> 
>> @@ -930,6 +961,9 @@ static int __init sgx_init(void)
>>       if (ret)
>>           goto err_kthread;
>> +    ret = sgx_cgroup_init();
>> +    if (ret)
>> +        goto err_provision;
>>       /*
>>        * Always try to initialize the native *and* KVM drivers.
>>        * The KVM driver is less picky than the native one and
>> @@ -943,6 +977,8 @@ static int __init sgx_init(void)
>>       if (sgx_vepc_init() && ret)
>>           goto err_provision;
> 
> In sgx_cgroup_init():
> 
>      sgx_cgroup_misc_init(misc_cg_root(), &sgx_cg_root);
> 
> .. also cannot fail.
> 
> I think it should be moved to the sgx_cgroup_register().  Otherwise, if 
> any step after sgx_cgroup_init() fails, there's no unwind for the above 
> operation.
> 
> The consequence is the misc_cg_root()->res[EPC].priv will remain 
> pointing to the SGX root cgroup.
> 
> It shouldn't cause any real issue for now, but it's weird to have that 
> set, and can potentially cause problem in the future.
> 
>> +    sgx_cgroup_register();
>> +
>>       return 0;
>>   err_provision:
> 
> So, I think we should do:
> 
> 1) Rename sgx_cgroup_register() -> sgx_cgroup_init(), and move the
> 
>      sgx_cgroup_misc_init(misc_cg_root(), &sgx_cg_root);
> 
> to it.  All operations in the (new) sgx_cgroup_init() won't fail.
> 
> 2) Remove (existing) sgx_cgroup_init() form this patch, but introduce it 
> in the patch "x86/sgx: Implement async reclamation for cgroup" and 
> rename it to sgx_cgroup_prepare() or something.  It just allocates 
> workqueue inside.  And sgx_cgroup_deinit() -> sgx_cgroup_cleanup().
> 
> Makes sense?
> 
> 

With the above addressed, and the k-doc warning fixed:

Reviewed-by: Kai Huang <kai.huang@...el.com>