Re: [PATCH] RISC-V: KVM: Account VM-scoped allocations to the VM cgroup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The `kvpmu->sdata` allocation uses `GFP_ATOMIC | __GFP_ACCOUNT` in a context that can safely sleep, draining atomic reserves and causing spurious memcg allocation failures.

Pre-existing issues:
- [High] Unconditional memory allocation for the PMU snapshot area leads to a memory leak if the guest repeatedly configures the snapshot.
--

--- Patch [1]: [PATCH] RISC-V: KVM: Account VM-scoped allocations to the VM cgroup ---
commit 7f2fdf55e4a9227b8c7e4853c498204f0c3f9d48
Author: Yuhang.chen <yhchen312@gmail.com>

RISC-V: KVM: Account VM-scoped allocations to the VM cgroup

Charge the per-VM and per-vCPU allocations (stage-2 PGD, APLIC state,
IMSIC context, vector context, FWFT config, PMU snapshot) to the
allocating process's memory cgroup via GFP_KERNEL_ACCOUNT / __GFP_ACCOUNT.

> diff --git a/arch/riscv/kvm/vcpu_pmu.c b/arch/riscv/kvm/vcpu_pmu.c
> index bb46dcbfb24da..e5018784779d5 100644
> --- a/arch/riscv/kvm/vcpu_pmu.c
> +++ b/arch/riscv/kvm/vcpu_pmu.c
> @@ -452,7 +452,7 @@ int kvm_riscv_vcpu_pmu_snapshot_set_shmem(struct kvm_vcpu *vcpu, unsigned long s
>  		}
>  	}
>  
> -	kvpmu->sdata = kzalloc(snapshot_area_size, GFP_ATOMIC);
> +	kvpmu->sdata = kzalloc(snapshot_area_size, GFP_ATOMIC | __GFP_ACCOUNT);

[Severity: High]
This isn't a bug introduced by this patch, but does this leak memory if the
guest repeatedly configures the snapshot?

If a guest makes multiple SBI_EXT_PMU_SNAPSHOT_SET_SHMEM calls, it looks
like this unconditionally allocates and overwrites kvpmu->sdata without
freeing the previous allocation.

[Severity: Medium]
Is it necessary to use GFP_ATOMIC here?

Since the immediate next call is kvm_vcpu_write_guest(), which can sleep:

	/* No need to check writable slot explicitly as kvm_vcpu_write_guest does it internally */
	if (kvm_vcpu_write_guest(vcpu, saddr, kvpmu->sdata, snapshot_area_size)) {
		kfree(kvpmu->sdata);
		kvpmu->sdata = NULL;
		sbiret = SBI_ERR_INVALID_ADDRESS;
		goto out;
	}

it seems this allocation could safely use GFP_KERNEL_ACCOUNT instead.

Combining __GFP_ACCOUNT with GFP_ATOMIC prevents the allocation from sleeping
to perform direct reclaim, which might cause spurious memcg allocation
failures when the cgroup is under pressure.

>  	if (!kvpmu->sdata) {
>  		sbiret = SBI_ERR_FAILURE;
>  		goto out;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716064112.2387938-1-yhchen312@gmail.com?part=1