Re: [PATCH v2 11/20] KVM: Add CLASS() constructs to automagically handle lock+check of gpc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The cleanup wrapper defined by `DEFINE_GPC_CLASS` uses `if (!IS_ERR(_T))` instead of `if (!IS_ERR_OR_NULL(_T))`, failing to guard against `NULL` pointers. This leads to a guaranteed kernel crash if ownership of the class variable is ever transferred or if it is initialized to `NULL`.
--

commit 88a61c5d744692bfbffaf3ccd332184029a7ffee
Author: Sean Christopherson <seanjc@google.com>

KVM: Add CLASS() constructs to automagically handle lock+check of gpc

This commit adds CLASS() definitions for mapping a PFN given a gfn_to_pfn
cache (gpc), deduplicating lock, check, and unlock loops. The pvclock code
is converted as the first user of this new construct.

> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h

[ ... ]

> +static inline void gpc_map_local_unlock(void **khva)
> +{
> +	struct gfn_to_pfn_cache *gpc = container_of(khva, struct gfn_to_pfn_cache, khva);
> +
> +	kvm_gpc_mark_dirty_in_slot(gpc);
> +
> +	read_unlock(&gpc->lock);
> +}
> +
> +static inline void gpc_map_local_unlock_ro(void **khva)
> +{
> +	read_unlock(&container_of(khva, struct gfn_to_pfn_cache, khva)->lock);
> +}
> +
> +#define DEFINE_GPC_CLASS(try, ro)						\
> +DEFINE_CLASS(gpc##try##_map_local##ro, void **,					\
> +	     if (!IS_ERR(_T)) gpc_map_local_unlock##ro(_T),			\

[Severity: High]
Does this guard expression properly handle NULL values, or could it lead to a
regression?

Since IS_ERR(NULL) evaluates to false, !IS_ERR(NULL) evaluates to true. If a
user of this class manually sets the variable to NULL or uses a primitive
like no_free_ptr() to inhibit cleanup, the destructor will run and
erroneously execute gpc_map_local_unlock(NULL).

Inside gpc_map_local_unlock(), container_of(NULL, ...) will produce a large
negative pointer. The subsequent calls to kvm_gpc_mark_dirty_in_slot() and
read_unlock() will dereference this invalid pointer and cause a regression.

Would it be safer to use if (!IS_ERR_OR_NULL(_T)) instead?

> +	     gpc##try##_map_local_lock(gpc, len),				\
> +	     struct gfn_to_pfn_cache *gpc, unsigned long len)			\
> +
> +DEFINE_GPC_CLASS(,);
> +DEFINE_GPC_CLASS(_try,);
> +DEFINE_GPC_CLASS(, _ro);
> +DEFINE_GPC_CLASS(_try, _ro);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529165114.748639-1-seanjc@google.com?part=11