[v3] s390/vfio-ap: clean up vfio_ap resources when KVM pointer invalidated

Commit Message

Anthony Krowiak Dec. 14, 2020, 4:56 p.m. UTC

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")
Signed-off-by: Tony Krowiak <[e-mail]>
---
 drivers/s390/crypto/vfio_ap_ops.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

Comments

Greg KH Dec. 14, 2020, 5:07 p.m. UTC | #1

On Mon, Dec 14, 2020 at 11:56:17AM -0500, Tony Krowiak 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")
> Signed-off-by: Tony Krowiak <[e-mail]>
> ---
>  drivers/s390/crypto/vfio_ap_ops.c | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

Anthony Krowiak Dec. 14, 2020, 7:39 p.m. UTC | #2

On 12/14/20 12:07 PM, Greg KH wrote:
> On Mon, Dec 14, 2020 at 11:56:17AM -0500, Tony Krowiak 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")
>> Signed-off-by: Tony Krowiak <[e-mail]>
>> ---
>>   drivers/s390/crypto/vfio_ap_ops.c | 29 ++++++++++++++++++++---------
>>   1 file changed, 20 insertions(+), 9 deletions(-)
> <formletter>
>
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree.  Please read:
>      https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> for how to do this properly.
>
> </formletter>

I read the document on the correct way to submit patches for inclusion in
the stable kernel. I apologize for my ignorance, but I don't see the
problem. Can you help me out here? Does a patch that fixes a memory leak
not qualify or is it something else?

Greg KH Dec. 14, 2020, 7:43 p.m. UTC | #3

On Mon, Dec 14, 2020 at 02:39:17PM -0500, Tony Krowiak wrote:
> 
> 
> On 12/14/20 12:07 PM, Greg KH wrote:
> > On Mon, Dec 14, 2020 at 11:56:17AM -0500, Tony Krowiak 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")
> > > Signed-off-by: Tony Krowiak <[e-mail]>
> > > ---
> > >   drivers/s390/crypto/vfio_ap_ops.c | 29 ++++++++++++++++++++---------
> > >   1 file changed, 20 insertions(+), 9 deletions(-)
> > <formletter>
> > 
> > This is not the correct way to submit patches for inclusion in the
> > stable kernel tree.  Please read:
> >      https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> > for how to do this properly.
> > 
> > </formletter>
> 
> I read the document on the correct way to submit patches for inclusion in
> the stable kernel. I apologize for my ignorance, but I don't see the
> problem. Can you help me out here? Does a patch that fixes a memory leak
> not qualify or is it something else?

You forgot to put "Cc: stable..." in the signed-off-by area.

thanks,

greg k-h

Anthony Krowiak Dec. 14, 2020, 7:48 p.m. UTC | #4

On 12/14/20 2:43 PM, Greg KH wrote:
> On Mon, Dec 14, 2020 at 02:39:17PM -0500, Tony Krowiak wrote:
>>
>> On 12/14/20 12:07 PM, Greg KH wrote:
>>> On Mon, Dec 14, 2020 at 11:56:17AM -0500, Tony Krowiak 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")
>>>> Signed-off-by: Tony Krowiak <[e-mail]>
>>>> ---
>>>>    drivers/s390/crypto/vfio_ap_ops.c | 29 ++++++++++++++++++++---------
>>>>    1 file changed, 20 insertions(+), 9 deletions(-)
>>> <formletter>
>>>
>>> This is not the correct way to submit patches for inclusion in the
>>> stable kernel tree.  Please read:
>>>       https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
>>> for how to do this properly.
>>>
>>> </formletter>
>> I read the document on the correct way to submit patches for inclusion in
>> the stable kernel. I apologize for my ignorance, but I don't see the
>> problem. Can you help me out here? Does a patch that fixes a memory leak
>> not qualify or is it something else?
> You forgot to put "Cc: stable..." in the signed-off-by area.
>
> thanks,

Option 1, I must be blind. thanks

>
> greg k-h

Halil Pasic Dec. 15, 2020, 10:57 a.m. UTC | #5

On Mon, 14 Dec 2020 11:56:17 -0500
Tony Krowiak <[e-mail]> 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")
> Signed-off-by: Tony Krowiak <[e-mail]>
> ---
>  drivers/s390/crypto/vfio_ap_ops.c | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index e0bde8518745..cd22e85588e1 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -1037,8 +1037,6 @@ 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);
> @@ -1049,7 +1047,6 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
>  	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 +1080,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;


This patch LGTM. The only concern I have with it is whether a
different cpu is guaranteed to observe the above assignment as
an atomic operation. I think we didn't finish this discussion
at v1, or did we?

Regards,
Halil

> +	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)

Cornelia Huck Dec. 15, 2020, 5:42 p.m. UTC | #6

On Mon, 14 Dec 2020 11:56:17 -0500
Tony Krowiak <[e-mail]> 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")
> Signed-off-by: Tony Krowiak <[e-mail]>
> ---
>  drivers/s390/crypto/vfio_ap_ops.c | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)

Reviewed-by: Cornelia Huck <[e-mail]>

Christian Borntraeger Dec. 15, 2020, 6:10 p.m. UTC | #7

On 15.12.20 11:57, Halil Pasic wrote:
> On Mon, 14 Dec 2020 11:56:17 -0500
> Tony Krowiak <[e-mail]> 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")
>> Signed-off-by: Tony Krowiak <[e-mail]>
>> ---
>>  drivers/s390/crypto/vfio_ap_ops.c | 29 ++++++++++++++++++++---------
>>  1 file changed, 20 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index e0bde8518745..cd22e85588e1 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -1037,8 +1037,6 @@ 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);
>> @@ -1049,7 +1047,6 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
>>  	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 +1080,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;
> 
> 
> This patch LGTM. The only concern I have with it is whether a
> different cpu is guaranteed to observe the above assignment as
> an atomic operation. I think we didn't finish this discussion
> at v1, or did we?

You mean just this assigment:
>> +	matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
should either have the old or the new value, but not halve zero halve old?

Normally this should be ok (and I would consider this a compiler bug if
this is split into 2 32 bit zeroes) But if you really want to be sure then we
can use WRITE_ONCE.
I think we take this via the s390 tree? I can add the WRITE_ONCE when applying?

Halil Pasic Dec. 16, 2020, 1:21 a.m. UTC | #8

On Tue, 15 Dec 2020 19:10:20 +0100
Christian Borntraeger <[e-mail]> wrote:

> 
> 
> On 15.12.20 11:57, Halil Pasic wrote:
> > On Mon, 14 Dec 2020 11:56:17 -0500
> > Tony Krowiak <[e-mail]> 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")
> >> Signed-off-by: Tony Krowiak <[e-mail]>
> >> ---
> >>  drivers/s390/crypto/vfio_ap_ops.c | 29 ++++++++++++++++++++---------
> >>  1 file changed, 20 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> >> index e0bde8518745..cd22e85588e1 100644
> >> --- a/drivers/s390/crypto/vfio_ap_ops.c
> >> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> >> @@ -1037,8 +1037,6 @@ 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);
> >> @@ -1049,7 +1047,6 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
> >>  	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 +1080,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;
> > 
> > 
> > This patch LGTM. The only concern I have with it is whether a
> > different cpu is guaranteed to observe the above assignment as
> > an atomic operation. I think we didn't finish this discussion
> > at v1, or did we?
> 
> You mean just this assigment:
> >> +	matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
> should either have the old or the new value, but not halve zero halve old?
>

Yes that is the assignment I was referring to. Old value will work as well because
kvm holds a reference to this module while in the pqap_hook.
 
> Normally this should be ok (and I would consider this a compiler bug if
> this is split into 2 32 bit zeroes) But if you really want to be sure then we
> can use WRITE_ONCE.

Just my curiosity: what would make this a bug? Is it the s390 elf ABI,
or some gcc feature, or even the C standard? Also how exactly would
WRITE_ONCE, also access via volatile help in this particular situation?

I agree, if the member is properly aligned, (which it is),
normally/probably we are fine on s390x (which is also a given). 

> I think we take this via the s390 tree? I can add the WRITE_ONCE when applying?

Yes that works fine with me.

Reviewed-by: Halil Pasic <[e-mail]>

Christian Borntraeger Dec. 16, 2020, 9:58 a.m. UTC | #9

On 16.12.20 02:21, Halil Pasic wrote:
> On Tue, 15 Dec 2020 19:10:20 +0100
> Christian Borntraeger <[e-mail]> wrote:
> 
>>
>>
>> On 15.12.20 11:57, Halil Pasic wrote:
>>> On Mon, 14 Dec 2020 11:56:17 -0500
>>> Tony Krowiak <[e-mail]> 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")
>>>> Signed-off-by: Tony Krowiak <[e-mail]>
>>>> ---
>>>>  drivers/s390/crypto/vfio_ap_ops.c | 29 ++++++++++++++++++++---------
>>>>  1 file changed, 20 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>>>> index e0bde8518745..cd22e85588e1 100644
>>>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>>>> @@ -1037,8 +1037,6 @@ 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);
>>>> @@ -1049,7 +1047,6 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
>>>>  	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 +1080,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;
>>>
>>>
>>> This patch LGTM. The only concern I have with it is whether a
>>> different cpu is guaranteed to observe the above assignment as
>>> an atomic operation. I think we didn't finish this discussion
>>> at v1, or did we?
>>
>> You mean just this assigment:
>>>> +	matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
>> should either have the old or the new value, but not halve zero halve old?
>>
> 
> Yes that is the assignment I was referring to. Old value will work as well because
> kvm holds a reference to this module while in the pqap_hook.
>  
>> Normally this should be ok (and I would consider this a compiler bug if
>> this is split into 2 32 bit zeroes) But if you really want to be sure then we
>> can use WRITE_ONCE.
> 
> Just my curiosity: what would make this a bug? Is it the s390 elf ABI,
> or some gcc feature, or even the C standard? Also how exactly would
> WRITE_ONCE, also access via volatile help in this particular situation?

I think its a tricky things and not strictly guaranteed, but there is a lot
of code that relies on the atomicity of word sizes. see for example the discussion
here
https://lore.kernel.org/lkml/CAHk-=[e-mail]/

WRITE_ONCE will not change the guarantees a lot, but it is mostly a documentation
that we assume atomic access here.


> 
> I agree, if the member is properly aligned, (which it is),
> normally/probably we are fine on s390x (which is also a given). 
> 
>> I think we take this via the s390 tree? I can add the WRITE_ONCE when applying?
> 
> Yes that works fine with me.
> 
> Reviewed-by: Halil Pasic <[e-mail]>
>

Christian Borntraeger Dec. 16, 2020, 4:05 p.m. UTC | #10

On 16.12.20 10:58, Christian Borntraeger wrote:
> On 16.12.20 02:21, Halil Pasic wrote:
>> On Tue, 15 Dec 2020 19:10:20 +0100
>> Christian Borntraeger <[e-mail]> wrote:
>>
>>>
>>>
>>> On 15.12.20 11:57, Halil Pasic wrote:
>>>> On Mon, 14 Dec 2020 11:56:17 -0500
>>>> Tony Krowiak <[e-mail]> 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")
>>>>> Signed-off-by: Tony Krowiak <[e-mail]>
>>>>> ---
>>>>>  drivers/s390/crypto/vfio_ap_ops.c | 29 ++++++++++++++++++++---------
>>>>>  1 file changed, 20 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>>>>> index e0bde8518745..cd22e85588e1 100644
>>>>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>>>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>>>>> @@ -1037,8 +1037,6 @@ 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);
>>>>> @@ -1049,7 +1047,6 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
>>>>>  	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 +1080,49 @@ static int vfio_ap_mdev_iommu_notifier(struct notifier_block *nb,
>>>>>  	return NOTIFY_DONE;
>>>>>  }
>>>>>
>>>>> +static void "(struct ap_matrix_mdev *matrix_mdev)
>>>>> +{
>>>>> +	kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
>>>>> +	matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
>>>>
>>>>
>>>> This patch LGTM. The only concern I have with it is whether a
>>>> different cpu is guaranteed to observe the above assignment as
>>>> an atomic operation. I think we didn't finish this discussion
>>>> at v1, or did we?
>>>
>>> You mean just this assigment:
>>>>> +	matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
>>> should either have the old or the new value, but not halve zero halve old?
>>>
>>
>> Yes that is the assignment I was referring to. Old value will work as well because
>> kvm holds a reference to this module while in the pqap_hook.
>>  
>>> Normally this should be ok (and I would consider this a compiler bug if
>>> this is split into 2 32 bit zeroes) But if you really want to be sure then we
>>> can use WRITE_ONCE.
>>
>> Just my curiosity: what would make this a bug? Is it the s390 elf ABI,
>> or some gcc feature, or even the C standard? Also how exactly would
>> WRITE_ONCE, also access via volatile help in this particular situation?
> 
> I think its a tricky things and not strictly guaranteed, but there is a lot
> of code that relies on the atomicity of word sizes. see for example the discussion
> here
> https://lore.kernel.org/lkml/CAHk-=[e-mail]/
> 
> WRITE_ONCE will not change the guarantees a lot, but it is mostly a documentation
> that we assume atomic access here.

After looking again at the code, I think I have to correct myself.
WRITE_ONCE does not look necessary.


Another thing, though:
Shouldnt we also replace this code

[...]
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;
[...]

with vfio_ap_mdev_unset_kvm ?

Halil Pasic Dec. 16, 2020, 10 p.m. UTC | #11

On Wed, 16 Dec 2020 17:05:24 +0100
Christian Borntraeger <[e-mail]> wrote:

> 
> 
> On 16.12.20 10:58, Christian Borntraeger wrote:
> > On 16.12.20 02:21, Halil Pasic wrote:
> >> On Tue, 15 Dec 2020 19:10:20 +0100
> >> Christian Borntraeger <[e-mail]> wrote:
> >>
> >>>
> >>>
> >>> On 15.12.20 11:57, Halil Pasic wrote:
> >>>> On Mon, 14 Dec 2020 11:56:17 -0500
> >>>> Tony Krowiak <[e-mail]> 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")
> >>>>> Signed-off-by: Tony Krowiak <[e-mail]>
> >>>>> ---
> >>>>>  drivers/s390/crypto/vfio_ap_ops.c | 29 ++++++++++++++++++++---------
> >>>>>  1 file changed, 20 insertions(+), 9 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> >>>>> index e0bde8518745..cd22e85588e1 100644
> >>>>> --- a/drivers/s390/crypto/vfio_ap_ops.c
> >>>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> >>>>> @@ -1037,8 +1037,6 @@ 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);
> >>>>> @@ -1049,7 +1047,6 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
> >>>>>  	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 +1080,49 @@ static int vfio_ap_mdev_iommu_notifier(struct notifier_block *nb,
> >>>>>  	return NOTIFY_DONE;
> >>>>>  }
> >>>>>
> >>>>> +static void "(struct ap_matrix_mdev *matrix_mdev)
> >>>>> +{
> >>>>> +	kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
> >>>>> +	matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
> >>>>
> >>>>
> >>>> This patch LGTM. The only concern I have with it is whether a
> >>>> different cpu is guaranteed to observe the above assignment as
> >>>> an atomic operation. I think we didn't finish this discussion
> >>>> at v1, or did we?
> >>>
> >>> You mean just this assigment:
> >>>>> +	matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
> >>> should either have the old or the new value, but not halve zero halve old?
> >>>
> >>
> >> Yes that is the assignment I was referring to. Old value will work as well because
> >> kvm holds a reference to this module while in the pqap_hook.
> >>  
> >>> Normally this should be ok (and I would consider this a compiler bug if
> >>> this is split into 2 32 bit zeroes) But if you really want to be sure then we
> >>> can use WRITE_ONCE.
> >>
> >> Just my curiosity: what would make this a bug? Is it the s390 elf ABI,
> >> or some gcc feature, or even the C standard? Also how exactly would
> >> WRITE_ONCE, also access via volatile help in this particular situation?
> > 
> > I think its a tricky things and not strictly guaranteed, but there is a lot
> > of code that relies on the atomicity of word sizes. see for example the discussion
> > here
> > https://lore.kernel.org/lkml/CAHk-=[e-mail]/
> > 
> > WRITE_ONCE will not change the guarantees a lot, but it is mostly a documentation
> > that we assume atomic access here.
> 
> After looking again at the code, I think I have to correct myself.
> WRITE_ONCE does not look necessary.
> 

Yes, volatile access is not about atomic access. Whether a volatile
access here is a good idea or not, is a different question.

> 
> Another thing, though:
> Shouldnt we also replace this code
> 
> [...]
> 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;
> [...]
> 
> with vfio_ap_mdev_unset_kvm ?

Yes, we had something like this in v2 already. Would be good style to
do so.

Regards,
Halil

Halil Pasic Dec. 16, 2020, 10:49 p.m. UTC | #12

On Wed, 16 Dec 2020 10:58:48 +0100
Christian Borntraeger <[e-mail]> wrote:

> On 16.12.20 02:21, Halil Pasic wrote:
> > On Tue, 15 Dec 2020 19:10:20 +0100
> > Christian Borntraeger <[e-mail]> wrote:
> > 
> >>
> >>
> >> On 15.12.20 11:57, Halil Pasic wrote:
> >>> On Mon, 14 Dec 2020 11:56:17 -0500
> >>> Tony Krowiak <[e-mail]> 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")
> >>>> Signed-off-by: Tony Krowiak <[e-mail]>
> >>>> ---
> >>>>  drivers/s390/crypto/vfio_ap_ops.c | 29 ++++++++++++++++++++---------
> >>>>  1 file changed, 20 insertions(+), 9 deletions(-)
> >>>>
> >>>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> >>>> index e0bde8518745..cd22e85588e1 100644
> >>>> --- a/drivers/s390/crypto/vfio_ap_ops.c
> >>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> >>>> @@ -1037,8 +1037,6 @@ 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);
> >>>> @@ -1049,7 +1047,6 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
> >>>>  	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 +1080,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;
> >>>
> >>>
> >>> This patch LGTM. The only concern I have with it is whether a
> >>> different cpu is guaranteed to observe the above assignment as
> >>> an atomic operation. I think we didn't finish this discussion
> >>> at v1, or did we?
> >>
> >> You mean just this assigment:
> >>>> +	matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
> >> should either have the old or the new value, but not halve zero halve old?
> >>
> > 
> > Yes that is the assignment I was referring to. Old value will work as well because
> > kvm holds a reference to this module while in the pqap_hook.
> >  
> >> Normally this should be ok (and I would consider this a compiler bug if
> >> this is split into 2 32 bit zeroes) But if you really want to be sure then we
> >> can use WRITE_ONCE.
> > 
> > Just my curiosity: what would make this a bug? Is it the s390 elf ABI,
> > or some gcc feature, or even the C standard? Also how exactly would
> > WRITE_ONCE, also access via volatile help in this particular situation?
> 
> I think its a tricky things and not strictly guaranteed, but there is a lot
> of code that relies on the atomicity of word sizes. see for example the discussion
> here
> https://lore.kernel.org/lkml/CAHk-=[e-mail]/
> 
> WRITE_ONCE will not change the guarantees a lot, but it is mostly a documentation
> that we assume atomic access here.

Thanks a lot! I've read it, and IMHO it seems to contradict the section
https://lwn.net/Articles/793253/#Store%20Tearing a little. From there, I also learned
that WRITE_ONCE (i.e. volatile access) can help, although I don't really
understand why. Of course, we don't need to be portable here, as this
is s390 only code. So we might be safe without anything -- I don't know.
I believe, if volatile were enough (under any circumstances), the C
standard wouldn't have introduced atomic types.

Regards,
Halil

Anthony Krowiak Dec. 17, 2020, 12:39 a.m. UTC | #13

On 12/14/20 11:56 AM, Tony Krowiak 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")
> Signed-off-by: Tony Krowiak <[e-mail]>
> ---
>   drivers/s390/crypto/vfio_ap_ops.c | 29 ++++++++++++++++++++---------
>   1 file changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index e0bde8518745..cd22e85588e1 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -1037,8 +1037,6 @@ 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);

This unlock needs to be removed.

> @@ -1049,7 +1047,6 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
>   	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 +1080,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)

Anthony Krowiak Dec. 17, 2020, 12:46 a.m. UTC | #14

On 12/16/20 11:05 AM, Christian Borntraeger wrote:
>
> On 16.12.20 10:58, Christian Borntraeger wrote:
>> On 16.12.20 02:21, Halil Pasic wrote:
>>> On Tue, 15 Dec 2020 19:10:20 +0100
>>> Christian Borntraeger <[e-mail]> wrote:
>>>
>>>>
>>>> On 15.12.20 11:57, Halil Pasic wrote:
>>>>> On Mon, 14 Dec 2020 11:56:17 -0500
>>>>> Tony Krowiak <[e-mail]> 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")
>>>>>> Signed-off-by: Tony Krowiak <[e-mail]>
>>>>>> ---
>>>>>>   drivers/s390/crypto/vfio_ap_ops.c | 29 ++++++++++++++++++++---------
>>>>>>   1 file changed, 20 insertions(+), 9 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>>>>>> index e0bde8518745..cd22e85588e1 100644
>>>>>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>>>>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>>>>>> @@ -1037,8 +1037,6 @@ 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);
>>>>>> @@ -1049,7 +1047,6 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
>>>>>>   	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 +1080,49 @@ static int vfio_ap_mdev_iommu_notifier(struct notifier_block *nb,
>>>>>>   	return NOTIFY_DONE;
>>>>>>   }
>>>>>>
>>>>>> +static void "(struct ap_matrix_mdev *matrix_mdev)
>>>>>> +{
>>>>>> +	kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
>>>>>> +	matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
>>>>>
>>>>> This patch LGTM. The only concern I have with it is whether a
>>>>> different cpu is guaranteed to observe the above assignment as
>>>>> an atomic operation. I think we didn't finish this discussion
>>>>> at v1, or did we?
>>>> You mean just this assigment:
>>>>>> +	matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
>>>> should either have the old or the new value, but not halve zero halve old?
>>>>
>>> Yes that is the assignment I was referring to. Old value will work as well because
>>> kvm holds a reference to this module while in the pqap_hook.
>>>   
>>>> Normally this should be ok (and I would consider this a compiler bug if
>>>> this is split into 2 32 bit zeroes) But if you really want to be sure then we
>>>> can use WRITE_ONCE.
>>> Just my curiosity: what would make this a bug? Is it the s390 elf ABI,
>>> or some gcc feature, or even the C standard? Also how exactly would
>>> WRITE_ONCE, also access via volatile help in this particular situation?
>> I think its a tricky things and not strictly guaranteed, but there is a lot
>> of code that relies on the atomicity of word sizes. see for example the discussion
>> here
>> https://lore.kernel.org/lkml/CAHk-=[e-mail]/
>>
>> WRITE_ONCE will not change the guarantees a lot, but it is mostly a documentation
>> that we assume atomic access here.
> After looking again at the code, I think I have to correct myself.
> WRITE_ONCE does not look necessary.
>
>
> Another thing, though:
> Shouldnt we also replace this code
>
> [...]
> 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;
> [...]
>
> with vfio_ap_mdev_unset_kvm ?

I had that in the v2 patches, but mistakenly removed it
because of a misinterpretation of the docs on posting a
patch for a stable release. I'll restore it since I have to
remove the unlock from the vfio_ap_mdev_unset_kvm
function.

Patch

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index e0bde8518745..cd22e85588e1 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1037,8 +1037,6 @@  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);
@@ -1049,7 +1047,6 @@  static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
 	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 +1080,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)