Re: [RFC PATCH 16/20] mshv: Add debugfs interface to page tracker

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The newly introduced `mshv_iterate_preserved()` and its debugfs callers invoke KHO APIs without checking `kho_is_enabled()`, triggering a kernel warning on systems where KHO is disabled.
- [Medium] The error handling path in `mshv_debugfs_init()` fails to clean up resources allocated by `mshv_debugfs_parent_partition_create()`.
- [Medium] The `pt_tree` debugfs interface uses `single_open()` for potentially unbounded radix tree iteration, leading to O(N^2) processing overhead, prolonged lock contention, and allocation failures.
--

commit 0782bfee5c54e7b8251a28d7d1fba762890a41cc
Author: Jork Loeser <jloeser@linux.microsoft.com>

mshv: Add debugfs interface to page tracker

This commit adds debugfs files to expose statistics and tracked pages from the
MSHV page tracker, as well as the scheduler type. It also adds a new API to
iterate over preserved pages using the KHO radix tree.

> diff --git a/drivers/hv/mshv_debugfs.c b/drivers/hv/mshv_debugfs.c
> index 3c3e02237ae94..d79898e21b361 100644
> --- a/drivers/hv/mshv_debugfs.c
> +++ b/drivers/hv/mshv_debugfs.c

[ ... ]

> @@ -668,8 +675,89 @@ void mshv_debugfs_partition_remove(struct mshv_partition *partition)

[ ... ]

> +static int pt_stats_show(struct seq_file *m, void *v)
> +{
> +	const struct kho_radix_walk_cb cb = {
> +		.key = pt_count_data_cb,
> +		.table = pt_count_meta_cb,
> +	};
> +
> +	struct mshv_pt_stats pt_stats = {0};
> +
> +	mshv_iterate_preserved(&cb, &pt_stats);

[Severity: Medium]
Is it possible to read this file when KHO is disabled?

According to KHO subsystem rules, callers must gate KHO usage on
kho_is_enabled(). If kho_is_enabled() is false, preserved_pages_tree may remain
uninitialized (root is NULL), and kho_radix_walk_tree() will trigger a kernel
warning.

> +	seq_printf(m, "Data pages: %lu\n", pt_stats.count_data);
> +	seq_printf(m, "Meta pages: %lu\n", pt_stats.count_meta);
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(pt_stats);

[ ... ]

> +static int pt_tree_show(struct seq_file *m, void *v)
> +{
> +	const struct kho_radix_walk_cb cb = {
> +		.key = pt_tree_data_cb,
> +		.table = pt_tree_meta_cb,
> +	};
> +
> +	struct mshv_pt_stats pt_stats = {.stat_file = m};
> +
> +	mshv_iterate_preserved(&cb, &pt_stats);
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(pt_tree);

[Severity: Medium]
Will this lead to large allocations and O(N^2) lock contention for large VMs?

DEFINE_SHOW_ATTRIBUTE() uses single_open(), which requires all output to fit in
a single contiguous kmalloc buffer. Iterating over all preserved pages can scale
to millions of pages.

If the output exceeds the initial seq_file buffer, seq_printf() discards the
overflow, but the tree traversal continues to completion. Afterwards, seq_read()
will reallocate a larger buffer and restart the entire traversal. This creates
an retry loop that monopolizes the KHO radix tree lock and will likely
fail with -ENOMEM when kmalloc reaches the MAX_ORDER limit.

Should unbounded outputs use stateful seq_operations instead?

[ ... ]

> @@ -694,6 +782,17 @@ int __init mshv_debugfs_init(void)
>  	if (err)
>  		goto unmap_lp_stats;
>  
> +	err = mshv_debugfs_pt_create(mshv_debugfs);
> +	if (err)
> +		goto unmap_lp_stats;

[Severity: Medium]
Does this error path bypass cleaning up the parent partition stats?

If mshv_debugfs_pt_create() fails, it jumps to unmap_lp_stats, which
skips mshv_debugfs_parent_partition_remove(). This appears to permanently leak
the resources allocated by the preceding mshv_debugfs_parent_partition_create()
call.

> +
> +	d = debugfs_create_file("scheduler_info", 0400, mshv_debugfs, NULL,
> +				&scheduler_info_fops);
> +	if (IS_ERR(d)) {
> +		err = PTR_ERR(d);
> +		goto unmap_lp_stats;
> +	}
> +
>  	return 0;
>  
>  unmap_lp_stats:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528004204.1484584-1-jloeser@linux.microsoft.com?part=16