Re: [PATCH v2 1/2] RISC-V: KVM: Add PMU event filter support

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The implementation fails to clear the active filter when `nevents == 0` as documented in the commit message.
--

--- Patch [1]: [PATCH v2 1/2] RISC-V: KVM: Add PMU event filter support ---
commit f5fab5adc86a94658a09438bfb27fb33aa49c8df
Author: Yuhang.chen <[email protected]>

RISC-V: KVM: Add PMU event filter support
    
Allow userspace to restrict which SBI PMU events a guest is permitted
to program via the new VM ioctl KVM_SET_PMU_EVENT_FILTER.

> diff --git a/arch/riscv/kvm/vm.c b/arch/riscv/kvm/vm.c
> index a9f083feeb767..a5a3a2182d1e4 100644
> --- a/arch/riscv/kvm/vm.c
> +++ b/arch/riscv/kvm/vm.c

[ ... ]

> @@ -265,7 +268,71 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm, struct kvm_enable_cap *cap)

[ ... ]

> +	if (tmp.nevents > KVM_PMU_EVENT_FILTER_MAX_EVENTS)
> +		return -E2BIG;
> +
> +	size = struct_size(filter, events, tmp.nevents);
> +	filter = kzalloc(size, GFP_KERNEL_ACCOUNT);
> +	if (!filter)
> +		return -ENOMEM;

[Severity: Medium]
The commit message states that "nevents == 0 clears any active filter". 
However, this code unconditionally allocates and installs a new filter even
when nevents == 0, instead of skipping the allocation and setting the pointer
to NULL.

If userspace passes a zeroed structure to clear the filter (meaning nevents
is 0 and action defaults to 0, which is KVM_PMU_EVENT_ALLOW), won't this
install a 0-event ALLOW filter?

> diff --git a/arch/riscv/kvm/vcpu_pmu.c b/arch/riscv/kvm/vcpu_pmu.c
> index 2025b664961c8..cf360f8496e5e 100644
> --- a/arch/riscv/kvm/vcpu_pmu.c
> +++ b/arch/riscv/kvm/vcpu_pmu.c

[ ... ]

> @@ -733,6 +733,32 @@ int kvm_riscv_vcpu_pmu_ctr_stop(struct kvm_vcpu *vcpu, unsigned long ctr_base,

[ ... ]

> +	for (i = 0; i < filter->nevents; i++) {
> +		if ((unsigned long)filter->events[i] == eidx) {
> +			in_list = true;
> +			break;
> +		}
> +	}
> +
> +	/* ALLOW: permit only listed events; DENY: reject them. */
> +	if (filter->action == KVM_PMU_EVENT_ALLOW)
> +		return in_list;
> +	return !in_list;
> +}

[Severity: Medium]
If a 0-event ALLOW filter was installed because nevents was 0, in_list will
be false here. 

Does this result in kvm_riscv_pmu_event_allowed() returning false and
rejecting all guest PMU events, breaking PMU functionality rather than
restoring the default allow-all behavior?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1