On 18.05.21 20:14, Tony Krowiak wrote:
On 5/18/21 9:59 AM, Christian Borntraeger wrote:
On 18.05.21 15:42, Tony Krowiak wrote:
On 5/18/21 5:30 AM, Christian Borntraeger wrote:
On 17.05.21 21:10, Halil Pasic wrote:
On Mon, 17 May 2021 09:37:42 -0400 Tony Krowiak akrowiak@linux.ibm.com wrote:
> > 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?
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?
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?
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.
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.
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.