linux-kernel - Re: [PATCH] KVM: X86: avoid meaningless kvm_apicv

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <5d633c81d24d40aa848e1605eb0df857@huawei.com>
Date:   Wed, 26 Feb 2020 03:20:33 +0000
From:   linmiaohe <linmiaohe@...wei.com>
To:     Vitaly Kuznetsov <vkuznets@...hat.com>
CC:     "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "rkrcmar@...hat.com" <rkrcmar@...hat.com>,
        "sean.j.christopherson@...el.com" <sean.j.christopherson@...el.com>,
        "suravee.suthikulpanit@....com" <suravee.suthikulpanit@....com>,
        "jmattson@...gle.com" <jmattson@...gle.com>,
        "joro@...tes.org" <joro@...tes.org>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "bp@...en8.de" <bp@...en8.de>, "hpa@...or.com" <hpa@...or.com>
Subject: Re: [PATCH] KVM: X86: avoid meaningless kvm_apicv_activated() check

Vitaly Kuznetsov <vkuznets@...hat.com> writes:
>linmiaohe <linmiaohe@...wei.com> writes:
>
>> From: Miaohe Lin <linmiaohe@...wei.com>
>>
>> After test_and_set_bit() for kvm->arch.apicv_inhibit_reasons, we will 
>> always get false when calling kvm_apicv_activated() because it's sure 
>> apicv_inhibit_reasons do not equal to 0.
>>
>> Signed-off-by: Miaohe Lin <linmiaohe@...wei.com>
>> ---
>>  arch/x86/kvm/x86.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 
>> ddcc51b89e2c..fa62dcb0ed0c 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -8018,8 +8018,7 @@ void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)
>>  		    !kvm_apicv_activated(kvm))
>>  			return;
>>  	} else {
>> -		if (test_and_set_bit(bit, &kvm->arch.apicv_inhibit_reasons) ||
>> -		    kvm_apicv_activated(kvm))
>> +		if (test_and_set_bit(bit, &kvm->arch.apicv_inhibit_reasons))
>>  			return;
>>  	}
>
>This seems to be correct in a sense that we are not really protected against concurrent modifications of 'apicv_inhibit_reasons' (like what if 'apicv_inhibit_reasons' gets modified right after we've checked 'kvm_apicv_activated(kvm)').

Yes, there might be a race window. But this looks benign as we recalculate kvm_apicv_activated() when we proceed with KVM_REQ_APICV_UPDATE.

>
>The function, however, still gives a flase impression it is somewhat protected against concurent modifications. Like what are these
>test_and_{set,clear}_bit() for?

Yes, I think so too. And also test_and_{set,clear}_bit() checks wheather the requested bit is {set,clear} to the requested state.

>
>If I'm not mistaken, the logic this function was supposed to implement
>is: change the requested bit to the requested state and, if
>kvm_apicv_activated() changed (we set the first bit or cleared the last), proceed with KVM_REQ_APICV_UPDATE. What if we re-write it like
>
>diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 2103101eca78..b97b8ff4a789 100644
>--- a/arch/x86/kvm/x86.c
>+++ b/arch/x86/kvm/x86.c
>@@ -8027,19 +8027,19 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_update_apicv);
>  */
> void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)  {
>+       bool apicv_was_activated = kvm_apicv_activated(kvm);
>+
>        if (!kvm_x86_ops->check_apicv_inhibit_reasons ||
>            !kvm_x86_ops->check_apicv_inhibit_reasons(bit))
>                return;
> 
>-       if (activate) {
>-               if (!test_and_clear_bit(bit, &kvm->arch.apicv_inhibit_reasons) ||
>-                   !kvm_apicv_activated(kvm))
>-                       return;
>-       } else {
>-               if (test_and_set_bit(bit, &kvm->arch.apicv_inhibit_reasons) ||
>-                   kvm_apicv_activated(kvm))
>-                       return;
>-       }
>+       if (activate)
>+               clear_bit(bit, &kvm->arch.apicv_inhibit_reasons);
>+       else
>+               set_bit(bit, &kvm->arch.apicv_inhibit_reasons);
>+
>+       if (kvm_apicv_activated(kvm) == apicv_was_activated)
>+               return;
> 
>        trace_kvm_apicv_update_request(activate, bit);
>        if (kvm_x86_ops->pre_update_apicv_exec_ctrl)
>
>Is this equal?
>

Looks good. I think this version also improves the readability. Many thanks for your advice and review!