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
| ||
|
Date: Thu, 22 Jul 2021 09:16:19 -0400 From: Tony Krowiak <akrowiak@...ux.ibm.com> To: jjherne@...ux.ibm.com, linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org Cc: borntraeger@...ibm.com, cohuck@...hat.com, pasic@...ux.vnet.ibm.com, jgg@...dia.com, alex.williamson@...hat.com, kwankhede@...dia.com, david@...hat.com Subject: Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification On 7/21/21 3:37 PM, Jason J. Herne wrote: > On 7/19/21 3:35 PM, Tony Krowiak wrote: >> It was pointed out during an unrelated patch review that locks should >> not >> be open coded - i.e., writing the algorithm of a standard lock in a >> function instead of using a lock from the standard library. The >> setting and >> testing of a busy flag and sleeping on a wait_event is the same thing >> a lock does. The open coded locks are invisible to lockdep, so potential >> locking problems are not detected. >> >> This patch removes the open coded locks used during >> VFIO_GROUP_NOTIFY_SET_KVM notification. The busy flag >> and wait queue were introduced to resolve a possible circular locking >> dependency reported by lockdep when starting a secure execution guest >> configured with AP adapters and domains. Reversing the order in which >> the kvm->lock mutex and matrix_dev->lock mutex are locked resolves the >> issue reported by lockdep, thus enabling the removal of the open coded >> locks. >> >> Signed-off-by: Tony Krowiak <akrowiak@...ux.ibm.com> >> --- >> arch/s390/kvm/kvm-s390.c | 27 +++++- >> drivers/s390/crypto/vfio_ap_ops.c | 132 ++++++++------------------ >> drivers/s390/crypto/vfio_ap_private.h | 2 - >> 3 files changed, 63 insertions(+), 98 deletions(-) >> >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >> index a08f242a9f27..4d2ef3a3286e 100644 >> --- a/arch/s390/kvm/kvm-s390.c >> +++ b/arch/s390/kvm/kvm-s390.c >> @@ -2559,12 +2559,24 @@ static void kvm_s390_set_crycb_format(struct >> kvm *kvm) >> kvm->arch.crypto.crycbd |= CRYCB_FORMAT1; >> } >> +/* >> + * kvm_arch_crypto_set_masks >> + * >> + * @kvm: a pointer to the object containing the crypto masks > > This should probably say "a pointer to the target guest's KVM struct" > or something to that effect. Same comment for the comment above > kvm_arch_crypto_clear_masks. Will do. > >> + * @apm: the mask identifying the accessible AP adapters >> + * @aqm: the mask identifying the accessible AP domains >> + * @adm: the mask identifying the accessible AP control domains >> + * >> + * Set the masks that identify the adapters, domains and control >> domains to >> + * which the KVM guest is granted access. >> + * >> + * Note: The kvm->lock mutex must be locked by the caller. >> + */ >> void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm, >> unsigned long *aqm, unsigned long *adm) >> { >> struct kvm_s390_crypto_cb *crycb = kvm->arch.crypto.crycb; >> - mutex_lock(&kvm->lock); >> kvm_s390_vcpu_block_all(kvm); >> switch (kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) { >> @@ -2595,13 +2607,21 @@ void kvm_arch_crypto_set_masks(struct kvm >> *kvm, unsigned long *apm, >> /* recreate the shadow crycb for each vcpu */ >> kvm_s390_sync_request_broadcast(kvm, KVM_REQ_VSIE_RESTART); >> kvm_s390_vcpu_unblock_all(kvm); >> - mutex_unlock(&kvm->lock); >> } >> EXPORT_SYMBOL_GPL(kvm_arch_crypto_set_masks); >> +/* >> + * kvm_arch_crypto_set_masks > > Copy/paste error here. Rename to kvm_arch_crypto_CLEAR_masks I will fix it. > > I did not find anything else in my review. However, I'm still very new > to this code, so take that with a grain of salt :). The grain of salt has been ingested. > > Also, I could not apply this to master. If there is a next version do > you mind rebasing? Seeing the patch in full context would be helpful. This was rebased on the latest master branch at the time then re-tested. This is something I always do prior to submitting patches, so is it possible you were using an older version of master? > > >
Powered by blists - more mailing lists