Re: [PATCH] KVM: TDX: Charge misc cgroup before allocating HKID

On 8/25/2026 5:10 AM, Edgecombe, Rick P wrote:
> On Fri, 2026-08-21 at 17:39 +0800, Binbin Wu wrote:
>> Add a tdx_hkid_alloc() helper that charges the misc cgroup before
>> allocating an HKID, and unwind the charge if HKID allocation fails.
>>
>> __tdx_td_init() currently allocates an HKID before charging the misc
>> cgroup. If the charge fails, the error path calls tdx_hkid_free(), which
>> uncharges a resource that was never successfully charged. This can make
>> the misc-cgroup usage negative.
>>
>> Charge the cgroup before allocating the HKID.  Wrapping both steps in
>> tdx_hkid_alloc() makes it the exact counterpart of tdx_hkid_free(), i.e.
>> keeps resource allocation and release symmetric, and lets __tdx_td_init()
>> simply bail on failure instead of open coding the unwind.
>>
>> Reported-by: [email protected]
>> Closes: https://lore.kernel.org/all/[email protected]
>> Closes: https://lore.kernel.org/all/[email protected]
>> Fixes: 7c035bea9407 ("KVM: TDX: Register TDX host key IDs to cgroup misc controller")
>> Signed-off-by: Binbin Wu <[email protected]>
> 
> As a straightforward bug fix:
> Reviewed-by: Rick Edgecombe <[email protected]>
> 
> But it seems a bit awkward how the keyid allocator is carefully hidden away in
> arch/x86 but the KVM caller does the cgroup maintenance. Hmm, I'd wonder if we
> could move the struct misc_cg pointer to struct tdx_td or otherwise pass it in,
> and make this stuff managed by arch/x86.
> 
> I think the only reason it is KVM managed is that an old cgroup patch got
> applied on top of the base series. The old design from the era of that patch had
> the keyid range exported, and KVM used it to manage the keyid allocation. Then
> when the keyid range got hidden, it resulted in the alloc/free functions getting
> exported. So I wonder if the new tdx_hkid_alloc() should live in arch/x86.
> Otherwise we are doing the thing where KVM just wraps arch/x86 exports to do
> what it needed to do in the first place.

Yes, make sense.

> 
> But not needed for this patch in any case.

It could be a separate cleanup patch.