Re: [PATCH v2 03/29] KVM: VMX: Generalize VPID allocation to be vendor-neutral

[Yosry Ahmed <yosry@kernel.org>]

On Thu, Oct 08, 2026 at 12:30:27AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] kvm: x86: memory leak of tlb_tags.bitmap on TDX setup failure
> --
> 
> commit e4dabbc78aa00d0293ef8965888f054d5ea676ef
> Author: Yosry Ahmed <yosry@kernel.org>
> 
> KVM: VMX: Generalize VPID allocation to be vendor-neutral
> 
> This commit generalizes the VMX VPID allocation code to a common TLB
> tags allocator and allocates the bitmap dynamically. It also bounds the
> number of allowed tags to VMX's hardware cap and introduces wrapper
> functions for initialization and destruction.
> 
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index aadd5bdc2c97a..97d49247d9d82 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> [ ... ]
> > @@ -8178,6 +8178,84 @@ void kvm_mmu_pre_destroy_vm(struct kvm *kvm)
> >  		vhost_task_stop(kvm->arch.nx_huge_page_recovery_thread);
> >  }
> >  
> > +static struct {
> > +	spinlock_t	lock;
> > +	unsigned long	*bitmap;
> > +	unsigned int	nr;
> > +} tlb_tags;
> > +
> > +int kvm_init_tlb_tags(unsigned int nr)
> > +{
> > +	/*
> > +	 * Limit the number of TLB tags to VMX's hardcoded maximum of 0x10000
> > +	 * to avoid wasting memory for the bitmap in the unlikely scenario the
> > +	 * CPU supports an inordinate number of ASIDs (on AMD).  If userspace
> > +	 * wants to concurrently run tens of thousands of vCPUs, they'll likely
> > +	 * need a solution that works for both Intel and AMD.
> > +	 */
> > +	const unsigned int MAX_NR_TLB_TAGS = VMX_NR_VPIDS;
> > +
> > +	if (!nr)
> > +		return 0;
> > +
> > +	if (nr > MAX_NR_TLB_TAGS) {
> > +		pr_warn_once("Number of TLB tags capped (%u instead of %u)\n",
> > +			     MAX_NR_TLB_TAGS, nr);
> > +		nr = MAX_NR_TLB_TAGS;
> > +	}
> > +
> > +	tlb_tags.bitmap = bitmap_zalloc(nr, GFP_KERNEL);
> > +	if (!tlb_tags.bitmap)
> > +		return -ENOMEM;
> 
> [Severity: Medium]
> Does this code leak the tlb_tags.bitmap on TDX setup failure?
> 
> The patch converts the static VPID bitmap to a dynamically allocated
> tlb_tags.bitmap in kvm_init_tlb_tags(), which is called by
> vmx_hardware_setup().
> 
> In arch/x86/kvm/vmx/main.c:vt_hardware_setup(), it sequentially calls
> vmx_hardware_setup() and then tdx_hardware_setup():
> 
> arch/x86/kvm/vmx/main.c:vt_hardware_setup() {
> 	ret = vmx_hardware_setup();
> 	if (ret)
> 		return ret;
> 
> 	return enable_tdx ? tdx_hardware_setup() : 0;
> }
> 
> If tdx_hardware_setup() returns an error, vt_hardware_setup() returns the
> error directly without invoking vmx_hardware_unsetup() to undo
> vmx_hardware_setup()'s allocations.
> 
> KVM's generic module initialization explicitly skips calling
> ops->hardware_unsetup() if ops->hardware_setup() fails.
> 
> Does this permanently leak the newly dynamically allocated tlb_tags.bitmap?

Yes, but I think this is a pre-existing issue. I can find at least one
other allocation that would get leaked due to lack of cleanup if
vmx_hardware_setup() succeeds and tdx_hardware_setup() fails:
vmx_bitmap[] allocated by nested_vmx_hardware_setup().

So unless someone feels strongly, I'd rather avoid growing this series
by fixing this as well.

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261008001425.2458927-1-yosry@kernel.org?part=3