linux-kernel - Re: [PATCH] KVM: arm: memcg awareness

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: <87mtv2i1s3.wl-maz@kernel.org>
Date:   Wed, 17 Mar 2021 09:10:20 +0000
From:   Marc Zyngier <maz@...nel.org>
To:     Wanpeng Li <kernellwp@...il.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [PATCH] KVM: arm: memcg awareness

On Wed, 17 Mar 2021 05:46:24 +0000,
Wanpeng Li <kernellwp@...il.com> wrote:
> 
> From: Wanpeng Li <wanpengli@...cent.com>
> 
> KVM allocations in the arm kvm code which are tied to the life 
> of the VM process should be charged to the VM process's cgroup.
> This will help the memcg controler to do the right decisions.
> 
> Signed-off-by: Wanpeng Li <wanpengli@...cent.com>
> ---
>  arch/arm64/kvm/arm.c               |  5 +++--
>  arch/arm64/kvm/hyp/pgtable.c       |  4 ++--
>  arch/arm64/kvm/mmu.c               |  4 ++--
>  arch/arm64/kvm/pmu-emul.c          |  2 +-
>  arch/arm64/kvm/reset.c             |  2 +-
>  arch/arm64/kvm/vgic/vgic-debug.c   |  2 +-
>  arch/arm64/kvm/vgic/vgic-init.c    |  2 +-
>  arch/arm64/kvm/vgic/vgic-irqfd.c   |  2 +-
>  arch/arm64/kvm/vgic/vgic-its.c     | 14 +++++++-------
>  arch/arm64/kvm/vgic/vgic-mmio-v3.c |  2 +-
>  arch/arm64/kvm/vgic/vgic-v4.c      |  2 +-
>  11 files changed, 21 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 7f06ba7..8040874 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -278,9 +278,10 @@ long kvm_arch_dev_ioctl(struct file *filp,
>  struct kvm *kvm_arch_alloc_vm(void)
>  {
>  	if (!has_vhe())
> -		return kzalloc(sizeof(struct kvm), GFP_KERNEL);
> +		return kzalloc(sizeof(struct kvm), GFP_KERNEL_ACCOUNT);
>  
> -	return vzalloc(sizeof(struct kvm));
> +	return __vmalloc(sizeof(struct kvm),
> +			GFP_KERNEL_ACCOUNT | __GFP_ZERO);
>  }
>  
>  void kvm_arch_free_vm(struct kvm *kvm)
> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> index 926fc07..a0845d3 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c
> @@ -366,7 +366,7 @@ static int hyp_map_walker(u64 addr, u64 end, u32 level, kvm_pte_t *ptep,
>  	if (WARN_ON(level == KVM_PGTABLE_MAX_LEVELS - 1))
>  		return -EINVAL;
>  
> -	childp = (kvm_pte_t *)get_zeroed_page(GFP_KERNEL);
> +	childp = (kvm_pte_t *)get_zeroed_page(GFP_KERNEL_ACCOUNT);

No, this is wrong.

You cannot account the hypervisor page tables to the guest because we
don't ever unmap them, and that we can't distinguish two data
structures from two different VMs occupying the same page.

>  	if (!childp)
>  		return -ENOMEM;
>  
> @@ -401,7 +401,7 @@ int kvm_pgtable_hyp_init(struct kvm_pgtable *pgt, u32 va_bits)
>  {
>  	u64 levels = ARM64_HW_PGTABLE_LEVELS(va_bits);
>  
> -	pgt->pgd = (kvm_pte_t *)get_zeroed_page(GFP_KERNEL);
> +	pgt->pgd = (kvm_pte_t *)get_zeroed_page(GFP_KERNEL_ACCOUNT);

There is no VM in this context. There isn't even any userspace
whatsoever in the system when this is called.

[...]

> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
> index 66508b0..a80cc37 100644
> --- a/arch/arm64/kvm/vgic/vgic-v4.c
> +++ b/arch/arm64/kvm/vgic/vgic-v4.c
> @@ -227,7 +227,7 @@ int vgic_v4_init(struct kvm *kvm)
>  	nr_vcpus = atomic_read(&kvm->online_vcpus);
>  
>  	dist->its_vm.vpes = kcalloc(nr_vcpus, sizeof(*dist->its_vm.vpes),
> -				    GFP_KERNEL);
> +				    GFP_KERNEL_ACCOUNT);

And now for the elephant in the room: what you do for the GICv4 VPTs
that are allocated for each vPE?

	M.

-- 
Without deviation from the norm, progress is not possible.