* Re: [PATCH] KVM: arm64: vgic: Annotate struct vgic_dist with __counted_by_ptr
2026-03-19 1:54 [PATCH] KVM: arm64: vgic: Annotate struct vgic_dist with __counted_by_ptr Bill Wendling
@ 2026-03-20 19:08 ` Kees Cook
0 siblings, 0 replies; 2+ messages in thread
From: Kees Cook @ 2026-03-20 19:08 UTC (permalink / raw)
To: Bill Wendling
Cc: linux-kernel, Marc Zyngier, Oliver Upton, Joey Gouly,
Suzuki K Poulose, Zenghui Yu, Gogul Balakrishnan,
Arman Hasanzadeh, linux-arm-kernel, kvmarm,
codemender-patching+linux
On Thu, Mar 19, 2026 at 01:54:10AM +0000, Bill Wendling wrote:
> Add the __counted_by_ptr attribute to the spis pointer field in struct
> vgic_dist. This pointer field points to an array of struct vgic_irq
> elements, and the number of elements is tracked by the nr_spis field
> within the same structure.
>
> The nr_spis field is initialized in vgic_init() (or earlier via
> userspace) before the spis array is allocated in kvm_vgic_dist_init().
> The nr_spis value remains constant during the lifetime of the spis
> allocation, making it a suitable counter for the array.
This one eluded me briefly, but yeah:
int vgic_init(struct kvm *kvm)
{
struct vgic_dist *dist = &kvm->arch.vgic;
...
/* freeze the number of spis */
if (!dist->nr_spis)
dist->nr_spis = VGIC_NR_IRQS_LEGACY - VGIC_NR_PRIVATE_IRQS;
ret = kvm_vgic_dist_init(kvm, dist->nr_spis);
static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int nr_spis)
{
struct vgic_dist *dist = &kvm->arch.vgic;
...
dist->spis = kzalloc_objs(struct vgic_irq, nr_spis, GFP_KERNEL_ACCOUNT);
It seems weird that nr_spis is passed to kvm_vgic_dist_init at all:
there's only 1 caller, and the value is stored in dist->nr_spis
(available through kvm) immediately before the call. For readability,
I'd almost prefer to see:
diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index 9b3091ad868c..9e6ca2f04581 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -190,16 +190,15 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
/**
* kvm_vgic_dist_init: initialize the dist data structures
* @kvm: kvm struct pointer
- * @nr_spis: number of spis, frozen by caller
*/
-static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int nr_spis)
+static int kvm_vgic_dist_init(struct kvm *kvm)
{
struct vgic_dist *dist = &kvm->arch.vgic;
struct kvm_vcpu *vcpu0 = kvm_get_vcpu(kvm, 0);
int i;
dist->active_spis = (atomic_t)ATOMIC_INIT(0);
- dist->spis = kzalloc_objs(struct vgic_irq, nr_spis, GFP_KERNEL_ACCOUNT);
+ dist->spis = kzalloc_objs(struct vgic_irq, dist->nr_spis, GFP_KERNEL_ACCOUNT);
if (!dist->spis)
return -ENOMEM;
@@ -211,7 +210,7 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int nr_spis)
* require prior initialization in case of a virtual GICv3 or trigger
* initialization when using a virtual GICv2.
*/
- for (i = 0; i < nr_spis; i++) {
+ for (i = 0; i < dist->nr_spis; i++) {
struct vgic_irq *irq = &dist->spis[i];
irq->intid = i + VGIC_NR_PRIVATE_IRQS;
@@ -401,7 +400,7 @@ int vgic_init(struct kvm *kvm)
if (!dist->nr_spis)
dist->nr_spis = VGIC_NR_IRQS_LEGACY - VGIC_NR_PRIVATE_IRQS;
- ret = kvm_vgic_dist_init(kvm, dist->nr_spis);
+ ret = kvm_vgic_dist_init(kvm);
if (ret)
goto out;
The other question I'd have (which isn't related to this patch
specifically) is if dist->spis is guaranteed to be NULL before
kvm_vgic_dist_init() is called.
Regardless:
Reviewed-by: Kees Cook <kees@kernel.org>
-Kees
>
> This patch was generated by CodeMender and reviewed by Bill Wendling.
> Tested with the KVM selftests.
>
> Signed-off-by: Bill Wendling <morbo@google.com>
> ---
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Oliver Upton <oupton@kernel.org>
> Cc: Joey Gouly <joey.gouly@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Zenghui Yu <yuzenghui@huawei.com>
> Cc: Gogul Balakrishnan <bgogul@google.com>
> Cc: Arman Hasanzadeh <armanihm@google.com>
> Cc: Kees Cook <kees@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: kvmarm@lists.linux.dev
> Cc: linux-kernel@vger.kernel.org
> Cc: codemender-patching+linux@google.com
> ---
> include/kvm/arm_vgic.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index f2eafc65bbf4..1cca87623d92 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -284,7 +284,7 @@ struct vgic_dist {
> /* Wants SGIs without active state */
> bool nassgireq;
>
> - struct vgic_irq *spis;
> + struct vgic_irq *spis __counted_by_ptr(nr_spis);
>
> struct vgic_io_device dist_iodev;
> struct vgic_io_device cpuif_iodev;
> --
> 2.53.0.851.ga537e3e6e9-goog
>
--
Kees Cook
^ permalink raw reply related [flat|nested] 2+ messages in thread