The vfio_ap device driver registers a group notifier with VFIO when the
file descriptor for a VFIO mediated device for a KVM guest is opened to
receive notification that the KVM pointer is set (VFIO_GROUP_NOTIFY_SET_KVM
event). When the KVM pointer is set, the vfio_ap driver takes the
following actions:
1. Stashes the KVM pointer in the vfio_ap_mdev struct that holds the state
of the mediated device.
2. Calls the kvm_get_kvm() function to increment its reference counter.
3. Sets the function pointer to the function that handles interception of
the instruction that enables/disables interrupt processing.
4. Sets the masks in the KVM guest's CRYCB to pass AP resources through to
the guest.
In order to avoid memory leaks, when the notifier is called to receive
notification that the KVM pointer has been set to NULL, the vfio_ap device
driver should reverse the actions taken when the KVM pointer was set.
Fixes: 258287c994de ("s390: vfio-ap: implement mediated device open callback")
Cc: [email protected]
Signed-off-by: Tony Krowiak <[email protected]>
Reviewed-by: Halil Pasic <[email protected]>
Reviewed-by: Cornelia Huck <[email protected]>
---
drivers/s390/crypto/vfio_ap_ops.c | 42 +++++++++++++++++--------------
1 file changed, 23 insertions(+), 19 deletions(-)
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index e0bde8518745..44f3378540d5 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1037,19 +1037,14 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
{
struct ap_matrix_mdev *m;
- mutex_lock(&matrix_dev->lock);
-
list_for_each_entry(m, &matrix_dev->mdev_list, node) {
- if ((m != matrix_mdev) && (m->kvm == kvm)) {
- mutex_unlock(&matrix_dev->lock);
+ if ((m != matrix_mdev) && (m->kvm == kvm))
return -EPERM;
- }
}
matrix_mdev->kvm = kvm;
kvm_get_kvm(kvm);
kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook;
- mutex_unlock(&matrix_dev->lock);
return 0;
}
@@ -1083,35 +1078,49 @@ static int vfio_ap_mdev_iommu_notifier(struct notifier_block *nb,
return NOTIFY_DONE;
}
+static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
+{
+ kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
+ matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
+ vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
+ kvm_put_kvm(matrix_mdev->kvm);
+ matrix_mdev->kvm = NULL;
+}
+
static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
unsigned long action, void *data)
{
- int ret;
+ int ret, notify_rc = NOTIFY_DONE;
struct ap_matrix_mdev *matrix_mdev;
if (action != VFIO_GROUP_NOTIFY_SET_KVM)
return NOTIFY_OK;
matrix_mdev = container_of(nb, struct ap_matrix_mdev, group_notifier);
+ mutex_lock(&matrix_dev->lock);
if (!data) {
- matrix_mdev->kvm = NULL;
- return NOTIFY_OK;
+ if (matrix_mdev->kvm)
+ vfio_ap_mdev_unset_kvm(matrix_mdev);
+ notify_rc = NOTIFY_OK;
+ goto notify_done;
}
ret = vfio_ap_mdev_set_kvm(matrix_mdev, data);
if (ret)
- return NOTIFY_DONE;
+ goto notify_done;
/* If there is no CRYCB pointer, then we can't copy the masks */
if (!matrix_mdev->kvm->arch.crypto.crycbd)
- return NOTIFY_DONE;
+ goto notify_done;
kvm_arch_crypto_set_masks(matrix_mdev->kvm, matrix_mdev->matrix.apm,
matrix_mdev->matrix.aqm,
matrix_mdev->matrix.adm);
- return NOTIFY_OK;
+notify_done:
+ mutex_unlock(&matrix_dev->lock);
+ return notify_rc;
}
static void vfio_ap_irq_disable_apqn(int apqn)
@@ -1222,13 +1231,8 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev)
struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
mutex_lock(&matrix_dev->lock);
- if (matrix_mdev->kvm) {
- kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
- matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
- vfio_ap_mdev_reset_queues(mdev);
- kvm_put_kvm(matrix_mdev->kvm);
- matrix_mdev->kvm = NULL;
- }
+ if (matrix_mdev->kvm)
+ vfio_ap_mdev_unset_kvm(matrix_mdev);
mutex_unlock(&matrix_dev->lock);
vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
--
2.21.1
On Mon, 21 Dec 2020 13:56:25 -0500
Tony Krowiak <[email protected]> wrote:
> The vfio_ap device driver registers a group notifier with VFIO when the
> file descriptor for a VFIO mediated device for a KVM guest is opened to
> receive notification that the KVM pointer is set (VFIO_GROUP_NOTIFY_SET_KVM
> event). When the KVM pointer is set, the vfio_ap driver takes the
> following actions:
> 1. Stashes the KVM pointer in the vfio_ap_mdev struct that holds the state
> of the mediated device.
> 2. Calls the kvm_get_kvm() function to increment its reference counter.
> 3. Sets the function pointer to the function that handles interception of
> the instruction that enables/disables interrupt processing.
> 4. Sets the masks in the KVM guest's CRYCB to pass AP resources through to
> the guest.
>
> In order to avoid memory leaks, when the notifier is called to receive
> notification that the KVM pointer has been set to NULL, the vfio_ap device
> driver should reverse the actions taken when the KVM pointer was set.
>
> Fixes: 258287c994de ("s390: vfio-ap: implement mediated device open callback")
> Cc: [email protected]
> Signed-off-by: Tony Krowiak <[email protected]>
> Reviewed-by: Halil Pasic <[email protected]>
> Reviewed-by: Cornelia Huck <[email protected]>
[..]
> static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
> unsigned long action, void *data)
> {
> - int ret;
> + int ret, notify_rc = NOTIFY_DONE;
> struct ap_matrix_mdev *matrix_mdev;
>
> if (action != VFIO_GROUP_NOTIFY_SET_KVM)
> return NOTIFY_OK;
>
> matrix_mdev = container_of(nb, struct ap_matrix_mdev, group_notifier);
> + mutex_lock(&matrix_dev->lock);
>
> if (!data) {
> - matrix_mdev->kvm = NULL;
> - return NOTIFY_OK;
> + if (matrix_mdev->kvm)
> + vfio_ap_mdev_unset_kvm(matrix_mdev);
> + notify_rc = NOTIFY_OK;
> + goto notify_done;
> }
>
> ret = vfio_ap_mdev_set_kvm(matrix_mdev, data);
> if (ret)
> - return NOTIFY_DONE;
> + goto notify_done;
>
> /* If there is no CRYCB pointer, then we can't copy the masks */
> if (!matrix_mdev->kvm->arch.crypto.crycbd)
> - return NOTIFY_DONE;
> + goto notify_done;
>
> kvm_arch_crypto_set_masks(matrix_mdev->kvm, matrix_mdev->matrix.apm,
> matrix_mdev->matrix.aqm,
> matrix_mdev->matrix.adm);
>
> - return NOTIFY_OK;
Shouldn't there be an
+ notify_rc = NOTIFY_OK;
here? I mean you initialize notify_rc to NOTIFY_DONE, in the !data branch
on success you set notify_rc to NOTIFY_OK, but in the !!data branch it
just stays NOTIFY_DONE. Or am I missing something?
Otherwise LGTM!
Regards,
Halil
> +notify_done:
> + mutex_unlock(&matrix_dev->lock);
> + return notify_rc;
> }
>
[..]
On 12/21/20 11:05 PM, Halil Pasic wrote:
> On Mon, 21 Dec 2020 13:56:25 -0500
> Tony Krowiak <[email protected]> wrote:
>
>> The vfio_ap device driver registers a group notifier with VFIO when the
>> file descriptor for a VFIO mediated device for a KVM guest is opened to
>> receive notification that the KVM pointer is set (VFIO_GROUP_NOTIFY_SET_KVM
>> event). When the KVM pointer is set, the vfio_ap driver takes the
>> following actions:
>> 1. Stashes the KVM pointer in the vfio_ap_mdev struct that holds the state
>> of the mediated device.
>> 2. Calls the kvm_get_kvm() function to increment its reference counter.
>> 3. Sets the function pointer to the function that handles interception of
>> the instruction that enables/disables interrupt processing.
>> 4. Sets the masks in the KVM guest's CRYCB to pass AP resources through to
>> the guest.
>>
>> In order to avoid memory leaks, when the notifier is called to receive
>> notification that the KVM pointer has been set to NULL, the vfio_ap device
>> driver should reverse the actions taken when the KVM pointer was set.
>>
>> Fixes: 258287c994de ("s390: vfio-ap: implement mediated device open callback")
>> Cc: [email protected]
>> Signed-off-by: Tony Krowiak <[email protected]>
>> Reviewed-by: Halil Pasic <[email protected]>
>> Reviewed-by: Cornelia Huck <[email protected]>
> [..]
>
>> static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
>> unsigned long action, void *data)
>> {
>> - int ret;
>> + int ret, notify_rc = NOTIFY_DONE;
>> struct ap_matrix_mdev *matrix_mdev;
>>
>> if (action != VFIO_GROUP_NOTIFY_SET_KVM)
>> return NOTIFY_OK;
>>
>> matrix_mdev = container_of(nb, struct ap_matrix_mdev, group_notifier);
>> + mutex_lock(&matrix_dev->lock);
>>
>> if (!data) {
>> - matrix_mdev->kvm = NULL;
>> - return NOTIFY_OK;
>> + if (matrix_mdev->kvm)
>> + vfio_ap_mdev_unset_kvm(matrix_mdev);
>> + notify_rc = NOTIFY_OK;
>> + goto notify_done;
>> }
>>
>> ret = vfio_ap_mdev_set_kvm(matrix_mdev, data);
>> if (ret)
>> - return NOTIFY_DONE;
>> + goto notify_done;
>>
>> /* If there is no CRYCB pointer, then we can't copy the masks */
>> if (!matrix_mdev->kvm->arch.crypto.crycbd)
>> - return NOTIFY_DONE;
>> + goto notify_done;
>>
>> kvm_arch_crypto_set_masks(matrix_mdev->kvm, matrix_mdev->matrix.apm,
>> matrix_mdev->matrix.aqm,
>> matrix_mdev->matrix.adm);
>>
>> - return NOTIFY_OK;
> Shouldn't there be an
> + notify_rc = NOTIFY_OK;
> here? I mean you initialize notify_rc to NOTIFY_DONE, in the !data branch
> on success you set notify_rc to NOTIFY_OK, but in the !!data branch it
> just stays NOTIFY_DONE. Or am I missing something?
I don't think it matters much since NOTIFY_OK and NOTIFY_DONE have
no further effect on processing of the notification queue, but I believe
you are correct, this is a change from what we originally had. I can
restore the original return values if you'd prefer.
>
> Otherwise LGTM!
>
> Regards,
> Halil
>
>> +notify_done:
>> + mutex_unlock(&matrix_dev->lock);
>> + return notify_rc;
>> }
>>
> [..]
On Tue, 22 Dec 2020 10:37:01 -0500
Tony Krowiak <[email protected]> wrote:
> On 12/21/20 11:05 PM, Halil Pasic wrote:
> > On Mon, 21 Dec 2020 13:56:25 -0500
> > Tony Krowiak <[email protected]> wrote:
> >> static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
> >> unsigned long action, void *data)
> >> {
> >> - int ret;
> >> + int ret, notify_rc = NOTIFY_DONE;
> >> struct ap_matrix_mdev *matrix_mdev;
> >>
> >> if (action != VFIO_GROUP_NOTIFY_SET_KVM)
> >> return NOTIFY_OK;
> >>
> >> matrix_mdev = container_of(nb, struct ap_matrix_mdev, group_notifier);
> >> + mutex_lock(&matrix_dev->lock);
> >>
> >> if (!data) {
> >> - matrix_mdev->kvm = NULL;
> >> - return NOTIFY_OK;
> >> + if (matrix_mdev->kvm)
> >> + vfio_ap_mdev_unset_kvm(matrix_mdev);
> >> + notify_rc = NOTIFY_OK;
> >> + goto notify_done;
> >> }
> >>
> >> ret = vfio_ap_mdev_set_kvm(matrix_mdev, data);
> >> if (ret)
> >> - return NOTIFY_DONE;
> >> + goto notify_done;
> >>
> >> /* If there is no CRYCB pointer, then we can't copy the masks */
> >> if (!matrix_mdev->kvm->arch.crypto.crycbd)
> >> - return NOTIFY_DONE;
> >> + goto notify_done;
> >>
> >> kvm_arch_crypto_set_masks(matrix_mdev->kvm, matrix_mdev->matrix.apm,
> >> matrix_mdev->matrix.aqm,
> >> matrix_mdev->matrix.adm);
> >>
> >> - return NOTIFY_OK;
> > Shouldn't there be an
> > + notify_rc = NOTIFY_OK;
> > here? I mean you initialize notify_rc to NOTIFY_DONE, in the !data branch
> > on success you set notify_rc to NOTIFY_OK, but in the !!data branch it
> > just stays NOTIFY_DONE. Or am I missing something?
>
> I don't think it matters much since NOTIFY_OK and NOTIFY_DONE have
> no further effect on processing of the notification queue, but I believe
> you are correct, this is a change from what we originally had. I can
> restore the original return values if you'd prefer.
Even if they have the same semantics now, that might change in the
future; restoring the original behaviour looks like the right thing to
do.
>
> >
> > Otherwise LGTM!
Same here.
> >
> > Regards,
> > Halil
> >
> >> +notify_done:
> >> + mutex_unlock(&matrix_dev->lock);
> >> + return notify_rc;
> >> }
> >>
> > [..]
>
On Tue, 22 Dec 2020 16:57:06 +0100
Cornelia Huck <[email protected]> wrote:
> On Tue, 22 Dec 2020 10:37:01 -0500
> Tony Krowiak <[email protected]> wrote:
>
> > On 12/21/20 11:05 PM, Halil Pasic wrote:
> > > On Mon, 21 Dec 2020 13:56:25 -0500
> > > Tony Krowiak <[email protected]> wrote:
>
> > >> static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
> > >> unsigned long action, void *data)
> > >> {
> > >> - int ret;
> > >> + int ret, notify_rc = NOTIFY_DONE;
> > >> struct ap_matrix_mdev *matrix_mdev;
> > >>
> > >> if (action != VFIO_GROUP_NOTIFY_SET_KVM)
> > >> return NOTIFY_OK;
> > >>
> > >> matrix_mdev = container_of(nb, struct ap_matrix_mdev, group_notifier);
> > >> + mutex_lock(&matrix_dev->lock);
> > >>
> > >> if (!data) {
> > >> - matrix_mdev->kvm = NULL;
> > >> - return NOTIFY_OK;
> > >> + if (matrix_mdev->kvm)
> > >> + vfio_ap_mdev_unset_kvm(matrix_mdev);
> > >> + notify_rc = NOTIFY_OK;
> > >> + goto notify_done;
> > >> }
> > >>
> > >> ret = vfio_ap_mdev_set_kvm(matrix_mdev, data);
> > >> if (ret)
> > >> - return NOTIFY_DONE;
> > >> + goto notify_done;
> > >>
> > >> /* If there is no CRYCB pointer, then we can't copy the masks */
> > >> if (!matrix_mdev->kvm->arch.crypto.crycbd)
> > >> - return NOTIFY_DONE;
> > >> + goto notify_done;
> > >>
> > >> kvm_arch_crypto_set_masks(matrix_mdev->kvm, matrix_mdev->matrix.apm,
> > >> matrix_mdev->matrix.aqm,
> > >> matrix_mdev->matrix.adm);
> > >>
> > >> - return NOTIFY_OK;
> > > Shouldn't there be an
> > > + notify_rc = NOTIFY_OK;
> > > here? I mean you initialize notify_rc to NOTIFY_DONE, in the !data branch
> > > on success you set notify_rc to NOTIFY_OK, but in the !!data branch it
> > > just stays NOTIFY_DONE. Or am I missing something?
> >
> > I don't think it matters much since NOTIFY_OK and NOTIFY_DONE have
> > no further effect on processing of the notification queue, but I believe
> > you are correct, this is a change from what we originally had. I can
> > restore the original return values if you'd prefer.
>
> Even if they have the same semantics now, that might change in the
> future; restoring the original behaviour looks like the right thing to
> do.
I agree. Especially since we do care to preserve the behavior in
the !data branch. If there is no difference between the two, then it
would probably make sense to clean that up globally.
Regards,
Halil
On 12/22/20 2:43 PM, Halil Pasic wrote:
> On Tue, 22 Dec 2020 16:57:06 +0100
> Cornelia Huck <[email protected]> wrote:
>
>> On Tue, 22 Dec 2020 10:37:01 -0500
>> Tony Krowiak <[email protected]> wrote:
>>
>>> On 12/21/20 11:05 PM, Halil Pasic wrote:
>>>> On Mon, 21 Dec 2020 13:56:25 -0500
>>>> Tony Krowiak <[email protected]> wrote:
>>>>> static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
>>>>> unsigned long action, void *data)
>>>>> {
>>>>> - int ret;
>>>>> + int ret, notify_rc = NOTIFY_DONE;
>>>>> struct ap_matrix_mdev *matrix_mdev;
>>>>>
>>>>> if (action != VFIO_GROUP_NOTIFY_SET_KVM)
>>>>> return NOTIFY_OK;
>>>>>
>>>>> matrix_mdev = container_of(nb, struct ap_matrix_mdev, group_notifier);
>>>>> + mutex_lock(&matrix_dev->lock);
>>>>>
>>>>> if (!data) {
>>>>> - matrix_mdev->kvm = NULL;
>>>>> - return NOTIFY_OK;
>>>>> + if (matrix_mdev->kvm)
>>>>> + vfio_ap_mdev_unset_kvm(matrix_mdev);
>>>>> + notify_rc = NOTIFY_OK;
>>>>> + goto notify_done;
>>>>> }
>>>>>
>>>>> ret = vfio_ap_mdev_set_kvm(matrix_mdev, data);
>>>>> if (ret)
>>>>> - return NOTIFY_DONE;
>>>>> + goto notify_done;
>>>>>
>>>>> /* If there is no CRYCB pointer, then we can't copy the masks */
>>>>> if (!matrix_mdev->kvm->arch.crypto.crycbd)
>>>>> - return NOTIFY_DONE;
>>>>> + goto notify_done;
>>>>>
>>>>> kvm_arch_crypto_set_masks(matrix_mdev->kvm, matrix_mdev->matrix.apm,
>>>>> matrix_mdev->matrix.aqm,
>>>>> matrix_mdev->matrix.adm);
>>>>>
>>>>> - return NOTIFY_OK;
>>>> Shouldn't there be an
>>>> + notify_rc = NOTIFY_OK;
>>>> here? I mean you initialize notify_rc to NOTIFY_DONE, in the !data branch
>>>> on success you set notify_rc to NOTIFY_OK, but in the !!data branch it
>>>> just stays NOTIFY_DONE. Or am I missing something?
>>> I don't think it matters much since NOTIFY_OK and NOTIFY_DONE have
>>> no further effect on processing of the notification queue, but I believe
>>> you are correct, this is a change from what we originally had. I can
>>> restore the original return values if you'd prefer.
>> Even if they have the same semantics now, that might change in the
>> future; restoring the original behaviour looks like the right thing to
>> do.
> I agree. Especially since we do care to preserve the behavior in
> the !data branch. If there is no difference between the two, then it
> would probably make sense to clean that up globally.
Got it. I'm going to do a quick turnaround on the next version so we
can get this merged if need be. I will be taking off for Christmas vacation
and will be gone until sometime the first week in January.
>
> Regards,
> Halil