s390/vfio-ap: fix memory leak in mdev remove callback
Subsystems (5)
Tony Krowiak
maintainerPATCH v2 1/1 s390/vfio-ap: fix memory leak in mdev remove callback
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. This could result in a memory leak of the resources allocated when the mdev was created. In addition, the KVM guest will still have access to the AP devices assigned to the mdev even though the mdev no longer exists.
To prevent this scenario, cleanup will be done - including unplugging the AP adapters, domains and control domains - regardless of whether the mdev is in use by a KVM guest or not.
--- drivers/s390/crypto/vfio_ap_ops.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)
drivers/s390/crypto/vfio_ap_ops.c +2 -11
@@ -26,6 +26,7 @@static int vfio_ap_mdev_reset_queues (struct mdev_device *mdev);static struct vfio_ap_queue *vfio_ap_find_queue (int apqn); +static void vfio_ap_mdev_unset_kvm (struct ap_matrix_mdev *matrix_mdev);static int match_apqn (struct device *dev,const void *data) {
@@ -366,17 +367,7 @@ 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); +vfio_ap_mdev_unset_kvm (matrix_mdev);list_del (&matrix_mdev->node);kfree (matrix_mdev);mdev_set_drvdata (mdev,NULL );
Signature · 1 line
2.30.2
Tony Krowiak
maintainerQuoted 8 lines · Tony Krowiak
The mdev remove callback for the
vfio_apdevice driver bails out with-EBUSYif 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. This could result in a memory leak of the resources allocated when the mdev was created. In addition, the KVM guest will still have access to the AP devices assigned to the mdev even though the mdev no longer exists.
Quoted 7 lines
To prevent this scenario, cleanup will be done - including unplugging the AP adapters, domains and control domains - regardless of whether the mdev is in use by a KVM guest or not.
Fixes 258287c994de ("s390: vfio-ap: implement mediated device open callback")Signed-off-by Tony Krowiak <[email protected]>
The Signed-off-by was erroneously put in by the git sendemail command. Please take this out of your reply-all responses. thanks.
Quoted 35 lines
--- drivers/s390/crypto/vfio_ap_ops.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)drivers/s390/crypto/vfio_ap_ops.c +2
-11@@ -26,6 +26,7 @@static int vfio_ap_mdev_reset_queues (struct mdev_device *mdev);static struct vfio_ap_queue *vfio_ap_find_queue (int apqn); +static void vfio_ap_mdev_unset_kvm (struct ap_matrix_mdev *matrix_mdev);static int match_apqn (struct device *dev,const void *data) {@@ -366,17 +367,7 @@ 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); +vfio_ap_mdev_unset_kvm (matrix_mdev);list_del (&matrix_mdev->node);kfree (matrix_mdev);mdev_set_drvdata (mdev,NULL );
Quoted 12 lines · On Mon, 10 May 2021 17:48:37 -0400 Tony Krowiak <[email protected]> wrote:
The mdev remove callback for the
vfio_apdevice driver bails out with-EBUSYif 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. This could result in a memory leak of the resources allocated when the mdev was created. In addition, the KVM guest will still have access to the AP devices assigned to the mdev even though the mdev no longer exists.To prevent this scenario, cleanup will be done - including unplugging the AP adapters, domains and control domains - regardless of whether the mdev is in use by a KVM guest or not.
Quoted 6 lines
Fixes 258287c994de ("s390: vfio-ap: implement mediated device open callback")Signed-off-by Tony Krowiak <[email protected]>--- drivers/s390/crypto/vfio_ap_ops.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)
With the S-o-b fixed,
Quoted 12 lines · Tony Krowiak
The mdev remove callback for the
vfio_apdevice driver bails out with-EBUSYif 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. This could result in a memory leak of the resources allocated when the mdev was created. In addition, the KVM guest will still have access to the AP devices assigned to the mdev even though the mdev no longer exists.To prevent this scenario, cleanup will be done - including unplugging the AP adapters, domains and control domains - regardless of whether the mdev is in use by a KVM guest or not.
Quoted 6 lines
Fixes 258287c994de ("s390: vfio-ap: implement mediated device open callback")Signed-off-by Tony Krowiak <[email protected]>--- drivers/s390/crypto/vfio_ap_ops.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)
Can you please ensure this goes to a -rc branch or through Alex's tree?
Thanks,
Jason
Quoted 20 lines · Jason Gunthorpe
Quoted 12 lines · Tony Krowiak
The mdev remove callback for the
vfio_apdevice driver bails out with-EBUSYif 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. This could result in a memory leak of the resources allocated when the mdev was created. In addition, the KVM guest will still have access to the AP devices assigned to the mdev even though the mdev no longer exists.To prevent this scenario, cleanup will be done - including unplugging the AP adapters, domains and control domains - regardless of whether the mdev is in use by a KVM guest or not.
Quoted 6 lines
Fixes 258287c994de ("s390: vfio-ap: implement mediated device open callback")Signed-off-by Tony Krowiak <[email protected]>--- drivers/s390/crypto/vfio_ap_ops.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)
Quoted 2 lines
Can you please ensure this goes to a -rc branch or through Alex's tree?
So you want this is 5.13-rc?
I can apply this to the s390 tree if that is ok.
Quoted 22 lines · Christian Borntraeger
Quoted 19 lines · Jason Gunthorpe
Quoted 17 lines · Tony Krowiak
The mdev remove callback for the
vfio_apdevice driver bails out with-EBUSYif 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. This could result in a memory leak of the resources allocated when the mdev was created. In addition, the KVM guest will still have access to the AP devices assigned to the mdev even though the mdev no longer exists.To prevent this scenario, cleanup will be done - including unplugging the AP adapters, domains and control domains - regardless of whether the mdev is in use by a KVM guest or not.
Fixes 258287c994de ("s390: vfio-ap: implement mediated device open callback")Signed-off-by Tony Krowiak <[email protected]>drivers/s390/crypto/vfio_ap_ops.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)
Quoted 5 lines
Quoted 2 lines
Can you please ensure this goes to a -rc branch or through Alex's tree?
So you want this is 5.13-rc?
I can apply this to the s390 tree if that is ok.
Yes please
Jason
Tony Krowiak
maintainerQuoted 23 lines · Christian Borntraeger
Quoted 20 lines · Jason Gunthorpe
Quoted 18 lines · Tony Krowiak
The mdev remove callback for the
vfio_apdevice driver bails out with-EBUSYif 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. This could result in a memory leak of the resources allocated when the mdev was created. In addition, the KVM guest will still have access to the AP devices assigned to the mdev even though the mdev no longer exists.To prevent this scenario, cleanup will be done - including unplugging the AP adapters, domains and control domains - regardless of whether the mdev is in use by a KVM guest or not.
Fixes 258287c994de ("s390: vfio-ap: implement mediated device open callback")Signed-off-by Tony Krowiak <[email protected]>---
drivers/s390/crypto/vfio_ap_ops.c| 13 ++-----------1 file changed, 2 insertions(+), 11 deletions(-)
Quoted 5 lines
Quoted 2 lines
Can you please ensure this goes to a -rc branch or through Alex's tree?
So you want this is 5.13-rc?
I can apply this to the s390 tree if that is ok.
If it is in time for 5.13.-rc, then yes, go ahead and apply it.
Tony Krowiak
maintainerQuoted 13 lines · Jason Gunthorpe
Quoted 12 lines · Tony Krowiak
The mdev remove callback for the
vfio_apdevice driver bails out with-EBUSYif 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. This could result in a memory leak of the resources allocated when the mdev was created. In addition, the KVM guest will still have access to the AP devices assigned to the mdev even though the mdev no longer exists.To prevent this scenario, cleanup will be done - including unplugging the AP adapters, domains and control domains - regardless of whether the mdev is in use by a KVM guest or not.
Quoted 8 lines
Quoted 6 lines
Fixes 258287c994de ("s390: vfio-ap: implement mediated device open callback")Signed-off-by Tony Krowiak <[email protected]>--- drivers/s390/crypto/vfio_ap_ops.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)Can you please ensure this goes to a -rc branch or through Alex's tree?
I'm sorry, I don't know what a -rc branch is nor how to push this to Alex's tree, but I'd be happy to do so if you tell me now:)
Quoted 3 lines
Thanks,
Jason
Quoted 21 lines · Tony Krowiak
Quoted 18 lines · Jason Gunthorpe
Quoted 17 lines · Tony Krowiak
The mdev remove callback for the
vfio_apdevice driver bails out with-EBUSYif 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. This could result in a memory leak of the resources allocated when the mdev was created. In addition, the KVM guest will still have access to the AP devices assigned to the mdev even though the mdev no longer exists.To prevent this scenario, cleanup will be done - including unplugging the AP adapters, domains and control domains - regardless of whether the mdev is in use by a KVM guest or not.
Fixes 258287c994de ("s390: vfio-ap: implement mediated device open callback")Signed-off-by Tony Krowiak <[email protected]>drivers/s390/crypto/vfio_ap_ops.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)
Quoted 5 lines
Quoted 2 lines
Can you please ensure this goes to a -rc branch or through Alex's tree?
I'm sorry, I don't know what a -rc branch is nor how to push this to Alex's tree, but I'd be happy to do so if you tell me now:)
If Christian takes it for 5.13-rc then that is OK
Otherwise please ask AlexW to take it
Thanks,
Jason
Halil Pasic
maintainerQuoted 4 lines · On Thu, 13 May 2021 14:25:09 -0300 Jason Gunthorpe <[email protected]> wrote:
Quoted 2 lines
I'm sorry, I don't know what a -rc branch is nor how to push this to Alex's tree, but I'd be happy to do so if you tell me now:)
If Christian takes it for 5.13-rc then that is OK
I'm pretty confident that Christian is handling this. IMHO there is no need to bother Alex with it.
Regards,
Halil
Quoted 10 lines · Halil Pasic
Quoted 4 lines · On Thu, 13 May 2021 14:25:09 -0300 Jason Gunthorpe <[email protected]> wrote:
Quoted 2 lines
I'm sorry, I don't know what a -rc branch is nor how to push this to Alex's tree, but I'd be happy to do so if you tell me now:)
If Christian takes it for 5.13-rc then that is OK
I'm pretty confident that Christian is handling this. IMHO there is no need to bother Alex with it.
We need to ensure there are not a bunch of conflicts with the VFIO tree that will have a nother batch of work on this driver for the next merge window.
Jason
Quoted 8 lines · Tony Krowiak
The mdev remove callback for the
vfio_apdevice driver bails out with-EBUSYif 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. This could result in a memory leak of the resources allocated when the mdev was created. In addition, the KVM guest will still have access to the AP devices assigned to the mdev even though the mdev no longer exists.
Quoted 7 lines
To prevent this scenario, cleanup will be done - including unplugging the AP adapters, domains and control domains - regardless of whether the mdev is in use by a KVM guest or not.
Fixes 258287c994de ("s390: vfio-ap: implement mediated device open callback")Signed-off-by Tony Krowiak <[email protected]>
applied to the internal s390 tree. Lets give it some days of testing coverage of CI and others.
Vasily, can you add this for the s390/fixes branch early next week if nothing goes wrong?
Quoted 36 lines
--- drivers/s390/crypto/vfio_ap_ops.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)drivers/s390/crypto/vfio_ap_ops.c +2
-11@@ -26,6 +26,7 @@static int vfio_ap_mdev_reset_queues (struct mdev_device *mdev);static struct vfio_ap_queue *vfio_ap_find_queue (int apqn); +static void vfio_ap_mdev_unset_kvm (struct ap_matrix_mdev *matrix_mdev);static int match_apqn (struct device *dev,const void *data) {@@ -366,17 +367,7 @@ 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); +vfio_ap_mdev_unset_kvm (matrix_mdev);list_del (&matrix_mdev->node);kfree (matrix_mdev);mdev_set_drvdata (mdev,NULL );
Halil Pasic
maintainerQuoted 40 lines · On Mon, 10 May 2021 17:48:37 -0400 Tony Krowiak <[email protected]> wrote:
The mdev remove callback for the
vfio_apdevice driver bails out with-EBUSYif 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. This could result in a memory leak of the resources allocated when the mdev was created. In addition, the KVM guest will still have access to the AP devices assigned to the mdev even though the mdev no longer exists.To prevent this scenario, cleanup will be done - including unplugging the AP adapters, domains and control domains - regardless of whether the mdev is in use by a KVM guest or not.
Fixes 258287c994de ("s390: vfio-ap: implement mediated device open callback")Signed-off-by Tony Krowiak <[email protected]>--- drivers/s390/crypto/vfio_ap_ops.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)drivers/s390/crypto/vfio_ap_ops.c +1
-5@@ -26,6 +26,7 @@static int vfio_ap_mdev_reset_queues (struct mdev_device *mdev);static struct vfio_ap_queue *vfio_ap_find_queue (int apqn); +static void vfio_ap_mdev_unset_kvm (struct ap_matrix_mdev *matrix_mdev);static int match_apqn (struct device *dev,const void *data) {@@ -366,17 +367,7 @@ 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. -*/
drivers/s390/crypto/vfio_ap_ops.c static int vfio_ap_mdev_remove(struct mdev_device *mdev) · line 373
Quoted 10 lines
drivers/s390/crypto/vfio_ap_ops.c +1
-6-if (matrix_mdev->kvm_busy || matrix_mdev->kvm) { -mutex_unlock (&matrix_dev->lock); -return -EBUSY ; - } - -vfio_ap_mdev_reset_queues (mdev); +vfio_ap_mdev_unset_kvm (matrix_mdev);list_del (&matrix_mdev->node);kfree (matrix_mdev);
Are we at risk of handle_pqap() in arch/s390/kvm/priv.c using an already freed pqap_hook (which is a member of the matrix_mdev pointee that is freed just above my comment).
I'm aware of the fact that vfio_ap_mdev_unset_kvm() does a matrix_mdev->kvm->arch.crypto.pqap_hook = NULL but that is AFRICT not done under any lock relevant for handle_pqap(). I guess the idea is, I guess, the check cited below
static int handle_pqap(struct kvm_vcpu *vcpu)
[..]
/*
* Verify that the hook callback is registered, lock the owner
* and call the hook.
*/
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);
if (!ret && vcpu->run->s.regs.gprs[1] & 0x00ff0000)
kvm_s390_set_psw_cc(vcpu, 3);
return ret;
}is going to catch it, but I'm not sure it is guaranteed to catch it. Opinions?
Regards,
Halil
Quoted 1 line
mdev_set_drvdata(mdev, NULL);
Tony Krowiak
maintainerQuoted 77 lines · Halil Pasic
Quoted 39 lines · On Mon, 10 May 2021 17:48:37 -0400 Tony Krowiak <[email protected]> wrote:
The mdev remove callback for the
vfio_apdevice driver bails out with-EBUSYif 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. This could result in a memory leak of the resources allocated when the mdev was created. In addition, the KVM guest will still have access to the AP devices assigned to the mdev even though the mdev no longer exists.To prevent this scenario, cleanup will be done - including unplugging the AP adapters, domains and control domains - regardless of whether the mdev is in use by a KVM guest or not.
Fixes 258287c994de ("s390: vfio-ap: implement mediated device open callback")Signed-off-by Tony Krowiak <[email protected]>--- drivers/s390/crypto/vfio_ap_ops.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)drivers/s390/crypto/vfio_ap_ops.c +1
-4@@ -26,6 +26,7 @@static int vfio_ap_mdev_reset_queues (struct mdev_device *mdev);static struct vfio_ap_queue *vfio_ap_find_queue (int apqn); +static void vfio_ap_mdev_unset_kvm (struct ap_matrix_mdev *matrix_mdev);static int match_apqn (struct device *dev,const void *data) {@@ -366,17 +367,7 @@ 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. drivers/s390/crypto/vfio_ap_ops.c static int vfio_ap_mdev_remove(struct mdev_device *mdev) · line 372
Quoted 10 lines
drivers/s390/crypto/vfio_ap_ops.c +1
-7-*/ -if (matrix_mdev->kvm_busy || matrix_mdev->kvm) { -mutex_unlock (&matrix_dev->lock); -return -EBUSY ; - } - -vfio_ap_mdev_reset_queues (mdev); +vfio_ap_mdev_unset_kvm (matrix_mdev);list_del (&matrix_mdev->node);kfree (matrix_mdev);Are we at risk of
handle_pqap()inarch/s390/kvm/priv.cusing an already freedpqap_hook(which is a member of thematrix_mdevpointee that is freed just above my comment).I'm aware of the fact that
vfio_ap_mdev_unset_kvm()does amatrix_mdev->kvm->arch.crypto.pqap_hook= NULL but that is AFRICT not done under any lock relevant forhandle_pqap(). I guess the idea is, I guess, the check cited belowstatic int
handle_pqap(struct kvm_vcpu *vcpu)
[..]/* * Verify that the hook callback is registered, lock the owner * and call the hook. */ 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); if (!ret && vcpu->run->s.regs.gprs[1] & 0x00ff0000) kvm_s390_set_psw_cc(vcpu, 3); return ret; }
Quoted 2 lines
is going to catch it, but I'm not sure it is guaranteed to catch it. Opinions?
The hook itself - handle_pqap() function in vfio_ap_ops.c - also checks to see if the reference to the hook is set and terminates with an error if it is not. If the hook is invoked subsequent to the remove callback above, all should be fine since the check is also done under the matrix_dev->lock.
Quoted 6 lines
Regards,
HalilQuoted 1 line
drivers/s390/crypto/vfio_ap_ops.c +0
-0mdev_set_drvdata (mdev,NULL );
Halil Pasic
maintainerQuoted 78 lines · On Thu, 13 May 2021 10:35:05 -0400 Tony Krowiak <[email protected]> wrote:
Quoted 77 lines · Halil Pasic
Quoted 39 lines · On Mon, 10 May 2021 17:48:37 -0400 Tony Krowiak <[email protected]> wrote:
The mdev remove callback for the
vfio_apdevice driver bails out with-EBUSYif 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. This could result in a memory leak of the resources allocated when the mdev was created. In addition, the KVM guest will still have access to the AP devices assigned to the mdev even though the mdev no longer exists.To prevent this scenario, cleanup will be done - including unplugging the AP adapters, domains and control domains - regardless of whether the mdev is in use by a KVM guest or not.
Fixes 258287c994de ("s390: vfio-ap: implement mediated device open callback")Signed-off-by Tony Krowiak <[email protected]>--- drivers/s390/crypto/vfio_ap_ops.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)drivers/s390/crypto/vfio_ap_ops.c +1
-4@@ -26,6 +26,7 @@static int vfio_ap_mdev_reset_queues (struct mdev_device *mdev);static struct vfio_ap_queue *vfio_ap_find_queue (int apqn); +static void vfio_ap_mdev_unset_kvm (struct ap_matrix_mdev *matrix_mdev);static int match_apqn (struct device *dev,const void *data) {@@ -366,17 +367,7 @@ 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. drivers/s390/crypto/vfio_ap_ops.c static int vfio_ap_mdev_remove(struct mdev_device *mdev) · line 372
Quoted 10 lines
drivers/s390/crypto/vfio_ap_ops.c +1
-7-*/ -if (matrix_mdev->kvm_busy || matrix_mdev->kvm) { -mutex_unlock (&matrix_dev->lock); -return -EBUSY ; - } - -vfio_ap_mdev_reset_queues (mdev); +vfio_ap_mdev_unset_kvm (matrix_mdev);list_del (&matrix_mdev->node);kfree (matrix_mdev);Are we at risk of
handle_pqap()inarch/s390/kvm/priv.cusing an already freedpqap_hook(which is a member of thematrix_mdevpointee that is freed just above my comment).I'm aware of the fact that
vfio_ap_mdev_unset_kvm()does amatrix_mdev->kvm->arch.crypto.pqap_hook= NULL but that is AFRICT not done under any lock relevant forhandle_pqap(). I guess the idea is, I guess, the check cited belowstatic int
handle_pqap(struct kvm_vcpu *vcpu)
[..]/* * Verify that the hook callback is registered, lock the owner * and call the hook. */ 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); if (!ret && vcpu->run->s.regs.gprs[1] & 0x00ff0000) kvm_s390_set_psw_cc(vcpu, 3); return ret; }
Quoted 9 lines
Quoted 2 lines
is going to catch it, but I'm not sure it is guaranteed to catch it. Opinions?
The hook itself -
handle_pqap()function invfio_ap_ops.c- also checks to see if the reference to the hook is set and terminates with an error if it
is not. If the hook is invoked subsequent to the remove callback above, all should be fine since the check is also done under thematrix_dev->lock.
I don't quite understand your logic. Let us assume matrix_mdev was freed, but vcpu->kvm->arch.crypto.pqap_hook still points to what used to be (*matrix_mdev).pqap_hook. In that case the function pointer vcpu->kvm->arch.crypto.pqap_hook->hook is used after it was freed, and may not point to the handle_pqap() function in vfio_ap_ops.c, thus the check you are referring to ain't necessarily relevant. Than is if you mean the check in the handle_pqap() function in vfio_ap_ops.c; if you mean the check in handle_pqap() in arch/s390/kvm/priv.c, that one is not done under the matrix_dev->lock. Or do I have a hole somewhere in my reasoning?
Regards,
Halil
Tony Krowiak
maintainerQuoted 89 lines · Halil Pasic
Quoted 78 lines · On Thu, 13 May 2021 10:35:05 -0400 Tony Krowiak <[email protected]> wrote:
Quoted 77 lines · Halil Pasic
Quoted 39 lines · On Mon, 10 May 2021 17:48:37 -0400 Tony Krowiak <[email protected]> wrote:
The mdev remove callback for the
vfio_apdevice driver bails out with-EBUSYif 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. This could result in a memory leak of the resources allocated when the mdev was created. In addition, the KVM guest will still have access to the AP devices assigned to the mdev even though the mdev no longer exists.To prevent this scenario, cleanup will be done - including unplugging the AP adapters, domains and control domains - regardless of whether the mdev is in use by a KVM guest or not.
Fixes 258287c994de ("s390: vfio-ap: implement mediated device open callback")Signed-off-by Tony Krowiak <[email protected]>--- drivers/s390/crypto/vfio_ap_ops.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)drivers/s390/crypto/vfio_ap_ops.c +1
-4@@ -26,6 +26,7 @@static int vfio_ap_mdev_reset_queues (struct mdev_device *mdev);static struct vfio_ap_queue *vfio_ap_find_queue (int apqn); +static void vfio_ap_mdev_unset_kvm (struct ap_matrix_mdev *matrix_mdev);static int match_apqn (struct device *dev,const void *data) {@@ -366,17 +367,7 @@ 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. drivers/s390/crypto/vfio_ap_ops.c static int vfio_ap_mdev_remove(struct mdev_device *mdev) · line 372
Quoted 10 lines
drivers/s390/crypto/vfio_ap_ops.c +1
-7-*/ -if (matrix_mdev->kvm_busy || matrix_mdev->kvm) { -mutex_unlock (&matrix_dev->lock); -return -EBUSY ; - } - -vfio_ap_mdev_reset_queues (mdev); +vfio_ap_mdev_unset_kvm (matrix_mdev);list_del (&matrix_mdev->node);kfree (matrix_mdev);Are we at risk of
handle_pqap()inarch/s390/kvm/priv.cusing an already freedpqap_hook(which is a member of thematrix_mdevpointee that is freed just above my comment).I'm aware of the fact that
vfio_ap_mdev_unset_kvm()does amatrix_mdev->kvm->arch.crypto.pqap_hook= NULL but that is AFRICT not done under any lock relevant forhandle_pqap(). I guess the idea is, I guess, the check cited belowstatic int
handle_pqap(struct kvm_vcpu *vcpu)
[..]/* * Verify that the hook callback is registered, lock the owner * and call the hook. */ 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); if (!ret && vcpu->run->s.regs.gprs[1] & 0x00ff0000) kvm_s390_set_psw_cc(vcpu, 3); return ret; }Quoted 8 lines
Quoted 2 lines
is going to catch it, but I'm not sure it is guaranteed to catch it. Opinions?
The hook itself -
handle_pqap()function invfio_ap_ops.c- also checks to see if the reference to the hook is set and terminates with an error if it
is not. If the hook is invoked subsequent to the remove callback above, all should be fine since the check is also done under thematrix_dev->lock.
Quoted 10 lines
I don't quite understand your logic. Let us assume
matrix_mdevwas freed, butvcpu->kvm->arch.crypto.pqap_hookstill points to what used to be (*matrix_mdev).pqap_hook. In that case the function pointervcpu->kvm->arch.crypto.pqap_hook->hookis used after it was freed, and may not point to thehandle_pqap()function invfio_ap_ops.c, thus the check you are referring to ain't necessarily relevant. Than is if you mean the check in thehandle_pqap()function invfio_ap_ops.c; if you mean the check inhandle_pqap()inarch/s390/kvm/priv.c, that one is not done under thematrix_dev->lock. Or do I have a hole somewhere in my reasoning?
What I am saying is the vcpu->kvm->arch.crypto.pqap_hook
will either be NULL or point to the handle_pqap() function in the vfio_ap driver. In the latter case, the handler in the driver will get called and try to acquire the matrix_dev->lock. The function that sets the vcpu->kvm->arch.crypto.pqap_hook to NULL also takes that lock. If the pointer is still active, then the handler will do its thing. If not, then the handler will return without enabling or disabling IRQs. That should not be a problem since the unset_kvm function resets the queues which will disable the IRQs.
I don't see how
the vcpu->kvm->arch.crypto.pqap_hook can point to anything other than the handler or be NULL unless KVM is gone. Based on my observations of the behavior, unless there is some other way for the remove callback to be invoked other than in response to a request from userspace via the sysfs remove attribute, it will not get called until the file descriptor is closed in which case the release callback will also unset_kvm. I think you are worrying about something that will likely never happen.
Quoted 4 lines
Regards,
Halil
Halil Pasic
maintainerQuoted 101 lines · On Thu, 13 May 2021 15:23:27 -0400 Tony Krowiak <[email protected]> wrote:
Quoted 89 lines · Halil Pasic
Quoted 78 lines · On Thu, 13 May 2021 10:35:05 -0400 Tony Krowiak <[email protected]> wrote:
Quoted 77 lines · Halil Pasic
Quoted 39 lines · On Mon, 10 May 2021 17:48:37 -0400 Tony Krowiak <[email protected]> wrote:
The mdev remove callback for the
vfio_apdevice driver bails out with-EBUSYif 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. This could result in a memory leak of the resources allocated when the mdev was created. In addition, the KVM guest will still have access to the AP devices assigned to the mdev even though the mdev no longer exists.To prevent this scenario, cleanup will be done - including unplugging the AP adapters, domains and control domains - regardless of whether the mdev is in use by a KVM guest or not.
Fixes 258287c994de ("s390: vfio-ap: implement mediated device open callback")Signed-off-by Tony Krowiak <[email protected]>--- drivers/s390/crypto/vfio_ap_ops.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)drivers/s390/crypto/vfio_ap_ops.c +1
-4@@ -26,6 +26,7 @@static int vfio_ap_mdev_reset_queues (struct mdev_device *mdev);static struct vfio_ap_queue *vfio_ap_find_queue (int apqn); +static void vfio_ap_mdev_unset_kvm (struct ap_matrix_mdev *matrix_mdev);static int match_apqn (struct device *dev,const void *data) {@@ -366,17 +367,7 @@ 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. drivers/s390/crypto/vfio_ap_ops.c static int vfio_ap_mdev_remove(struct mdev_device *mdev) · line 372
Quoted 10 lines
drivers/s390/crypto/vfio_ap_ops.c +1
-7-*/ -if (matrix_mdev->kvm_busy || matrix_mdev->kvm) { -mutex_unlock (&matrix_dev->lock); -return -EBUSY ; - } - -vfio_ap_mdev_reset_queues (mdev); +vfio_ap_mdev_unset_kvm (matrix_mdev);list_del (&matrix_mdev->node);kfree (matrix_mdev);Are we at risk of
handle_pqap()inarch/s390/kvm/priv.cusing an already freedpqap_hook(which is a member of thematrix_mdevpointee that is freed just above my comment).I'm aware of the fact that
vfio_ap_mdev_unset_kvm()does amatrix_mdev->kvm->arch.crypto.pqap_hook= NULL but that is AFRICT not done under any lock relevant forhandle_pqap(). I guess the idea is, I guess, the check cited belowstatic int
handle_pqap(struct kvm_vcpu *vcpu)
[..]/* * Verify that the hook callback is registered, lock the owner * and call the hook. */ 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); if (!ret && vcpu->run->s.regs.gprs[1] & 0x00ff0000) kvm_s390_set_psw_cc(vcpu, 3); return ret; }Quoted 8 lines
Quoted 2 lines
is going to catch it, but I'm not sure it is guaranteed to catch it. Opinions?
The hook itself -
handle_pqap()function invfio_ap_ops.c- also checks to see if the reference to the hook is set and terminates with an error if it
is not. If the hook is invoked subsequent to the remove callback above, all should be fine since the check is also done under thematrix_dev->lock.Quoted 10 lines
I don't quite understand your logic. Let us assume
matrix_mdevwas freed, butvcpu->kvm->arch.crypto.pqap_hookstill points to what used to be (*matrix_mdev).pqap_hook. In that case the function pointervcpu->kvm->arch.crypto.pqap_hook->hookis used after it was freed, and may not point to thehandle_pqap()function invfio_ap_ops.c, thus the check you are referring to ain't necessarily relevant. Than is if you mean the check in thehandle_pqap()function invfio_ap_ops.c; if you mean the check inhandle_pqap()inarch/s390/kvm/priv.c, that one is not done under thematrix_dev->lock. Or do I have a hole somewhere in my reasoning?
Quoted 3 lines
What I am saying is the
vcpu->kvm->arch.crypto.pqap_hookwill either be NULL or point to thehandle_pqap()function in thevfio_apdriver.
Please read the code again. In my reading of the code vcpu->kvm->arch.crypto.pqap_hook is never supposed to point to >(or does point to) the handle_pqap() function defined in vfio_ap_ops.c. It points to the pqap_hook member of struct ap_matrix_mdev (the type of the member is struct kvm_s390_module_hook, which in turn has a function pointer member called hook, which is supposed to hold the address of handle_pqap() function defined in vfio_ap_ops.c, and thus point to it).
Because of this, I don't think the rest of your argument is valid. Furthermore I believe we first need to get to common ground on this one before proceeding any further. If you happen to preserve your opinion after checking again, I think we should try to discuss this offline, as one of us is likely looking at the wrong code.
Regards,
Halil
Quoted 24 lines
In the latter case, the handler in the driver will get
called and try to acquire thematrix_dev->lock. The function that sets thevcpu->kvm->arch.crypto.pqap_hookto NULL also takes that lock. If the pointer is still active, then the handler will do its thing. If not, then the handler will return without enabling or disabling IRQs. That should not be a problem since theunset_kvmfunction resets the queues which will disable the IRQs.I don't see how
thevcpu->kvm->arch.crypto.pqap_hookcan point to anything other than the handler or be NULL unless KVM is gone. Based on my observations of the behavior, unless there is some other way for the remove callback to be invoked other than in response to a request from userspace via the sysfs remove attribute, it will not get called until the file descriptor is closed in which case the release callback will alsounset_kvm. I think you are worrying about something that will likely never happen.Quoted 4 lines
Regards,
Halil
Tony Krowiak
maintainerQuoted 106 lines · Halil Pasic
Quoted 100 lines · On Thu, 13 May 2021 15:23:27 -0400 Tony Krowiak <[email protected]> wrote:
Quoted 89 lines · Halil Pasic
Quoted 78 lines · On Thu, 13 May 2021 10:35:05 -0400 Tony Krowiak <[email protected]> wrote:
Quoted 77 lines · Halil Pasic
Quoted 39 lines · On Mon, 10 May 2021 17:48:37 -0400 Tony Krowiak <[email protected]> wrote:
The mdev remove callback for the
vfio_apdevice driver bails out with-EBUSYif 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. This could result in a memory leak of the resources allocated when the mdev was created. In addition, the KVM guest will still have access to the AP devices assigned to the mdev even though the mdev no longer exists.To prevent this scenario, cleanup will be done - including unplugging the AP adapters, domains and control domains - regardless of whether the mdev is in use by a KVM guest or not.
Fixes 258287c994de ("s390: vfio-ap: implement mediated device open callback")Signed-off-by Tony Krowiak <[email protected]>--- drivers/s390/crypto/vfio_ap_ops.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)drivers/s390/crypto/vfio_ap_ops.c +1
-4@@ -26,6 +26,7 @@static int vfio_ap_mdev_reset_queues (struct mdev_device *mdev);static struct vfio_ap_queue *vfio_ap_find_queue (int apqn); +static void vfio_ap_mdev_unset_kvm (struct ap_matrix_mdev *matrix_mdev);static int match_apqn (struct device *dev,const void *data) {@@ -366,17 +367,7 @@ 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. drivers/s390/crypto/vfio_ap_ops.c static int vfio_ap_mdev_remove(struct mdev_device *mdev) · line 372
Quoted 10 lines
drivers/s390/crypto/vfio_ap_ops.c +1
-7-*/ -if (matrix_mdev->kvm_busy || matrix_mdev->kvm) { -mutex_unlock (&matrix_dev->lock); -return -EBUSY ; - } - -vfio_ap_mdev_reset_queues (mdev); +vfio_ap_mdev_unset_kvm (matrix_mdev);list_del (&matrix_mdev->node);kfree (matrix_mdev);Are we at risk of
handle_pqap()inarch/s390/kvm/priv.cusing an already freedpqap_hook(which is a member of thematrix_mdevpointee that is freed just above my comment).I'm aware of the fact that
vfio_ap_mdev_unset_kvm()does amatrix_mdev->kvm->arch.crypto.pqap_hook= NULL but that is AFRICT not done under any lock relevant forhandle_pqap(). I guess the idea is, I guess, the check cited belowstatic int
handle_pqap(struct kvm_vcpu *vcpu)
[..]/* * Verify that the hook callback is registered, lock the owner * and call the hook. */ 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); if (!ret && vcpu->run->s.regs.gprs[1] & 0x00ff0000) kvm_s390_set_psw_cc(vcpu, 3); return ret; }Quoted 8 lines
Quoted 2 lines
is going to catch it, but I'm not sure it is guaranteed to catch it. Opinions?
The hook itself -
handle_pqap()function invfio_ap_ops.c- also checks to see if the reference to the hook is set and terminates with an error if it
is not. If the hook is invoked subsequent to the remove callback above, all should be fine since the check is also done under thematrix_dev->lock.Quoted 10 lines
I don't quite understand your logic. Let us assume
matrix_mdevwas freed, butvcpu->kvm->arch.crypto.pqap_hookstill points to what used to be (*matrix_mdev).pqap_hook. In that case the function pointervcpu->kvm->arch.crypto.pqap_hook->hookis used after it was freed, and may not point to thehandle_pqap()function invfio_ap_ops.c, thus the check you are referring to ain't necessarily relevant. Than is if you mean the check in thehandle_pqap()function invfio_ap_ops.c; if you mean the check inhandle_pqap()inarch/s390/kvm/priv.c, that one is not done under thematrix_dev->lock. Or do I have a hole somewhere in my reasoning?Quoted 3 lines
What I am saying is the
vcpu->kvm->arch.crypto.pqap_hookwill either be NULL or point to thehandle_pqap()function in thevfio_apdriver.
Quoted 8 lines
Please read the code again. In my reading of the code
vcpu->kvm->arch.crypto.pqap_hookis never supposed to point to >(or does point to) thehandle_pqap()function defined invfio_ap_ops.c. It points to thepqap_hookmember of structap_matrix_mdev(the type of the member is structkvm_s390_module_hook, which in turn has a function pointer member called hook, which is supposed to hold the address ofhandle_pqap()function defined invfio_ap_ops.c, and thus point to it).
You are correct, we are looking at the same code.
Quoted 2 lines
Because of this, I don't think the rest of your argument is valid.
Okay, so your concern is that between the point in time the vcpu->kvm->arch.crypto.pqap_hook pointer is checked in priv.c and the point in time the handle_pqap() function in vfio_ap_ops.c is called, the memory allocated for the matrix_mdev containing the struct kvm_s390_module_hook may get freed, thus rendering the function pointer invalid. While not impossible, that seems extremely unlikely to happen. Can you articulate a scenario where that could even occur?
Quoted 30 lines
Furthermore I believe we first need to get to common ground on this one before proceeding any further. If you happen to preserve your opinion after checking again, I think we should try to discuss this offline, as one of us is likely looking at the wrong code.
Regards,
HalilQuoted 22 lines
In the latter case, the handler in the driver will get
called and try to acquire thematrix_dev->lock. The function that sets thevcpu->kvm->arch.crypto.pqap_hookto NULL also takes that lock. If the pointer is still active, then the handler will do its thing. If not, then the handler will return without enabling or disabling IRQs. That should not be a problem since theunset_kvmfunction resets the queues which will disable the IRQs.I don't see how
thevcpu->kvm->arch.crypto.pqap_hookcan point to anything other than the handler or be NULL unless KVM is gone. Based on my observations of the behavior, unless there is some other way for the remove callback to be invoked other than in response to a request from userspace via the sysfs remove attribute, it will not get called until the file descriptor is closed in which case the release callback will alsounset_kvm. I think you are worrying about something that will likely never happen.Quoted 3 lines
Regards,
Halil
Halil Pasic
maintainerQuoted 12 lines · On Mon, 17 May 2021 09:37:42 -0400 Tony Krowiak <[email protected]> wrote:
Quoted 2 lines
Because of this, I don't think the rest of your argument is valid.
Okay, so your concern is that between the point in time the
vcpu->kvm->arch.crypto.pqap_hookpointer is checked inpriv.cand the point in time thehandle_pqap()function invfio_ap_ops.cis called, the memory allocated for thematrix_mdevcontaining the structkvm_s390_module_hookmay get freed, thus rendering the function pointer invalid. While not impossible, that seems extremely unlikely to happen. Can you articulate a scenario where that could even occur?
Malicious userspace. We tend to do the pqap aqic just once in the guest right after the queue is detected. I do agree it ain't very likely to happen during normal operation. But why are you asking?
I'm not sure I understood correctly what kind of a scenario are you asking for. PQAP AQIC and mdev remove are independent events originated in userspace, so AFAIK we may not assume that the execution of two won't overlap, nor are we allowed to make assumptions on how does the execution of these two overlap (except for the things we explicitly ensure -- e.g. some parts are made mutually exclusive using the matrix_dev->lock lock).
Regards,
Halil
Quoted 16 lines · Halil Pasic
Quoted 12 lines · On Mon, 17 May 2021 09:37:42 -0400 Tony Krowiak <[email protected]> wrote:
Quoted 2 lines
Because of this, I don't think the rest of your argument is valid.
Okay, so your concern is that between the point in time the
vcpu->kvm->arch.crypto.pqap_hookpointer is checked inpriv.cand the point in time thehandle_pqap()function invfio_ap_ops.cis called, the memory allocated for thematrix_mdevcontaining the structkvm_s390_module_hookmay get freed, thus rendering the function pointer invalid. While not impossible, that seems extremely unlikely to happen. Can you articulate a scenario where that could even occur?
Quoted 4 lines
Malicious userspace. We tend to do the pqap aqic just once in the guest right after the queue is detected. I do agree it ain't very likely to happen during normal operation. But why are you asking?
Would it help, if the code in priv.c would read the hook once and then only work on the copy? We could protect that with rcu and do a synchronize rcu in vfio_ap_mdev_unset_kvm after unsetting the pointer?
Quoted 13 lines
I'm not sure I understood correctly what kind of a scenario are you asking for. PQAP AQIC and mdev remove are independent events originated in userspace, so AFAIK we may not assume that the execution of two won't overlap, nor are we allowed to make assumptions on how does the execution of these two overlap (except for the things we explicitly ensure -- e.g. some parts are made mutually exclusive using the
matrix_dev->locklock).Regards,
Halil
Tony Krowiak
maintainerQuoted 24 lines · Christian Borntraeger
Quoted 16 lines · Halil Pasic
Quoted 12 lines · On Mon, 17 May 2021 09:37:42 -0400 Tony Krowiak <[email protected]> wrote:
Quoted 2 lines
Because of this, I don't think the rest of your argument is valid.
Okay, so your concern is that between the point in time the
vcpu->kvm->arch.crypto.pqap_hookpointer is checked inpriv.cand the point in time thehandle_pqap()function invfio_ap_ops.cis called, the memory allocated for thematrix_mdevcontaining the structkvm_s390_module_hookmay get freed, thus rendering the function pointer invalid. While not impossible, that seems extremely unlikely to happen. Can you articulate a scenario where that could even occur?Quoted 4 lines
Malicious userspace. We tend to do the pqap aqic just once in the guest right after the queue is detected. I do agree it ain't very likely to happen during normal operation. But why are you asking?
Quoted 4 lines
Would it help, if the code in
priv.cwould read the hook once and then only work on the copy? We could protect that with rcu and do a synchronize rcu invfio_ap_mdev_unset_kvmafter unsetting the pointer?
I'll look into this.
Quoted 13 lines
Quoted 13 lines
I'm not sure I understood correctly what kind of a scenario are you asking for. PQAP AQIC and mdev remove are independent events originated in userspace, so AFAIK we may not assume that the execution of two won't overlap, nor are we allowed to make assumptions on how does the execution of these two overlap (except for the things we explicitly ensure -- e.g. some parts are made mutually exclusive using the
matrix_dev->locklock).Regards,
Halil
Quoted 27 lines · Tony Krowiak
Quoted 24 lines · Christian Borntraeger
Quoted 20 lines · Halil Pasic
Quoted 12 lines · On Mon, 17 May 2021 09:37:42 -0400 Tony Krowiak <[email protected]> wrote:
Quoted 2 lines
Because of this, I don't think the rest of your argument is valid.
Okay, so your concern is that between the point in time the
vcpu->kvm->arch.crypto.pqap_hookpointer is checked inpriv.cand the point in time thehandle_pqap()function invfio_ap_ops.cis called, the memory allocated for thematrix_mdevcontaining the structkvm_s390_module_hookmay get freed, thus rendering the function pointer invalid. While not impossible, that seems extremely unlikely to happen. Can you articulate a scenario where that could even occur?Malicious userspace. We tend to do the pqap aqic just once in the guest right after the queue is detected. I do agree it ain't very likely to happen during normal operation. But why are you asking?
Quoted 6 lines
Quoted 4 lines
Would it help, if the code in
priv.cwould read the hook once and then only work on the copy? We could protect that with rcu and do a synchronize rcu invfio_ap_mdev_unset_kvmafter unsetting the pointer?I'll look into this.
I think it could work. in priv.c use rcu_readlock, save the pointer, do the check and call, call rcu_read_unlock. In vfio_ap use rcu_assign_pointer to set the pointer and after setting it to zero call sychronize_rcu.
Halil, I think we can do this as an addon patch as it makes sense to have this callback pointer protected independent of this patch. Agree?
Halil Pasic
maintainerQuoted 28 lines · On Tue, 18 May 2021 15:59:36 +0200 Christian Borntraeger…
Quoted 27 lines · Tony Krowiak
Quoted 24 lines · Christian Borntraeger
Quoted 20 lines · Halil Pasic
Quoted 12 lines · On Mon, 17 May 2021 09:37:42 -0400 Tony Krowiak <[email protected]> wrote:
Quoted 2 lines
Because of this, I don't think the rest of your argument is valid.
Okay, so your concern is that between the point in time the
vcpu->kvm->arch.crypto.pqap_hookpointer is checked inpriv.cand the point in time thehandle_pqap()function invfio_ap_ops.cis called, the memory allocated for thematrix_mdevcontaining the structkvm_s390_module_hookmay get freed, thus rendering the function pointer invalid. While not impossible, that seems extremely unlikely to happen. Can you articulate a scenario where that could even occur?Malicious userspace. We tend to do the pqap aqic just once in the guest right after the queue is detected. I do agree it ain't very likely to happen during normal operation. But why are you asking?
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Would it help, if the code in
priv.cwould read the hook once and then only work on the copy? We could protect that with rcu and do a synchronize rcu invfio_ap_mdev_unset_kvmafter unsetting the pointer?
Unfortunately just "the hook" is ambiguous in this context. We have kvm->arch.crypto.pqap_hook that is supposed to point to a struct kvm_s390_module_hook member of struct ap_matrix_mdev which is also called pqap_hook. And struct kvm_s390_module_hook has function pointer member named "hook".
Quoted 7 lines
Quoted 2 lines
I'll look into this.
I think it could work. in
priv.cusercu_readlock, save the pointer, do the check and call, callrcu_read_unlock. Invfio_apusercu_assign_pointerto set the pointer and after setting it to zero callsychronize_rcu.
In my opinion, we should make the accesses to the kvm->arch.crypto.pqap_hook pointer properly synchronized. I'm not sure if that is what you are proposing. How do we usually do synchronisation on the stuff that lives in kvm->arch?
BTW, something as simple as a cmpxchg which boils down to the CSG instruction for us would suffice in this case (or forcing any interlocked update type construct).
Quoted 4 lines
Halil, I think we can do this as an addon patch as it makes sense to have this callback pointer protected independent of this patch. Agree?
Unfortunately I didn't quite get at the bottom of what exactly gets leaked. My intuition is, that trading a leak for an use after free is in general not a good idea. In this particular case, assuming userspace is well behaved, the use after free is very unlikely,
but then I don't consider the leak to be awfully likely either. A well behaved userspace should not attempt to remove the mdev while it is associated with a guest. We documented that in:Documentation/s390/vfio-ap.rst
"""
remove:
deallocates the mediated matrix device's ap_matrix_mdev structure. This will be allowed only if a running guest is not using the mdev.
"""
BTW this patch should probably change that piece of documentation as well.
In this case, because the leak is much likelier than the use after free (assuming a non-malicious-userspace) the trade may be worth it. Yet my independent opinion is that I would prefer this fixed in one go and properly. But I do trust your judgement better than mine (especially in matters like this). So feel free to go ahead (i.e. I'm not going to NACK this).
Regards,
Halil
Quoted 36 lines · Halil Pasic
Quoted 28 lines · On Tue, 18 May 2021 15:59:36 +0200 Christian Borntraeger…
Quoted 27 lines · Tony Krowiak
Quoted 24 lines · Christian Borntraeger
Quoted 20 lines · Halil Pasic
Quoted 12 lines · On Mon, 17 May 2021 09:37:42 -0400 Tony Krowiak <[email protected]> wrote:
Quoted 2 lines
Because of this, I don't think the rest of your argument is valid.
Okay, so your concern is that between the point in time the
vcpu->kvm->arch.crypto.pqap_hookpointer is checked inpriv.cand the point in time thehandle_pqap()function invfio_ap_ops.cis called, the memory allocated for thematrix_mdevcontaining the structkvm_s390_module_hookmay get freed, thus rendering the function pointer invalid. While not impossible, that seems extremely unlikely to happen. Can you articulate a scenario where that could even occur?Malicious userspace. We tend to do the pqap aqic just once in the guest right after the queue is detected. I do agree it ain't very likely to happen during normal operation. But why are you asking?
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Would it help, if the code in
priv.cwould read the hook once and then only work on the copy? We could protect that with rcu and do a synchronize rcu invfio_ap_mdev_unset_kvmafter unsetting the pointer?
Quoted 5 lines
Unfortunately just "the hook" is ambiguous in this context. We have
kvm->arch.crypto.pqap_hookthat is supposed to point to a structkvm_s390_module_hookmember of structap_matrix_mdevwhich is also calledpqap_hook. And structkvm_s390_module_hookhas function pointer member named "hook".
I was referring to the full struct.
Quoted 9 lines
Quoted 8 lines
Quoted 3 lines
I'll look into this.
I think it could work. in
priv.cusercu_readlock, save the pointer, do the check and call, callrcu_read_unlock. Invfio_apusercu_assign_pointerto set the pointer and after setting it to zero callsychronize_rcu.
Quoted 5 lines
In my opinion, we should make the accesses to the
kvm->arch.crypto.pqap_hookpointer properly synchronized. I'm not sure if that is what you are proposing. How do we usually do synchronisation on the stuff that lives inkvm->arch?
RCU is a method of synchronization. We make sure that structure pqap_hook is still valid as long as we are inside the rcu read lock. So the idea is: clear pointer, wait until all old readers have finished and the proceed with getting rid of the structure.
Halil Pasic
maintainerQuoted 3 lines · On Tue, 18 May 2021 19:01:42 +0200 Christian Borntraeger…
Quoted 2 lines · Halil Pasic
On Tue, 18 May 2021 15:59:36 +0200
Christian Borntraeger <[email protected]> wrote:
[..]
Quoted 28 lines
Quoted 11 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Would it help, if the code in
priv.cwould read the hook once and then only work on the copy? We could protect that with rcu and do a synchronize rcu invfio_ap_mdev_unset_kvmafter unsetting the pointer?Unfortunately just "the hook" is ambiguous in this context. We have
kvm->arch.crypto.pqap_hookthat is supposed to point to a structkvm_s390_module_hookmember of structap_matrix_mdevwhich is also calledpqap_hook. And structkvm_s390_module_hookhas function pointer member named "hook".I was referring to the full struct.
Quoted 9 lines
Quoted 8 lines
Quoted 3 lines
I'll look into this.
I think it could work. in
priv.cusercu_readlock, save the pointer, do the check and call, callrcu_read_unlock. Invfio_apusercu_assign_pointerto set the pointer and after setting it to zero callsychronize_rcu.Quoted 5 lines
In my opinion, we should make the accesses to the
kvm->arch.crypto.pqap_hookpointer properly synchronized. I'm not sure if that is what you are proposing. How do we usually do synchronisation on the stuff that lives inkvm->arch?
Quoted 4 lines
RCU is a method of synchronization. We make sure that structure
pqap_hookis still valid as long as we are inside the rcu read lock. So the idea is: clear pointer, wait until all old readers have finished and the proceed with getting rid of the structure.
Yes I know that RCU is a method of synchronization, but I'm not very familiar with it. I'm a little confused by "read the hook once and then work on a copy". I guess, I would have to read up on the RCU again to get clarity. I intend to brush up my RCU knowledge once the patch comes along. I would be glad to have your help when reviewing an RCU based solution for this.
Regards,
Halil
Quoted 40 lines · Halil Pasic
Quoted 3 lines · On Tue, 18 May 2021 19:01:42 +0200 Christian Borntraeger…
Quoted 2 lines · Halil Pasic
On Tue, 18 May 2021 15:59:36 +0200
Christian Borntraeger <[email protected]> wrote:[..]
Quoted 28 lines
Quoted 11 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Would it help, if the code in
priv.cwould read the hook once and then only work on the copy? We could protect that with rcu and do a synchronize rcu invfio_ap_mdev_unset_kvmafter unsetting the pointer?Unfortunately just "the hook" is ambiguous in this context. We have
kvm->arch.crypto.pqap_hookthat is supposed to point to a structkvm_s390_module_hookmember of structap_matrix_mdevwhich is also calledpqap_hook. And structkvm_s390_module_hookhas function pointer member named "hook".I was referring to the full struct.
Quoted 9 lines
Quoted 8 lines
Quoted 3 lines
I'll look into this.
I think it could work. in
priv.cusercu_readlock, save the pointer, do the check and call, callrcu_read_unlock. Invfio_apusercu_assign_pointerto set the pointer and after setting it to zero callsychronize_rcu.Quoted 5 lines
In my opinion, we should make the accesses to the
kvm->arch.crypto.pqap_hookpointer properly synchronized. I'm not sure if that is what you are proposing. How do we usually do synchronisation on the stuff that lives inkvm->arch?Quoted 4 lines
RCU is a method of synchronization. We make sure that structure
pqap_hookis still valid as long as we are inside the rcu read lock. So the idea is: clear pointer, wait until all old readers have finished and the proceed with getting rid of the structure.
Quoted 6 lines
Yes I know that RCU is a method of synchronization, but I'm not very familiar with it. I'm a little confused by "read the hook once and then work on a copy". I guess, I would have to read up on the RCU again to get clarity. I intend to brush up my RCU knowledge once the patch comes along. I would be glad to have your help when reviewing an RCU based solution for this.
Just had a quick look. Its not trivial, as the hook function itself takes a mutex and an rcu section must not sleep. Will have a deeper look.
Quoted 48 lines · Christian Borntraeger
Quoted 38 lines · Halil Pasic
Quoted 3 lines · On Tue, 18 May 2021 19:01:42 +0200 Christian Borntraeger…
Quoted 2 lines · Halil Pasic
On Tue, 18 May 2021 15:59:36 +0200
Christian Borntraeger <[email protected]> wrote:[..]
Quoted 26 lines
Quoted 11 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Would it help, if the code in
priv.cwould read the hook once and then only work on the copy? We could protect that with rcu and do a synchronize rcu invfio_ap_mdev_unset_kvmafter unsetting the pointer?Unfortunately just "the hook" is ambiguous in this context. We have
kvm->arch.crypto.pqap_hookthat is supposed to point to a structkvm_s390_module_hookmember of structap_matrix_mdevwhich is also calledpqap_hook. And structkvm_s390_module_hookhas function pointer member named "hook".I was referring to the full struct.
Quoted 8 lines
Quoted 7 lines
Quoted 2 lines
I'll look into this.
I think it could work. in
priv.cusercu_readlock, save the pointer, do the check and call, callrcu_read_unlock. Invfio_apusercu_assign_pointerto set the pointer and after setting it to zero callsychronize_rcu.Quoted 4 lines
In my opinion, we should make the accesses to the
kvm->arch.crypto.pqap_hookpointer properly synchronized. I'm not sure if that is what you are proposing. How do we usually do synchronisation on the stuff that lives inkvm->arch?Quoted 4 lines
RCU is a method of synchronization. We make sure that structure
pqap_hookis still valid as long as we are inside the rcu read lock. So the idea is: clear pointer, wait until all old readers have finished and the proceed with getting rid of the structure.Quoted 6 lines
Yes I know that RCU is a method of synchronization, but I'm not very familiar with it. I'm a little confused by "read the hook once and then work on a copy". I guess, I would have to read up on the RCU again to get clarity. I intend to brush up my RCU knowledge once the patch comes along. I would be glad to have your help when reviewing an RCU based solution for this.
Quoted 3 lines
Just had a quick look. Its not trivial, as the hook function itself takes a mutex and an rcu section must not sleep. Will have a deeper look.
As a quick hack something like this could work. The whole locking is pretty complicated and this makes it even more complex so we might want to do a cleanup/locking rework later on.
index 9928f785c677..fde6e02aab54 100644
arch/s390/kvm/priv.c +12 -4
@@ -609,6 +609,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)*/ static int handle_pqap (struct kvm_vcpu *vcpu) { +struct kvm_s390_module_hook *pqap_hook;struct ap_queue_status status = {};unsigned long reg0;int ret;
@@ -657,14 +658,21 @@ static int handle_pqap(struct kvm_vcpu *vcpu)* Verify that the hook callback is registered, lock the owner * and call the hook. */ -if (vcpu->kvm->arch.crypto.pqap_hook) { -if (!try_module_get (vcpu->kvm->arch.crypto.pqap_hook->owner)) +rcu_read_lock (); + pqap_hook =rcu_dereference (vcpu->kvm->arch.crypto.pqap_hook); +if (pqap_hook) { +if (!try_module_get (pqap_hook->owner)) { +rcu_read_unlock ();return -EOPNOTSUPP ; - ret = vcpu->kvm->arch.crypto.pqap_hook->hook (vcpu); -module_put (vcpu->kvm->arch.crypto.pqap_hook->owner); + } +rcu_read_unlock (); + ret = pqap_hook->hook (vcpu); +module_put (pqap_hook->owner);if (!ret && vcpu->run->s.regs.gprs[1 ] &0x00ff0000 )kvm_s390_set_psw_cc (vcpu,3 );return ret; + }else { +rcu_read_unlock (); }/* * A vfio_driver must register a hook.
drivers/s390/crypto/vfio_ap_ops.c +1 -0
@@ -1194,6 +1194,7 @@ static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)mutex_lock (&matrix_dev->lock);vfio_ap_mdev_reset_queues (matrix_mdev->mdev); matrix_mdev->kvm->arch.crypto.pqap_hook =NULL ; +synchronize_rcu ();kvm_put_kvm (matrix_mdev->kvm); matrix_mdev->kvm =NULL ; matrix_mdev->kvm_busy = false;
Halil Pasic
maintainerQuoted 49 lines · On Wed, 19 May 2021 13:22:56 +0200 Christian Borntraeger…
Quoted 48 lines · Christian Borntraeger
Quoted 44 lines · Halil Pasic
Quoted 3 lines · On Tue, 18 May 2021 19:01:42 +0200 Christian Borntraeger…
Quoted 2 lines · Halil Pasic
On Tue, 18 May 2021 15:59:36 +0200
Christian Borntraeger <[email protected]> wrote:[..]
Quoted 26 lines
Quoted 11 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Would it help, if the code in
priv.cwould read the hook once and then only work on the copy? We could protect that with rcu and do a synchronize rcu invfio_ap_mdev_unset_kvmafter unsetting the pointer?Unfortunately just "the hook" is ambiguous in this context. We have
kvm->arch.crypto.pqap_hookthat is supposed to point to a structkvm_s390_module_hookmember of structap_matrix_mdevwhich is also calledpqap_hook. And structkvm_s390_module_hookhas function pointer member named "hook".I was referring to the full struct.
Quoted 8 lines
Quoted 7 lines
Quoted 2 lines
I'll look into this.
I think it could work. in
priv.cusercu_readlock, save the pointer, do the check and call, callrcu_read_unlock. Invfio_apusercu_assign_pointerto set the pointer and after setting it to zero callsychronize_rcu.Quoted 4 lines
In my opinion, we should make the accesses to the
kvm->arch.crypto.pqap_hookpointer properly synchronized. I'm not sure if that is what you are proposing. How do we usually do synchronisation on the stuff that lives inkvm->arch?Quoted 4 lines
RCU is a method of synchronization. We make sure that structure
pqap_hookis still valid as long as we are inside the rcu read lock. So the idea is: clear pointer, wait until all old readers have finished and the proceed with getting rid of the structure.Yes I know that RCU is a method of synchronization, but I'm not very familiar with it. I'm a little confused by "read the hook once and then work on a copy". I guess, I would have to read up on the RCU again to get clarity. I intend to brush up my RCU knowledge once the patch comes along. I would be glad to have your help when reviewing an RCU based solution for this.
Quoted 9 lines
Quoted 3 lines
Just had a quick look. Its not trivial, as the hook function itself takes a mutex and an rcu section must not sleep. Will have a deeper look.
As a quick hack something like this could work. The whole locking is pretty complicated and this makes it even more complex so we might want to do a cleanup/locking rework later on.
Hm, seems our emails crossed mid air...
Quoted 17 lines
index
9928f785c677..fde6e02aab54100644arch/s390/kvm/priv.c +1
-1@@ -609,6 +609,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)*/ static int handle_pqap (struct kvm_vcpu *vcpu) { +struct kvm_s390_module_hook *pqap_hook;struct ap_queue_status status = {};unsigned long reg0;int ret;@@ -657,14 +658,21 @@ static int handle_pqap(struct kvm_vcpu *vcpu)* Verify that the hook callback is registered, lock the owner * and call the hook. */ -if (vcpu->kvm->arch.crypto.pqap_hook) {
arch/s390/kvm/priv.c static int handle_pqap(struct kvm_vcpu *vcpu) · line 667
Quoted 10 lines
arch/s390/kvm/priv.c +6
-3-if (!try_module_get (vcpu->kvm->arch.crypto.pqap_hook->owner)) +rcu_read_lock (); + pqap_hook =rcu_dereference (vcpu->kvm->arch.crypto.pqap_hook); +if (pqap_hook) { +if (!try_module_get (pqap_hook->owner)) { +rcu_read_unlock ();return -EOPNOTSUPP ; - ret = vcpu->kvm->arch.crypto.pqap_hook->hook (vcpu); -module_put (vcpu->kvm->arch.crypto.pqap_hook->owner); + }
Up to this point the local pqap_hook is guaranteed to point to a valid object if not NULL, ...
arch/s390/kvm/priv.c static int handle_pqap(struct kvm_vcpu *vcpu) · line 668
Quoted 1 line
arch/s390/kvm/priv.c +1
-0+rcu_read_unlock ();
... and after this point IMHO it is not.
arch/s390/kvm/priv.c static int handle_pqap(struct kvm_vcpu *vcpu) · line 669
Quoted 1 line
arch/s390/kvm/priv.c +1
-0+ ret = pqap_hook->hook (vcpu);
So IMHO the pointer deference here is still problematic, but that can be fixed easily as I described in that email I've sent 3 minutes after yours. IMHO we need a local copy of cpu->kvm->arch.crypto.pqap_hook->hook taken within the rcu read critical section. Do you agree?
Regards,
Halil
Quoted 21 lines
arch/s390/kvm/priv.c +3
-0+module_put (pqap_hook->owner);if (!ret && vcpu->run->s.regs.gprs[1 ] &0x00ff0000 )kvm_s390_set_psw_cc (vcpu,3 );return ret; + }else { +rcu_read_unlock (); }/* * A vfio_driver must register a hook. drivers/s390/crypto/vfio_ap_ops.c +1
-0@@ -1194,6 +1194,7 @@ static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)mutex_lock (&matrix_dev->lock);vfio_ap_mdev_reset_queues (matrix_mdev->mdev); matrix_mdev->kvm->arch.crypto.pqap_hook =NULL ; +synchronize_rcu ();kvm_put_kvm (matrix_mdev->kvm); matrix_mdev->kvm =NULL ; matrix_mdev->kvm_busy = false;
Quoted 72 lines · Christian Borntraeger
Quoted 48 lines · Christian Borntraeger
Quoted 38 lines · Halil Pasic
Quoted 3 lines · On Tue, 18 May 2021 19:01:42 +0200 Christian Borntraeger…
Quoted 2 lines · Halil Pasic
On Tue, 18 May 2021 15:59:36 +0200
Christian Borntraeger <[email protected]> wrote:[..]
Quoted 26 lines
Quoted 11 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Would it help, if the code in
priv.cwould read the hook once and then only work on the copy? We could protect that with rcu and do a synchronize rcu invfio_ap_mdev_unset_kvmafter unsetting the pointer?Unfortunately just "the hook" is ambiguous in this context. We have
kvm->arch.crypto.pqap_hookthat is supposed to point to a structkvm_s390_module_hookmember of structap_matrix_mdevwhich is also calledpqap_hook. And structkvm_s390_module_hookhas function pointer member named "hook".I was referring to the full struct.
Quoted 8 lines
Quoted 7 lines
Quoted 2 lines
I'll look into this.
I think it could work. in
priv.cusercu_readlock, save the pointer, do the check and call, callrcu_read_unlock. Invfio_apusercu_assign_pointerto set the pointer and after setting it to zero callsychronize_rcu.Quoted 4 lines
In my opinion, we should make the accesses to the
kvm->arch.crypto.pqap_hookpointer properly synchronized. I'm not sure if that is what you are proposing. How do we usually do synchronisation on the stuff that lives inkvm->arch?Quoted 4 lines
RCU is a method of synchronization. We make sure that structure
pqap_hookis still valid as long as we are inside the rcu read lock. So the idea is: clear pointer, wait until all old readers have finished and the proceed with getting rid of the structure.Quoted 6 lines
Yes I know that RCU is a method of synchronization, but I'm not very familiar with it. I'm a little confused by "read the hook once and then work on a copy". I guess, I would have to read up on the RCU again to get clarity. I intend to brush up my RCU knowledge once the patch comes along. I would be glad to have your help when reviewing an RCU based solution for this.
Quoted 3 lines
Just had a quick look. Its not trivial, as the hook function itself takes a mutex and an rcu section must not sleep. Will have a deeper look.
As a quick hack something like this could work. The whole locking is pretty complicated and this makes it even more complex so we might want to do a cleanup/locking rework later on.
index
9928f785c677..fde6e02aab54100644
+++b/arch/s390/kvm/priv.carch/s390/kvm/priv.c +1
-0@@ -609,6 +609,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)*/ static int handle_pqap (struct kvm_vcpu *vcpu) { +struct kvm_s390_module_hook *pqap_hook;struct ap_queue_status status = {};unsigned long reg0;int ret;
arch/s390/kvm/priv.c static int handle_pqap(struct kvm_vcpu *vcpu) · line 664
Quoted 9 lines
arch/s390/kvm/priv.c +4
-2@@ -657,14 +658,21 @@ static int handle_pqap(struct kvm_vcpu *vcpu)* Verify that the hook callback is registered, lock the owner * and call the hook. */ -if (vcpu->kvm->arch.crypto.pqap_hook) { -if (!try_module_get (vcpu->kvm->arch.crypto.pqap_hook->owner)) +rcu_read_lock (); + pqap_hook =rcu_dereference (vcpu->kvm->arch.crypto.pqap_hook); +if (pqap_hook) { +if (!try_module_get (pqap_hook->owner)) {
module locking doesn't prevent driver unbinding
arch/s390/kvm/priv.c static int handle_pqap(struct kvm_vcpu *vcpu) · line 669
Quoted 7 lines
arch/s390/kvm/priv.c +4
-2+rcu_read_unlock ();return -EOPNOTSUPP ; - ret = vcpu->kvm->arch.crypto.pqap_hook->hook (vcpu); -module_put (vcpu->kvm->arch.crypto.pqap_hook->owner); + } +rcu_read_unlock (); + ret = pqap_hook->hook (vcpu);
So taking the pointer out of the rcu still isn't protected.
Unless this is super performance critical just use a rw sem
Jason
Halil Pasic
maintainerQuoted 48 lines · On Wed, 19 May 2021 10:17:49 +0200 Christian Borntraeger…
Quoted 40 lines · Halil Pasic
Quoted 3 lines · On Tue, 18 May 2021 19:01:42 +0200 Christian Borntraeger…
Quoted 2 lines · Halil Pasic
On Tue, 18 May 2021 15:59:36 +0200
Christian Borntraeger <[email protected]> wrote:[..]
Quoted 28 lines
Quoted 11 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Would it help, if the code in
priv.cwould read the hook once and then only work on the copy? We could protect that with rcu and do a synchronize rcu invfio_ap_mdev_unset_kvmafter unsetting the pointer?Unfortunately just "the hook" is ambiguous in this context. We have
kvm->arch.crypto.pqap_hookthat is supposed to point to a structkvm_s390_module_hookmember of structap_matrix_mdevwhich is also calledpqap_hook. And structkvm_s390_module_hookhas function pointer member named "hook".I was referring to the full struct.
Quoted 9 lines
Quoted 8 lines
Quoted 3 lines
I'll look into this.
I think it could work. in
priv.cusercu_readlock, save the pointer, do the check and call, callrcu_read_unlock. Invfio_apusercu_assign_pointerto set the pointer and after setting it to zero callsychronize_rcu.Quoted 5 lines
In my opinion, we should make the accesses to the
kvm->arch.crypto.pqap_hookpointer properly synchronized. I'm not sure if that is what you are proposing. How do we usually do synchronisation on the stuff that lives inkvm->arch?Quoted 4 lines
RCU is a method of synchronization. We make sure that structure
pqap_hookis still valid as long as we are inside the rcu read lock. So the idea is: clear pointer, wait until all old readers have finished and the proceed with getting rid of the structure.Quoted 6 lines
Yes I know that RCU is a method of synchronization, but I'm not very familiar with it. I'm a little confused by "read the hook once and then work on a copy". I guess, I would have to read up on the RCU again to get clarity. I intend to brush up my RCU knowledge once the patch comes along. I would be glad to have your help when reviewing an RCU based solution for this.
Quoted 3 lines
Just had a quick look. Its not trivial, as the hook function itself takes a mutex and an rcu section must not sleep. Will have a deeper look.
I refreshed my RCU knowledge and RCU seems to be a reasonable choice here. I don't think we have to make the rcu read section span the call to the callback. That is something like
arch/s390/kvm/priv.c +10 -2
@@ -613,6 +613,7 @@ static int handle_pqap(struct kvm_vcpu *vcpu)unsigned long reg0;int ret;uint8_t fc; +int (*pqap_hook)(struct kvm_vcpu *vcpu);/* Verify that the AP instruction are available */ if (!ap_instructions_available ())
@@ -657,14 +658,21 @@ static int handle_pqap(struct kvm_vcpu *vcpu)* Verify that the hook callback is registered, lock the owner * and call the hook. */ +rcu_read_lock ();if (vcpu->kvm->arch.crypto.pqap_hook) { -if (!try_module_get (vcpu->kvm->arch.crypto.pqap_hook->owner)) +if (!try_module_get (vcpu->kvm->arch.crypto.pqap_hook->owner)) { +rcu_read_unlock ();return -EOPNOTSUPP ; - ret = vcpu->kvm->arch.crypto.pqap_hook->hook (vcpu); + } + pqap_hook =READ_ONCE (vcpu->kvm->arch.crypto.pqap_hook->hook); +rcu_read_unlock (); + ret =pqap_hook ();module_put (vcpu->kvm->arch.crypto.pqap_hook->owner);if (!ret && vcpu->run->s.regs.gprs[1 ] &0x00ff0000 )kvm_s390_set_psw_cc (vcpu,3 );return ret; + }else { +rcu_read_unlock (); }/* * A vfio_driver must register a hook.
Should be sufficient. The module get ensures that the pointee is still around for the duration of the call. The handle_pqap() from vfio_ap_ops.c checks the vcpu->kvm->arch.crypto.pqap_hook the same lock that is used to set it to NULL, and bails out if it is NULL. It is a bit convoluted, but it should work.
Regards,
Halil
Tony Krowiak
maintainerQuoted 37 lines · Christian Borntraeger
Quoted 27 lines · Tony Krowiak
Quoted 24 lines · Christian Borntraeger
Quoted 20 lines · Halil Pasic
Quoted 12 lines · On Mon, 17 May 2021 09:37:42 -0400 Tony Krowiak <[email protected]> wrote:
Quoted 2 lines
Because of this, I don't think the rest of your argument is valid.
Okay, so your concern is that between the point in time the
vcpu->kvm->arch.crypto.pqap_hookpointer is checked inpriv.cand the point in time thehandle_pqap()function invfio_ap_ops.cis called, the memory allocated for thematrix_mdevcontaining the structkvm_s390_module_hookmay get freed, thus rendering the function pointer invalid. While not impossible, that seems extremely unlikely to happen. Can you articulate a scenario where that could even occur?Malicious userspace. We tend to do the pqap aqic just once in the guest right after the queue is detected. I do agree it ain't very likely to happen during normal operation. But why are you asking?
Quoted 6 lines
Quoted 4 lines
Would it help, if the code in
priv.cwould read the hook once and then only work on the copy? We could protect that with rcu and do a synchronize rcu invfio_ap_mdev_unset_kvmafter unsetting the pointer?I'll look into this.
Quoted 8 lines
I think it could work. in
priv.cusercu_readlock, save the pointer, do the check and call, callrcu_read_unlock. Invfio_apusercu_assign_pointerto set the pointer and after setting it to zero callsychronize_rcu.Halil, I think we can do this as an addon patch as it makes sense to have this callback pointer protected independent of this patch. Agree?
I agree that this is a viable option; however, this does not guarantee that the matrix_mdev is not freed thus rendering the function pointer to the interception handler invalid unless that is also included within the rcu_readlock/rcu_read_unlock. That is not possible given the matrix_mdev is freed within the remove callback and the pointer to the structure that contains the interception handler function pointer is cleared in the vfio_ap_mdev_unset_kvm() function. I am working on a patch and should be able to post it before EOD or first thing tomorrow.
Quoted 45 lines · Tony Krowiak
Quoted 42 lines · Christian Borntraeger
Quoted 27 lines · Tony Krowiak
Quoted 24 lines · Christian Borntraeger
Quoted 20 lines · Halil Pasic
Quoted 12 lines · On Mon, 17 May 2021 09:37:42 -0400 Tony Krowiak <[email protected]> wrote:
Quoted 2 lines
Because of this, I don't think the rest of your argument is valid.
Okay, so your concern is that between the point in time the
vcpu->kvm->arch.crypto.pqap_hookpointer is checked inpriv.cand the point in time thehandle_pqap()function invfio_ap_ops.cis called, the memory allocated for thematrix_mdevcontaining the structkvm_s390_module_hookmay get freed, thus rendering the function pointer invalid. While not impossible, that seems extremely unlikely to happen. Can you articulate a scenario where that could even occur?Malicious userspace. We tend to do the pqap aqic just once in the guest right after the queue is detected. I do agree it ain't very likely to happen during normal operation. But why are you asking?
Quoted 6 lines
Quoted 4 lines
Would it help, if the code in
priv.cwould read the hook once and then only work on the copy? We could protect that with rcu and do a synchronize rcu invfio_ap_mdev_unset_kvmafter unsetting the pointer?I'll look into this.
I think it could work. in
priv.cusercu_readlock, save the pointer, do the check and call, callrcu_read_unlock. Invfio_apusercu_assign_pointerto set the pointer and after setting it to zero callsychronize_rcu.
Quoted 8 lines
Quoted 3 lines
Halil, I think we can do this as an addon patch as it makes sense to have this callback pointer protected independent of this patch. Agree?
I agree that this is a viable option; however, this does not guarantee that the
matrix_mdevis not freed thus rendering the function pointer to the interception handler invalid unless that is also included within the rcu_readlock/rcu_read_unlock.
The trick should be the sychronize_rcu. This will put the deleting code (vfio_ap_mdev_unset_kvm) to sleep until the rcu read section has finished. So if you first set the pointer to zero, then call synchronize_rcu the code will only progress until all users of the old poiner have finished.
Quoted 7 lines
That is not possible given the
matrix_mdevis freed within the remove callback and the pointer to the structure that contains the interception handler function pointer is cleared in thevfio_ap_mdev_unset_kvm()function. I am working on a patch and should be able to post it before EOD or first thing tomorrow.
Tony Krowiak
maintainerQuoted 57 lines · Christian Borntraeger
Quoted 45 lines · Tony Krowiak
Quoted 42 lines · Christian Borntraeger
Quoted 27 lines · Tony Krowiak
Quoted 24 lines · Christian Borntraeger
Quoted 20 lines · Halil Pasic
Quoted 12 lines · On Mon, 17 May 2021 09:37:42 -0400 Tony Krowiak <[email protected]> wrote:
Quoted 2 lines
Because of this, I don't think the rest of your argument is valid.
Okay, so your concern is that between the point in time the
vcpu->kvm->arch.crypto.pqap_hookpointer is checked inpriv.cand the point in time thehandle_pqap()function invfio_ap_ops.cis called, the memory allocated for thematrix_mdevcontaining the structkvm_s390_module_hookmay get freed, thus rendering the function pointer invalid. While not impossible, that seems extremely unlikely to happen. Can you articulate a scenario where that could even occur?Malicious userspace. We tend to do the pqap aqic just once in the guest right after the queue is detected. I do agree it ain't very likely to happen during normal operation. But why are you asking?
Quoted 6 lines
Quoted 4 lines
Would it help, if the code in
priv.cwould read the hook once and then only work on the copy? We could protect that with rcu and do a synchronize rcu invfio_ap_mdev_unset_kvmafter unsetting the pointer?I'll look into this.
I think it could work. in
priv.cusercu_readlock, save the pointer, do the check and call, callrcu_read_unlock. Invfio_apusercu_assign_pointerto set the pointer and after setting it to zero callsychronize_rcu.Quoted 8 lines
Quoted 3 lines
Halil, I think we can do this as an addon patch as it makes sense to have this callback pointer protected independent of this patch. Agree?
I agree that this is a viable option; however, this does not guarantee that the
matrix_mdevis not freed thus rendering the function pointer to the interception handler invalid unless that is also included within the rcu_readlock/rcu_read_unlock.
Quoted 5 lines
The trick should be the
sychronize_rcu. This will put the deleting code (vfio_ap_mdev_unset_kvm) to sleep until the rcu read section has finished. So if you first set the pointer to zero, then callsynchronize_rcuthe code will only progress until all users of the old poiner have finished.
Yes, that is my understanding too.
Quoted 8 lines
Quoted 8 lines
That is not possible given the
matrix_mdevis freed within the remove callback and the pointer to the structure that contains the interception handler function pointer is cleared in thevfio_ap_mdev_unset_kvm()function. I am working on a patch and should be able to post it before EOD or first thing tomorrow.
Tony Krowiak
maintainerQuoted 13 lines · Halil Pasic
Quoted 10 lines · On Mon, 17 May 2021 09:37:42 -0400 Tony Krowiak <[email protected]> wrote:
Quoted 1 line
Because of this, I don't think the rest of your argument is valid.
Okay, so your concern is that between the point in time the
vcpu->kvm->arch.crypto.pqap_hookpointer is checked inpriv.cand the point in time thehandle_pqap()function invfio_ap_ops.cis called, the memory allocated for thematrix_mdevcontaining the structkvm_s390_module_hookmay get freed, thus rendering the function pointer invalid. While not impossible, that seems extremely unlikely to happen. Can you articulate a scenario where that could even occur?
Quoted 4 lines
Malicious userspace. We tend to do the pqap aqic just once in the guest right after the queue is detected. I do agree it ain't very likely to happen during normal operation. But why are you asking?
I'm just trying to wrap my head around how this can happen given the incredibly small window between access to the pointer to the structure containing the function pointer and access to the function pointer itself.
Quoted 9 lines
I'm not sure I understood correctly what kind of a scenario are you asking for. PQAP AQIC and mdev remove are independent events originated in userspace, so AFAIK we may not assume that the execution of two won't overlap, nor are we allowed to make assumptions on how does the execution of these two overlap (except for the things we explicitly ensure -- e.g. some parts are made mutually exclusive using the
matrix_dev->locklock).
It looks like we need a way to control access to the struct kvm_s390_module_hook. I'm looking into Christian's suggestion for using RCU as well as other solutions.
Quoted 4 lines
Regards,
Halil