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: Tue, 31 May 2022 06:32:12 -0400 From: Tony Krowiak <akrowiak@...ux.ibm.com> To: jjherne@...ux.ibm.com, linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org, kvm@...r.kernel.org Cc: freude@...ux.ibm.com, borntraeger@...ibm.com, cohuck@...hat.com, mjrosato@...ux.ibm.com, pasic@...ux.ibm.com, alex.williamson@...hat.com, kwankhede@...dia.com, fiuczy@...ux.ibm.com Subject: Re: [PATCH v19 10/20] s390/vfio-ap: prepare for dynamic update of guest's APCB on assign/unassign On 5/27/22 9:18 AM, Jason J. Herne wrote: > On 4/4/22 18:10, Tony Krowiak wrote: >> The functions backing the matrix mdev's sysfs attribute interfaces to >> assign/unassign adapters, domains and control domains must take and >> release the locks required to perform a dynamic update of a guest's APCB >> in the proper order. >> >> The proper order for taking the locks is: >> >> matrix_dev->guests_lock => kvm->lock => matrix_dev->mdevs_lock >> >> The proper order for releasing the locks is: >> >> matrix_dev->mdevs_lock => kvm->lock => matrix_dev->guests_lock >> >> Two new macros are introduced for this purpose: One to take the locks >> and >> the other to release the locks. These macros will be used by the >> assignment/unassignment functions to prepare for dynamic update of >> the KVM guest's APCB. >> >> Signed-off-by: Tony Krowiak <akrowiak@...ux.ibm.com> >> --- >> drivers/s390/crypto/vfio_ap_ops.c | 69 +++++++++++++++++++++++++------ >> 1 file changed, 57 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/s390/crypto/vfio_ap_ops.c >> b/drivers/s390/crypto/vfio_ap_ops.c >> index 757bbf449b04..2219b1069ceb 100644 >> --- a/drivers/s390/crypto/vfio_ap_ops.c >> +++ b/drivers/s390/crypto/vfio_ap_ops.c >> @@ -71,6 +71,51 @@ static const struct vfio_device_ops >> vfio_ap_matrix_dev_ops; >> mutex_unlock(&matrix_dev->guests_lock); \ >> }) >> +/** >> + * get_update_locks_for_mdev: Acquire the locks required to >> dynamically update a >> + * KVM guest's APCB in the proper order. >> + * >> + * @matrix_mdev: a pointer to a struct ap_matrix_mdev object >> containing the AP >> + * configuration data to use to update a KVM guest's APCB. >> + * >> + * The proper locking order is: >> + * 1. matrix_dev->guests_lock: required to use the KVM pointer to >> update a KVM >> + * guest's APCB. >> + * 2. matrix_mdev->kvm->lock: required to update a guest's APCB >> + * 3. matrix_dev->mdevs_lock: required to access data stored in a >> matrix_mdev >> + * >> + * Note: If @matrix_mdev is NULL or is not attached to a KVM guest, >> the KVM >> + * lock will not be taken. >> + */ > > Perhaps the locking order should be documented once at the top of all > of the locking > functions instead of in each comment. The current method seems > needlessly verbose. Perhaps, but I surmise this comment was motivated by the fact you are reviewing the locking macros/functions en masse. On the other hand, someone debugging the code may miss the locking order comments if their debug thread leads them to a locking macro/function that does not have said comments. I think the value of leaving the comments in place outweighs the value of limiting them as you suggested. > >> +#define get_update_locks_for_mdev(matrix_mdev) ({ \ >> + mutex_lock(&matrix_dev->guests_lock); \ >> + if (matrix_mdev && matrix_mdev->kvm) \ >> + mutex_lock(&matrix_mdev->kvm->lock); \ >> + mutex_lock(&matrix_dev->mdevs_lock); \ >> +}) > > It does not make sense to reference matrix_dev on the first line of > this macro and > then check it for a null value on the next line. If it can be null > then the check > needs to come before the usage. If it cannot be null, then we can > remove the check. > Same comment for the release macro. You must have misread the code. The second line checks the value of matrix_mdev for NULL, not matrix_dev. There are definitely cases where matrix_mdev can be passed as NULL. > >> +/** >> + * release_update_locks_for_mdev: Release the locks used to >> dynamically update a >> + * KVM guest's APCB in the proper order. >> + * >> + * @matrix_mdev: a pointer to a struct ap_matrix_mdev object >> containing the AP >> + * configuration data to use to update a KVM guest's APCB. >> + * >> + * The proper unlocking order is: >> + * 1. matrix_dev->mdevs_lock >> + * 2. matrix_mdev->kvm->lock >> + * 3. matrix_dev->guests_lock >> + * >> + * Note: If @matrix_mdev is NULL or is not attached to a KVM guest, >> the KVM >> + * lock will not be released. >> + */ >> +#define release_update_locks_for_mdev(matrix_mdev) ({ \ >> + mutex_unlock(&matrix_dev->mdevs_lock); \ >> + if (matrix_mdev && matrix_mdev->kvm) \ >> + mutex_unlock(&matrix_mdev->kvm->lock); \ >> + mutex_unlock(&matrix_dev->guests_lock); \ >> +}) >> +
Powered by blists - more mailing lists