linux-kernel - Re: [PATCH] Documentation: kvm: fix SRCU locking order docs

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 13 Jan 2023 23:26:19 +0100
From:   Michal Luczaj <mhal@...x.co>
To:     David Woodhouse <dwmw2@...radead.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Boqun Feng <boqun.feng@...il.com>,
        "Paul E. McKenney" <paulmck@...nel.org>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        seanjc@...gle.com, Joel Fernandes <joel@...lfernandes.org>,
        Matthew Wilcox <willy@...radead.org>,
        Josh Triplett <josh@...htriplett.org>, rcu@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH] Documentation: kvm: fix SRCU locking order docs

On 1/13/23 12:03, David Woodhouse wrote:
> On Fri, 2023-01-13 at 10:33 +0000, David Woodhouse wrote:
>> So everything seems to be working as it should... *except* for the fact
>> that I don't quite understand why xen_shinfo_test didn't trigger the
>> warning. Michal, I guess you already worked that out when you came up
>> with your deadlock-test instead... is there something we should add to
>> xen_shinfo_test that would mean it *would* have triggered?

No, I didn't implement those deadlock selftests out of xen_shinfo_test
because there was some problem. I just wanted to have a cleaner workspace
and then, maybe, move them to xen_shinfo_test, which, well, did not happen
:) I guess there's no need for them filthy races anymore; lockdep does a
better job.

> Got it. It only happens when kvm_xen_set_evtchn() takes the slow path
> when kvm_xen_set_evtchn_fast() fails.

I fully agree. And sorry for late reply.

> Not utterly sure why that works
> in your deadlock_test but I can make it happen in xen_shinfo_test just
> by invalidating the GPC by changing the memslots:

Could it be that deadlocks_test starts with the right conditions, i.e.
invalid KVM_XEN_ATTR_TYPE_SHARED_INFO along with valid
KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO? xen_shinfo_test, on the other hand, have
them both valid, and so the fast path is taken.

I suppose instead of changing memslots, you can invalidate the
KVM_XEN_ATTR_TYPE_SHARED_INFO for that particular test unit, e.g.

	struct kvm_xen_hvm_attr ha = {
		.type = KVM_XEN_ATTR_TYPE_SHARED_INFO,
		.u.shared_info.gfn = KVM_XEN_INVALID_GFN,
	};
	vm_ioctl(vm, KVM_XEN_HVM_SET_ATTR, &ha);

One more thing concerning the lockdep priming you did in kvm_create_vm();

	mutex_lock(&kvm->lock);
	synchronize_srcu(&kvm->srcu);
	mutex_unlock(&kvm->lock)

It seems that deadlocks_test's set_msr_filter() effectively did the same
thanks to kvm_vm_ioctl_set_msr_filter()'s sync-under-mutex (which won't
happen if those I-used-to-be-a-deadlock optimization patches[*] get
merged). Naturally, xen_shinfo_test do not mess with MSR filters, so that
could be another reason for inconsistencies you've noticed before the
priming?

[*] https://lore.kernel.org/kvm/20230107001256.2365304-1-mhal@rbox.co/

Michal