Re: [PATCH v3 1/2] KVM: arm64: ptdump: Check the page tables aren't freed when accessing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] kvm: arm64: ptdump: prolonged write_lock on mmu_lock causes lock contention
--

commit 6e17983aae35d9aa5807fc7910feb63951dae7e2
Author: Wei-Lin Chang <weilin.chang@arm.com>
KVM: arm64: ptdump: Check the page tables aren't freed when accessing

This patch adds checks to ensure that stage-2 page tables are not freed during
access when debugfs ptdump files are opened or read. It uses mmu_lock to
protect the existence of the page tables and switches related handlers to
keep a pointer to the mmu instead.

Link: https://lore.kernel.org/kvmarm/20260623144054.B91D21F000E9@smtp.kernel.org/

> diff --git a/arch/arm64/kvm/ptdump.c b/arch/arm64/kvm/ptdump.c
> index 69899797dbad8..4ead856c181a0 100644
> --- a/arch/arm64/kvm/ptdump.c
> +++ b/arch/arm64/kvm/ptdump.c
[ ... ]
>  static struct kvm_ptdump_guest_state *kvm_ptdump_parser_create(struct kvm_s2_mmu *mmu)
>  {
> +	struct kvm *kvm = kvm_s2_mmu_to_kvm(mmu);
>  	struct kvm_ptdump_guest_state *st;
> -	struct kvm_pgtable *pgtable = mmu->pgt;
> +	s8 start_level;
>  	int ret;
>  
> +	/*
> +	 * We only need the pgt start level to initialize the ptdump, get it
> +	 * while holding the mmu_lock. It's fine if the pgt gets freed
> +	 * afterwards, we'll check again when doing the actual dump.
> +	 */
> +	scoped_guard(write_lock, &kvm->mmu_lock) {

[Severity: Medium]
Does this code unnecessarily acquire the exclusive write lock just to read the
start_level in kvm_ptdump_parser_create()?

Since this is a read-only field check, acquiring the write lock could contend
with VM page faults. Could read_lock be used here instead?

> +		if (mmu->pgt)
> +			start_level = mmu->pgt->start_level;
> +		else
> +			return ERR_PTR(-ENOENT);
> +	}
[ ... ]
> @@ -149,6 +162,9 @@ static int kvm_ptdump_guest_show(struct seq_file *m, void *unused)
>  	};
>  
>  	guard(write_lock)(&kvm->mmu_lock);

[Severity: Medium]
Does this sequence create a denial of service regression by holding the
exclusive write lock for the entire duration of a heavy page table walk?

The full page table walk is performed synchronously while the exclusive lock
is held later in kvm_ptdump_guest_show():

    ret = kvm_pgtable_walk(mmu->pgt, 0, BIT(mmu->pgt->ia_bits), &walker);

This prolonged read-only walk blocks all VM page faults, causing severe lock
contention.

Since a user reading the ptdump file from debugfs can trigger this path, it
may allow host userspace to stall all vCPUs. Is it possible to perform this
walk using read_lock instead?

> +	if (!mmu->pgt)
> +		return 0;
> +
>  	st->parser_state = (struct ptdump_pg_state) {
[ ... ]
> @@ -211,17 +227,27 @@ static const struct file_operations kvm_ptdump_guest_fops = {
>  
>  static int kvm_pgtable_range_show(struct seq_file *m, void *unused)
>  {
> -	struct kvm_pgtable *pgtable = m->private;
> +	struct kvm_s2_mmu *mmu = m->private;
> +	struct kvm *kvm = kvm_s2_mmu_to_kvm(mmu);
> +
> +	guard(write_lock)(&kvm->mmu_lock);

[Severity: Medium]
Does this code unnecessarily acquire the exclusive write lock for a read-only
field check in kvm_pgtable_range_show()?

> +
> +	if (mmu->pgt)
> +		seq_printf(m, "%2u\n", mmu->pgt->ia_bits);
>  
> -	seq_printf(m, "%2u\n", pgtable->ia_bits);
>  	return 0;
>  }
>  
>  static int kvm_pgtable_levels_show(struct seq_file *m, void *unused)
>  {
> -	struct kvm_pgtable *pgtable = m->private;
> +	struct kvm_s2_mmu *mmu = m->private;
> +	struct kvm *kvm = kvm_s2_mmu_to_kvm(mmu);
> +
> +	guard(write_lock)(&kvm->mmu_lock);

[Severity: Medium]
Similarly, does this code unnecessarily acquire the exclusive write lock for a
read-only field check in kvm_pgtable_levels_show()?

> +
> +	if (mmu->pgt)
> +		seq_printf(m, "%1d\n", KVM_PGTABLE_MAX_LEVELS - mmu->pgt->start_level);
>  
> -	seq_printf(m, "%1d\n", KVM_PGTABLE_MAX_LEVELS - pgtable->start_level);
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916230337.4162485-1-weilin.chang@arm.com?part=1