From: Jinrong Liang <cloudliang@tencent.com>

From: Jinrong Liang <cloudliang@tencent.com>

Add test to verify the behavior of the pmu event filter when an
incomplete kvm_pmu_event_filter structure is used. By running the
test, we can ensure that the pmu event filter correctly handles
incomplete structures and does not allow events to be counted when
they should not be.

Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
---
 .../kvm/x86_64/pmu_event_filter_test.c        | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c b/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
index 9be4c6f8fb7e..a6b6e0d086ae 100644
--- a/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
+++ b/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
@@ -881,6 +881,24 @@ static bool fixed_ctr_is_allowed(uint8_t idx, uint32_t action, uint32_t bitmap)
 		(action == KVM_PMU_EVENT_DENY && !(bitmap & BIT_ULL(idx)));
 }
 
+struct incompatible_pmu_event_filter {
+	__u32 action;
+	__u32 nevents;
+	__u32 fixed_counter_bitmap;
+};
+
+static uint64_t test_incompatible_filter(struct kvm_vcpu *vcpu, uint32_t action,
+					 uint32_t bitmap)
+{
+	struct incompatible_pmu_event_filter err_f;
+
+	err_f.action = action;
+	err_f.fixed_counter_bitmap = bitmap;
+	ioctl((vcpu->vm)->fd, KVM_SET_PMU_EVENT_FILTER, &err_f.action);
+
+	return run_vcpu_to_sync(vcpu);
+}
+
 static void test_fixed_ctr_action_and_bitmap(struct kvm_vcpu *vcpu,
 					     uint8_t fixed_ctr_idx,
 					     uint8_t max_fixed_num)
@@ -918,6 +936,11 @@ static void test_fixed_ctr_action_and_bitmap(struct kvm_vcpu *vcpu,
 
 			TEST_ASSERT(expected == !!count,
 				    "Fixed event filter does not work as expected.");
+
+			/* Test incompatible event filter works as expected. */
+			count = test_incompatible_filter(vcpu, actions[i], bitmap);
+			TEST_ASSERT(expected == !!count,
+				    "Incompatible filter does not work as expected.");
 		}
 	}
 }
-- 
2.31.1

On Thu, Apr 20, 2023, Jinrong Liang wrote:
> From: Jinrong Liang <cloudliang@tencent.com>
> 
> From: Jinrong Liang <cloudliang@tencent.com>
> 
> Add test to verify the behavior of the pmu event filter when an
> incomplete kvm_pmu_event_filter structure is used. By running the
> test, we can ensure that the pmu event filter correctly handles
> incomplete structures and does not allow events to be counted when
> they should not be.
> 
> Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
> ---
>  .../kvm/x86_64/pmu_event_filter_test.c        | 23 +++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c b/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
> index 9be4c6f8fb7e..a6b6e0d086ae 100644
> --- a/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
> @@ -881,6 +881,24 @@ static bool fixed_ctr_is_allowed(uint8_t idx, uint32_t action, uint32_t bitmap)
>  		(action == KVM_PMU_EVENT_DENY && !(bitmap & BIT_ULL(idx)));
>  }
>  
> +struct incompatible_pmu_event_filter {
> +	__u32 action;
> +	__u32 nevents;
> +	__u32 fixed_counter_bitmap;
> +};
> +
> +static uint64_t test_incompatible_filter(struct kvm_vcpu *vcpu, uint32_t action,
> +					 uint32_t bitmap)
> +{
> +	struct incompatible_pmu_event_filter err_f;
> +
> +	err_f.action = action;
> +	err_f.fixed_counter_bitmap = bitmap;
> +	ioctl((vcpu->vm)->fd, KVM_SET_PMU_EVENT_FILTER, &err_f.action);

This is completely busted.  It "passes" by luck, not because it's a valid test.
The size of the argument is embedded in the IOCTL number itself, which means that
unless glibc is being very nice and using a macro + typeof + sizeof to sanity check
things, which I highly doubt is the case, this ioctl() is passing random stack data,
a.k.a. garbage, to KVM.

In short, drop this patch.

Sean Christopherson <seanjc@google.com> 于2023年5月25日周四 07:50写道：
>
> On Thu, Apr 20, 2023, Jinrong Liang wrote:
> > From: Jinrong Liang <cloudliang@tencent.com>
> >
> > From: Jinrong Liang <cloudliang@tencent.com>
> >
> > Add test to verify the behavior of the pmu event filter when an
> > incomplete kvm_pmu_event_filter structure is used. By running the
> > test, we can ensure that the pmu event filter correctly handles
> > incomplete structures and does not allow events to be counted when
> > they should not be.
> >
> > Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
> > ---
> >  .../kvm/x86_64/pmu_event_filter_test.c        | 23 +++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c b/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
> > index 9be4c6f8fb7e..a6b6e0d086ae 100644
> > --- a/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
> > +++ b/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
> > @@ -881,6 +881,24 @@ static bool fixed_ctr_is_allowed(uint8_t idx, uint32_t action, uint32_t bitmap)
> >               (action == KVM_PMU_EVENT_DENY && !(bitmap & BIT_ULL(idx)));
> >  }
> >
> > +struct incompatible_pmu_event_filter {
> > +     __u32 action;
> > +     __u32 nevents;
> > +     __u32 fixed_counter_bitmap;
> > +};
> > +
> > +static uint64_t test_incompatible_filter(struct kvm_vcpu *vcpu, uint32_t action,
> > +                                      uint32_t bitmap)
> > +{
> > +     struct incompatible_pmu_event_filter err_f;
> > +
> > +     err_f.action = action;
> > +     err_f.fixed_counter_bitmap = bitmap;
> > +     ioctl((vcpu->vm)->fd, KVM_SET_PMU_EVENT_FILTER, &err_f.action);
>
> This is completely busted.  It "passes" by luck, not because it's a valid test.
> The size of the argument is embedded in the IOCTL number itself, which means that
> unless glibc is being very nice and using a macro + typeof + sizeof to sanity check
> things, which I highly doubt is the case, this ioctl() is passing random stack data,
> a.k.a. garbage, to KVM.
>
> In short, drop this patch.

Thank you for letting us know about the issues with the patch. I will
drop the patch as suggested. Would you advise me to prepare version 3
to remove this patch?

On Thu, May 25, 2023, Jinrong Liang wrote:
> Sean Christopherson <seanjc@google.com> 于2023年5月25日周四 07:50写道：
> > > +static uint64_t test_incompatible_filter(struct kvm_vcpu *vcpu, uint32_t action,
> > > +                                      uint32_t bitmap)
> > > +{
> > > +     struct incompatible_pmu_event_filter err_f;
> > > +
> > > +     err_f.action = action;
> > > +     err_f.fixed_counter_bitmap = bitmap;
> > > +     ioctl((vcpu->vm)->fd, KVM_SET_PMU_EVENT_FILTER, &err_f.action);
> >
> > This is completely busted.  It "passes" by luck, not because it's a valid test.
> > The size of the argument is embedded in the IOCTL number itself, which means that
> > unless glibc is being very nice and using a macro + typeof + sizeof to sanity check
> > things, which I highly doubt is the case, this ioctl() is passing random stack data,
> > a.k.a. garbage, to KVM.
> >
> > In short, drop this patch.
> 
> Thank you for letting us know about the issues with the patch. I will
> drop the patch as suggested. Would you advise me to prepare version 3
> to remove this patch?

More comments on the other patches are incoming, please hold off on v3 until then.