* [PATCH v6 0/2] s390/vfio-ap: fix memory leak in mdev remove callback @ 2021-06-21 15:57 Tony Krowiak 2021-06-21 15:57 ` [PATCH v6 1/2] s390/vfio-ap: clean up mdev resources when remove callback invoked Tony Krowiak 2021-06-21 15:57 ` [PATCH v6 2/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer Tony Krowiak 0 siblings, 2 replies; 10+ messages in thread From: Tony Krowiak @ 2021-06-21 15:57 UTC (permalink / raw) To: linux-s390, linux-kernel Cc: borntraeger, cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, frankja, david, imbrenda, hca The mdev remove callback for the vfio_ap device driver bails out with -EBUSY if the mdev is in use by a KVM guest. The intended purpose was to prevent the mdev from being removed while in use; however, returning a non-zero rc does not prevent removal of the mdev. Consequently, the memory for the resources allocated when the mdev was created are leaked. To fix this issue: * The remove callback will not terminate with -EBUSY when the mdev is in use by a KVM guest. * The memory for the resources allocated when the mdev was created will be freed. * Since the struct ap_matrix_mdev now gets freed while the guest is is still running, we need to ensure that the pointer to the function that handles interception of the PQAP instruction executed on the guest is not accessed while it is being set to NULL. To prevent this, a r/w lock is introduced that protects the function pointer. Change log: v5 -> v6: -------- * Replaced struct kvm_s390_module_hook with function pointer int (*crypto_hook)(struct kvm_vcpu *vcpu) Tony Krowiak (2): s390/vfio-ap: clean up mdev resources when remove callback invoked s390/vfio-ap: r/w lock for PQAP interception handler function pointer arch/s390/include/asm/kvm_host.h | 8 +++---- arch/s390/kvm/kvm-s390.c | 1 + arch/s390/kvm/priv.c | 10 ++++---- drivers/s390/crypto/vfio_ap_ops.c | 33 ++++++++++++++------------- drivers/s390/crypto/vfio_ap_private.h | 2 +- 5 files changed, 28 insertions(+), 26 deletions(-) -- 2.30.2 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v6 1/2] s390/vfio-ap: clean up mdev resources when remove callback invoked 2021-06-21 15:57 [PATCH v6 0/2] s390/vfio-ap: fix memory leak in mdev remove callback Tony Krowiak @ 2021-06-21 15:57 ` Tony Krowiak 2021-06-21 16:04 ` Christian Borntraeger 2021-06-21 15:57 ` [PATCH v6 2/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer Tony Krowiak 1 sibling, 1 reply; 10+ messages in thread From: Tony Krowiak @ 2021-06-21 15:57 UTC (permalink / raw) To: linux-s390, linux-kernel Cc: borntraeger, cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, frankja, david, imbrenda, hca The mdev remove callback for the vfio_ap device driver bails out with -EBUSY if the mdev is in use by a KVM guest (i.e., the KVM pointer in the struct ap_matrix_mdev is not NULL). The intended purpose was to prevent the mdev from being removed while in use. There are two problems with this scenario: 1. Returning a non-zero return code from the remove callback does not prevent the removal of the mdev. 2. The KVM pointer in the struct ap_matrix_mdev will always be NULL because the remove callback will not get invoked until the mdev fd is closed. When the mdev fd is closed, the mdev release callback is invoked and clears the KVM pointer from the struct ap_matrix_mdev. Let's go ahead and remove the check for KVM in the remove callback and allow the cleanup of mdev resources to proceed. Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Cc: stable@vger.kernel.org --- drivers/s390/crypto/vfio_ap_ops.c | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c index b2c7e10dfdcd..122c85c22469 100644 --- a/drivers/s390/crypto/vfio_ap_ops.c +++ b/drivers/s390/crypto/vfio_ap_ops.c @@ -366,16 +366,6 @@ static int vfio_ap_mdev_remove(struct mdev_device *mdev) struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); mutex_lock(&matrix_dev->lock); - - /* - * If the KVM pointer is in flux or the guest is running, disallow - * un-assignment of control domain. - */ - if (matrix_mdev->kvm_busy || matrix_mdev->kvm) { - mutex_unlock(&matrix_dev->lock); - return -EBUSY; - } - vfio_ap_mdev_reset_queues(mdev); list_del(&matrix_mdev->node); kfree(matrix_mdev); -- 2.30.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v6 1/2] s390/vfio-ap: clean up mdev resources when remove callback invoked 2021-06-21 15:57 ` [PATCH v6 1/2] s390/vfio-ap: clean up mdev resources when remove callback invoked Tony Krowiak @ 2021-06-21 16:04 ` Christian Borntraeger 2021-06-21 17:27 ` Jason Gunthorpe 0 siblings, 1 reply; 10+ messages in thread From: Christian Borntraeger @ 2021-06-21 16:04 UTC (permalink / raw) To: Tony Krowiak, jgg Cc: cohuck, pasic, jjherne, alex.williamson, kwankhede, frankja, david, imbrenda, hca, Vasily Gorbik, linux-s390, linux-kernel On 21.06.21 17:57, Tony Krowiak wrote: > The mdev remove callback for the vfio_ap device driver bails out with > -EBUSY if the mdev is in use by a KVM guest (i.e., the KVM pointer in the > struct ap_matrix_mdev is not NULL). The intended purpose was > to prevent the mdev from being removed while in use. There are two > problems with this scenario: > > 1. Returning a non-zero return code from the remove callback does not > prevent the removal of the mdev. > > 2. The KVM pointer in the struct ap_matrix_mdev will always be NULL because > the remove callback will not get invoked until the mdev fd is closed. > When the mdev fd is closed, the mdev release callback is invoked and > clears the KVM pointer from the struct ap_matrix_mdev. > > Let's go ahead and remove the check for KVM in the remove callback and > allow the cleanup of mdev resources to proceed. > > Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > Cc: stable@vger.kernel.org This one is already queued on https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/log/?h=fixes Jason. Do you want this in stable? Then we should write a mail after merging. > --- > drivers/s390/crypto/vfio_ap_ops.c | 10 ---------- > 1 file changed, 10 deletions(-) > > diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c > index b2c7e10dfdcd..122c85c22469 100644 > --- a/drivers/s390/crypto/vfio_ap_ops.c > +++ b/drivers/s390/crypto/vfio_ap_ops.c > @@ -366,16 +366,6 @@ static int vfio_ap_mdev_remove(struct mdev_device *mdev) > struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); > > mutex_lock(&matrix_dev->lock); > - > - /* > - * If the KVM pointer is in flux or the guest is running, disallow > - * un-assignment of control domain. > - */ > - if (matrix_mdev->kvm_busy || matrix_mdev->kvm) { > - mutex_unlock(&matrix_dev->lock); > - return -EBUSY; > - } > - > vfio_ap_mdev_reset_queues(mdev); > list_del(&matrix_mdev->node); > kfree(matrix_mdev); > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v6 1/2] s390/vfio-ap: clean up mdev resources when remove callback invoked 2021-06-21 16:04 ` Christian Borntraeger @ 2021-06-21 17:27 ` Jason Gunthorpe 0 siblings, 0 replies; 10+ messages in thread From: Jason Gunthorpe @ 2021-06-21 17:27 UTC (permalink / raw) To: Christian Borntraeger Cc: Tony Krowiak, cohuck, pasic, jjherne, alex.williamson, kwankhede, frankja, david, imbrenda, hca, Vasily Gorbik, linux-s390, linux-kernel On Mon, Jun 21, 2021 at 06:04:54PM +0200, Christian Borntraeger wrote: > > > On 21.06.21 17:57, Tony Krowiak wrote: > > The mdev remove callback for the vfio_ap device driver bails out with > > -EBUSY if the mdev is in use by a KVM guest (i.e., the KVM pointer in the > > struct ap_matrix_mdev is not NULL). The intended purpose was > > to prevent the mdev from being removed while in use. There are two > > problems with this scenario: > > > > 1. Returning a non-zero return code from the remove callback does not > > prevent the removal of the mdev. > > > > 2. The KVM pointer in the struct ap_matrix_mdev will always be NULL because > > the remove callback will not get invoked until the mdev fd is closed. > > When the mdev fd is closed, the mdev release callback is invoked and > > clears the KVM pointer from the struct ap_matrix_mdev. > > > > Let's go ahead and remove the check for KVM in the remove callback and > > allow the cleanup of mdev resources to proceed. > > > > Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> > > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > > Cc: stable@vger.kernel.org > > This one is already queued on > > https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/log/?h=fixes > > Jason. Do you want this in stable? Then we should write a mail after > merging. It is fine as is, we are already at rc7, so long as it goes to some tree for this merge window Jason ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v6 2/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer 2021-06-21 15:57 [PATCH v6 0/2] s390/vfio-ap: fix memory leak in mdev remove callback Tony Krowiak 2021-06-21 15:57 ` [PATCH v6 1/2] s390/vfio-ap: clean up mdev resources when remove callback invoked Tony Krowiak @ 2021-06-21 15:57 ` Tony Krowiak 2021-06-30 15:18 ` Tony Krowiak 2021-07-01 15:13 ` Tony Krowiak 1 sibling, 2 replies; 10+ messages in thread From: Tony Krowiak @ 2021-06-21 15:57 UTC (permalink / raw) To: linux-s390, linux-kernel Cc: borntraeger, cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, frankja, david, imbrenda, hca The function pointer to the interception handler for the PQAP instruction can get changed during the interception process. Let's add a semaphore to struct kvm_s390_crypto to control read/write access to the function pointer contained therein. The semaphore must be locked for write access by the vfio_ap device driver when notified that the KVM pointer has been set or cleared. It must be locked for read access by the interception framework when the PQAP instruction is intercepted. Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Cc: stable@vger.kernel.org --- arch/s390/include/asm/kvm_host.h | 8 +++----- arch/s390/kvm/kvm-s390.c | 1 + arch/s390/kvm/priv.c | 10 ++++++---- drivers/s390/crypto/vfio_ap_ops.c | 23 +++++++++++++++++------ drivers/s390/crypto/vfio_ap_private.h | 2 +- 5 files changed, 28 insertions(+), 16 deletions(-) diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h index 8925f3969478..36441669ffba 100644 --- a/arch/s390/include/asm/kvm_host.h +++ b/arch/s390/include/asm/kvm_host.h @@ -803,14 +803,12 @@ struct kvm_s390_cpu_model { unsigned short ibc; }; -struct kvm_s390_module_hook { - int (*hook)(struct kvm_vcpu *vcpu); - struct module *owner; -}; +typedef int (*crypto_hook)(struct kvm_vcpu *vcpu); struct kvm_s390_crypto { struct kvm_s390_crypto_cb *crycb; - struct kvm_s390_module_hook *pqap_hook; + struct rw_semaphore pqap_hook_rwsem; + crypto_hook *pqap_hook; __u32 crycbd; __u8 aes_kw; __u8 dea_kw; diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index 1296fc10f80c..418d910df569 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -2606,6 +2606,7 @@ static void kvm_s390_crypto_init(struct kvm *kvm) { kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb; kvm_s390_set_crycb_format(kvm); + init_rwsem(&kvm->arch.crypto.pqap_hook_rwsem); if (!test_kvm_facility(kvm, 76)) return; diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c index 9928f785c677..6bed9406c1f3 100644 --- a/arch/s390/kvm/priv.c +++ b/arch/s390/kvm/priv.c @@ -610,6 +610,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu) static int handle_pqap(struct kvm_vcpu *vcpu) { struct ap_queue_status status = {}; + crypto_hook pqap_hook; unsigned long reg0; int ret; uint8_t fc; @@ -657,15 +658,16 @@ static int handle_pqap(struct kvm_vcpu *vcpu) * Verify that the hook callback is registered, lock the owner * and call the hook. */ + down_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem); if (vcpu->kvm->arch.crypto.pqap_hook) { - if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner)) - return -EOPNOTSUPP; - ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu); - module_put(vcpu->kvm->arch.crypto.pqap_hook->owner); + pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook; + ret = pqap_hook(vcpu); if (!ret && vcpu->run->s.regs.gprs[1] & 0x00ff0000) kvm_s390_set_psw_cc(vcpu, 3); + up_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem); return ret; } + up_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem); /* * A vfio_driver must register a hook. * No hook means no driver to enable the SIE CRYCB and no queues. diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c index 122c85c22469..742277bc8d1c 100644 --- a/drivers/s390/crypto/vfio_ap_ops.c +++ b/drivers/s390/crypto/vfio_ap_ops.c @@ -352,8 +352,7 @@ static int vfio_ap_mdev_create(struct mdev_device *mdev) vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->matrix); init_waitqueue_head(&matrix_mdev->wait_for_kvm); mdev_set_drvdata(mdev, matrix_mdev); - matrix_mdev->pqap_hook.hook = handle_pqap; - matrix_mdev->pqap_hook.owner = THIS_MODULE; + matrix_mdev->pqap_hook = handle_pqap; mutex_lock(&matrix_dev->lock); list_add(&matrix_mdev->node, &matrix_dev->mdev_list); mutex_unlock(&matrix_dev->lock); @@ -1115,15 +1114,20 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev, } kvm_get_kvm(kvm); + matrix_mdev->kvm = kvm; matrix_mdev->kvm_busy = true; mutex_unlock(&matrix_dev->lock); + + down_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem); + kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook; + up_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem); + kvm_arch_crypto_set_masks(kvm, matrix_mdev->matrix.apm, matrix_mdev->matrix.aqm, matrix_mdev->matrix.adm); + mutex_lock(&matrix_dev->lock); - kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook; - matrix_mdev->kvm = kvm; matrix_mdev->kvm_busy = false; wake_up_all(&matrix_mdev->wait_for_kvm); } @@ -1189,10 +1193,17 @@ static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev) if (matrix_mdev->kvm) { matrix_mdev->kvm_busy = true; mutex_unlock(&matrix_dev->lock); - kvm_arch_crypto_clear_masks(matrix_mdev->kvm); + + if (matrix_mdev->kvm->arch.crypto.crycbd) { + down_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem); + matrix_mdev->kvm->arch.crypto.pqap_hook = NULL; + up_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem); + + kvm_arch_crypto_clear_masks(matrix_mdev->kvm); + } + mutex_lock(&matrix_dev->lock); vfio_ap_mdev_reset_queues(matrix_mdev->mdev); - matrix_mdev->kvm->arch.crypto.pqap_hook = NULL; kvm_put_kvm(matrix_mdev->kvm); matrix_mdev->kvm = NULL; matrix_mdev->kvm_busy = false; diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h index f82a6396acae..e12218e5a629 100644 --- a/drivers/s390/crypto/vfio_ap_private.h +++ b/drivers/s390/crypto/vfio_ap_private.h @@ -86,7 +86,7 @@ struct ap_matrix_mdev { bool kvm_busy; wait_queue_head_t wait_for_kvm; struct kvm *kvm; - struct kvm_s390_module_hook pqap_hook; + crypto_hook pqap_hook; struct mdev_device *mdev; }; -- 2.30.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v6 2/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer 2021-06-21 15:57 ` [PATCH v6 2/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer Tony Krowiak @ 2021-06-30 15:18 ` Tony Krowiak 2021-07-01 15:25 ` Christian Borntraeger 2021-07-01 15:13 ` Tony Krowiak 1 sibling, 1 reply; 10+ messages in thread From: Tony Krowiak @ 2021-06-30 15:18 UTC (permalink / raw) To: linux-s390, linux-kernel Cc: borntraeger, cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, frankja, david, imbrenda, hca I assumed that this patch would get queued along with the other one in this series, but it looks like that was an erroneous assumption. Should this also be queued? On 6/21/21 11:57 AM, Tony Krowiak wrote: > The function pointer to the interception handler for the PQAP instruction > can get changed during the interception process. Let's add a > semaphore to struct kvm_s390_crypto to control read/write access to the > function pointer contained therein. > > The semaphore must be locked for write access by the vfio_ap device driver > when notified that the KVM pointer has been set or cleared. It must be > locked for read access by the interception framework when the PQAP > instruction is intercepted. > > Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > Cc: stable@vger.kernel.org > --- > arch/s390/include/asm/kvm_host.h | 8 +++----- > arch/s390/kvm/kvm-s390.c | 1 + > arch/s390/kvm/priv.c | 10 ++++++---- > drivers/s390/crypto/vfio_ap_ops.c | 23 +++++++++++++++++------ > drivers/s390/crypto/vfio_ap_private.h | 2 +- > 5 files changed, 28 insertions(+), 16 deletions(-) > > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h > index 8925f3969478..36441669ffba 100644 > --- a/arch/s390/include/asm/kvm_host.h > +++ b/arch/s390/include/asm/kvm_host.h > @@ -803,14 +803,12 @@ struct kvm_s390_cpu_model { > unsigned short ibc; > }; > > -struct kvm_s390_module_hook { > - int (*hook)(struct kvm_vcpu *vcpu); > - struct module *owner; > -}; > +typedef int (*crypto_hook)(struct kvm_vcpu *vcpu); > > struct kvm_s390_crypto { > struct kvm_s390_crypto_cb *crycb; > - struct kvm_s390_module_hook *pqap_hook; > + struct rw_semaphore pqap_hook_rwsem; > + crypto_hook *pqap_hook; > __u32 crycbd; > __u8 aes_kw; > __u8 dea_kw; > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 1296fc10f80c..418d910df569 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -2606,6 +2606,7 @@ static void kvm_s390_crypto_init(struct kvm *kvm) > { > kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb; > kvm_s390_set_crycb_format(kvm); > + init_rwsem(&kvm->arch.crypto.pqap_hook_rwsem); > > if (!test_kvm_facility(kvm, 76)) > return; > diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c > index 9928f785c677..6bed9406c1f3 100644 > --- a/arch/s390/kvm/priv.c > +++ b/arch/s390/kvm/priv.c > @@ -610,6 +610,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu) > static int handle_pqap(struct kvm_vcpu *vcpu) > { > struct ap_queue_status status = {}; > + crypto_hook pqap_hook; > unsigned long reg0; > int ret; > uint8_t fc; > @@ -657,15 +658,16 @@ static int handle_pqap(struct kvm_vcpu *vcpu) > * Verify that the hook callback is registered, lock the owner > * and call the hook. > */ > + down_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem); > if (vcpu->kvm->arch.crypto.pqap_hook) { > - if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner)) > - return -EOPNOTSUPP; > - ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu); > - module_put(vcpu->kvm->arch.crypto.pqap_hook->owner); > + pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook; > + ret = pqap_hook(vcpu); > if (!ret && vcpu->run->s.regs.gprs[1] & 0x00ff0000) > kvm_s390_set_psw_cc(vcpu, 3); > + up_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem); > return ret; > } > + up_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem); > /* > * A vfio_driver must register a hook. > * No hook means no driver to enable the SIE CRYCB and no queues. > diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c > index 122c85c22469..742277bc8d1c 100644 > --- a/drivers/s390/crypto/vfio_ap_ops.c > +++ b/drivers/s390/crypto/vfio_ap_ops.c > @@ -352,8 +352,7 @@ static int vfio_ap_mdev_create(struct mdev_device *mdev) > vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->matrix); > init_waitqueue_head(&matrix_mdev->wait_for_kvm); > mdev_set_drvdata(mdev, matrix_mdev); > - matrix_mdev->pqap_hook.hook = handle_pqap; > - matrix_mdev->pqap_hook.owner = THIS_MODULE; > + matrix_mdev->pqap_hook = handle_pqap; > mutex_lock(&matrix_dev->lock); > list_add(&matrix_mdev->node, &matrix_dev->mdev_list); > mutex_unlock(&matrix_dev->lock); > @@ -1115,15 +1114,20 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev, > } > > kvm_get_kvm(kvm); > + matrix_mdev->kvm = kvm; > matrix_mdev->kvm_busy = true; > mutex_unlock(&matrix_dev->lock); > + > + down_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem); > + kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook; > + up_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem); > + > kvm_arch_crypto_set_masks(kvm, > matrix_mdev->matrix.apm, > matrix_mdev->matrix.aqm, > matrix_mdev->matrix.adm); > + > mutex_lock(&matrix_dev->lock); > - kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook; > - matrix_mdev->kvm = kvm; > matrix_mdev->kvm_busy = false; > wake_up_all(&matrix_mdev->wait_for_kvm); > } > @@ -1189,10 +1193,17 @@ static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev) > if (matrix_mdev->kvm) { > matrix_mdev->kvm_busy = true; > mutex_unlock(&matrix_dev->lock); > - kvm_arch_crypto_clear_masks(matrix_mdev->kvm); > + > + if (matrix_mdev->kvm->arch.crypto.crycbd) { > + down_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem); > + matrix_mdev->kvm->arch.crypto.pqap_hook = NULL; > + up_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem); > + > + kvm_arch_crypto_clear_masks(matrix_mdev->kvm); > + } > + > mutex_lock(&matrix_dev->lock); > vfio_ap_mdev_reset_queues(matrix_mdev->mdev); > - matrix_mdev->kvm->arch.crypto.pqap_hook = NULL; > kvm_put_kvm(matrix_mdev->kvm); > matrix_mdev->kvm = NULL; > matrix_mdev->kvm_busy = false; > diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h > index f82a6396acae..e12218e5a629 100644 > --- a/drivers/s390/crypto/vfio_ap_private.h > +++ b/drivers/s390/crypto/vfio_ap_private.h > @@ -86,7 +86,7 @@ struct ap_matrix_mdev { > bool kvm_busy; > wait_queue_head_t wait_for_kvm; > struct kvm *kvm; > - struct kvm_s390_module_hook pqap_hook; > + crypto_hook pqap_hook; > struct mdev_device *mdev; > }; > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v6 2/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer 2021-06-30 15:18 ` Tony Krowiak @ 2021-07-01 15:25 ` Christian Borntraeger 2021-07-01 16:28 ` Tony Krowiak 2021-07-07 17:16 ` Tony Krowiak 0 siblings, 2 replies; 10+ messages in thread From: Christian Borntraeger @ 2021-07-01 15:25 UTC (permalink / raw) To: Tony Krowiak, linux-s390, linux-kernel Cc: cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, frankja, david, imbrenda, hca On 30.06.21 17:18, Tony Krowiak wrote: > I assumed that this patch would get queued along with the other one in this series, > but it looks like that was an erroneous assumption. Should this also be queued? Sorry, this is on my todo list. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v6 2/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer 2021-07-01 15:25 ` Christian Borntraeger @ 2021-07-01 16:28 ` Tony Krowiak 2021-07-07 17:16 ` Tony Krowiak 1 sibling, 0 replies; 10+ messages in thread From: Tony Krowiak @ 2021-07-01 16:28 UTC (permalink / raw) To: Christian Borntraeger, linux-s390, linux-kernel Cc: cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, frankja, david, imbrenda, hca On 7/1/21 11:25 AM, Christian Borntraeger wrote: > On 30.06.21 17:18, Tony Krowiak wrote: >> I assumed that this patch would get queued along with the other one >> in this series, >> but it looks like that was an erroneous assumption. Should this also >> be queued? > > Sorry, this is on my todo list. That's fine, it is probably a good thing it wasn't merged. I found a problem with the patch and am working on a replacement. I'll be posting it soon. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v6 2/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer 2021-07-01 15:25 ` Christian Borntraeger 2021-07-01 16:28 ` Tony Krowiak @ 2021-07-07 17:16 ` Tony Krowiak 1 sibling, 0 replies; 10+ messages in thread From: Tony Krowiak @ 2021-07-07 17:16 UTC (permalink / raw) To: Christian Borntraeger, linux-s390, linux-kernel Cc: cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, frankja, david, imbrenda, hca On 7/1/21 11:25 AM, Christian Borntraeger wrote: > On 30.06.21 17:18, Tony Krowiak wrote: >> I assumed that this patch would get queued along with the other one >> in this series, >> but it looks like that was an erroneous assumption. Should this also >> be queued? > > Sorry, this is on my todo list. I rolled this up into the patch I posted today: Message ID: <20210707154156.297139-1-akrowiak@linux.ibm.com> s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v6 2/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer 2021-06-21 15:57 ` [PATCH v6 2/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer Tony Krowiak 2021-06-30 15:18 ` Tony Krowiak @ 2021-07-01 15:13 ` Tony Krowiak 1 sibling, 0 replies; 10+ messages in thread From: Tony Krowiak @ 2021-07-01 15:13 UTC (permalink / raw) To: linux-s390, linux-kernel Cc: borntraeger, cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, frankja, david, imbrenda, hca On 6/21/21 11:57 AM, Tony Krowiak wrote: > The function pointer to the interception handler for the PQAP instruction > can get changed during the interception process. Let's add a > semaphore to struct kvm_s390_crypto to control read/write access to the > function pointer contained therein. > > The semaphore must be locked for write access by the vfio_ap device driver > when notified that the KVM pointer has been set or cleared. It must be > locked for read access by the interception framework when the PQAP > instruction is intercepted. > > Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > Cc: stable@vger.kernel.org > --- > arch/s390/include/asm/kvm_host.h | 8 +++----- > arch/s390/kvm/kvm-s390.c | 1 + > arch/s390/kvm/priv.c | 10 ++++++---- > drivers/s390/crypto/vfio_ap_ops.c | 23 +++++++++++++++++------ > drivers/s390/crypto/vfio_ap_private.h | 2 +- > 5 files changed, 28 insertions(+), 16 deletions(-) > > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h > index 8925f3969478..36441669ffba 100644 > --- a/arch/s390/include/asm/kvm_host.h > +++ b/arch/s390/include/asm/kvm_host.h > @@ -803,14 +803,12 @@ struct kvm_s390_cpu_model { > unsigned short ibc; > }; > > -struct kvm_s390_module_hook { > - int (*hook)(struct kvm_vcpu *vcpu); > - struct module *owner; > -}; > +typedef int (*crypto_hook)(struct kvm_vcpu *vcpu); > > struct kvm_s390_crypto { > struct kvm_s390_crypto_cb *crycb; > - struct kvm_s390_module_hook *pqap_hook; > + struct rw_semaphore pqap_hook_rwsem; > + crypto_hook *pqap_hook; > __u32 crycbd; > __u8 aes_kw; > __u8 dea_kw; > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 1296fc10f80c..418d910df569 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -2606,6 +2606,7 @@ static void kvm_s390_crypto_init(struct kvm *kvm) > { > kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb; > kvm_s390_set_crycb_format(kvm); > + init_rwsem(&kvm->arch.crypto.pqap_hook_rwsem); > > if (!test_kvm_facility(kvm, 76)) > return; > diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c > index 9928f785c677..6bed9406c1f3 100644 > --- a/arch/s390/kvm/priv.c > +++ b/arch/s390/kvm/priv.c > @@ -610,6 +610,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu) > static int handle_pqap(struct kvm_vcpu *vcpu) > { > struct ap_queue_status status = {}; > + crypto_hook pqap_hook; > unsigned long reg0; > int ret; > uint8_t fc; > @@ -657,15 +658,16 @@ static int handle_pqap(struct kvm_vcpu *vcpu) > * Verify that the hook callback is registered, lock the owner > * and call the hook. > */ > + down_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem); > if (vcpu->kvm->arch.crypto.pqap_hook) { > - if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner)) > - return -EOPNOTSUPP; > - ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu); > - module_put(vcpu->kvm->arch.crypto.pqap_hook->owner); > + pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook; > + ret = pqap_hook(vcpu); > if (!ret && vcpu->run->s.regs.gprs[1] & 0x00ff0000) > kvm_s390_set_psw_cc(vcpu, 3); > + up_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem); > return ret; > } > + up_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem); > /* > * A vfio_driver must register a hook. > * No hook means no driver to enable the SIE CRYCB and no queues. > diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c > index 122c85c22469..742277bc8d1c 100644 > --- a/drivers/s390/crypto/vfio_ap_ops.c > +++ b/drivers/s390/crypto/vfio_ap_ops.c > @@ -352,8 +352,7 @@ static int vfio_ap_mdev_create(struct mdev_device *mdev) > vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->matrix); > init_waitqueue_head(&matrix_mdev->wait_for_kvm); > mdev_set_drvdata(mdev, matrix_mdev); > - matrix_mdev->pqap_hook.hook = handle_pqap; > - matrix_mdev->pqap_hook.owner = THIS_MODULE; > + matrix_mdev->pqap_hook = handle_pqap; > mutex_lock(&matrix_dev->lock); > list_add(&matrix_mdev->node, &matrix_dev->mdev_list); > mutex_unlock(&matrix_dev->lock); > @@ -1115,15 +1114,20 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev, > } > > kvm_get_kvm(kvm); > + matrix_mdev->kvm = kvm; > matrix_mdev->kvm_busy = true; > mutex_unlock(&matrix_dev->lock); > + > + down_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem); > + kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook; Setting the hook using a field in matrix_mdev while not holding the matrix_dev->lock opens us up to a possible race condition. > + up_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem); > + > kvm_arch_crypto_set_masks(kvm, > matrix_mdev->matrix.apm, > matrix_mdev->matrix.aqm, > matrix_mdev->matrix.adm); > + > mutex_lock(&matrix_dev->lock); > - kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook; > - matrix_mdev->kvm = kvm; > matrix_mdev->kvm_busy = false; > wake_up_all(&matrix_mdev->wait_for_kvm); > } > @@ -1189,10 +1193,17 @@ static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev) > if (matrix_mdev->kvm) { > matrix_mdev->kvm_busy = true; > mutex_unlock(&matrix_dev->lock); > - kvm_arch_crypto_clear_masks(matrix_mdev->kvm); > + > + if (matrix_mdev->kvm->arch.crypto.crycbd) { > + down_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem); > + matrix_mdev->kvm->arch.crypto.pqap_hook = NULL; > + up_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem); > + > + kvm_arch_crypto_clear_masks(matrix_mdev->kvm); > + } > + > mutex_lock(&matrix_dev->lock); > vfio_ap_mdev_reset_queues(matrix_mdev->mdev); > - matrix_mdev->kvm->arch.crypto.pqap_hook = NULL; > kvm_put_kvm(matrix_mdev->kvm); > matrix_mdev->kvm = NULL; > matrix_mdev->kvm_busy = false; > diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h > index f82a6396acae..e12218e5a629 100644 > --- a/drivers/s390/crypto/vfio_ap_private.h > +++ b/drivers/s390/crypto/vfio_ap_private.h > @@ -86,7 +86,7 @@ struct ap_matrix_mdev { > bool kvm_busy; > wait_queue_head_t wait_for_kvm; > struct kvm *kvm; > - struct kvm_s390_module_hook pqap_hook; > + crypto_hook pqap_hook; > struct mdev_device *mdev; > }; > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-07-07 17:17 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-21 15:57 [PATCH v6 0/2] s390/vfio-ap: fix memory leak in mdev remove callback Tony Krowiak 2021-06-21 15:57 ` [PATCH v6 1/2] s390/vfio-ap: clean up mdev resources when remove callback invoked Tony Krowiak 2021-06-21 16:04 ` Christian Borntraeger 2021-06-21 17:27 ` Jason Gunthorpe 2021-06-21 15:57 ` [PATCH v6 2/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer Tony Krowiak 2021-06-30 15:18 ` Tony Krowiak 2021-07-01 15:25 ` Christian Borntraeger 2021-07-01 16:28 ` Tony Krowiak 2021-07-07 17:16 ` Tony Krowiak 2021-07-01 15:13 ` Tony Krowiak
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.