Re: [PATCH v4 2/2] s390/vfio-ap: control access to PQAP(AQIC) interception handler

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 5/24/21 10:37 AM, Jason J. Herne wrote:
On 5/21/21 3:36 PM, Tony Krowiak wrote:
The function pointer to the handler that processes interception of the
PQAP instruction is contained in the mdev. If the mdev is removed and
its storage de-allocated during the processing of the PQAP instruction,
the function pointer could get wiped out before the function is called
because there is currently nothing that controls access to it.

This patch introduces two new functions:
* The kvm_arch_crypto_register_hook() function registers a function pointer
   for processing intercepted crypto instructions.
* The kvm_arch_crypto_register_hook() function un-registers a function
   pointer that was previously registered.

Typo: You meant kvm_arch_crypto_UNregister_hook() in the second bullet.


Just one overall observation on this one. The whole hook system seems kind of over-engineered if this is our only use for it. It looks like a kvm_s390_crypto_hook is meant to link a specific module with a function pointer. Do we really need this concept?

I think a simpler design could be to just place a mutex and a function pointer in the kvm_s390_crypto struct. Then you can grab the mutex in vfio_ap_ops.c when registering/unregistering. You would also grab the mutex in priv.c when calling the function pointer. What I am suggesting is essentially the exact same scheme you have implemented here, but simpler and with less infrastructure.

With that said, I'll point out that I am relative new to this code (and this patch series) so maybe I've missed something and the extra complexity is needed for some reason. But if it is not, I'm all in favor of keeping things simple.


After thinking about this problem a bit more, I'm wondering if we can remove the lock entirely. How about we store a function pointer in kvm_s390_crypto? Initially that function pointer will point to a stub function that handles the error case, exactly like it is done in priv.c:handle_pqap() today when the function pointer would be NULL. When the ap module loads, we can simply change the function pointer to point to vfio_ap_ops:handle_pqap(). When we unload the module we change the function pointer back to the stub. The updates should be atomic operations so no lock needed, right?

--
-- Jason J. Herne (jjherne@xxxxxxxxxxxxx)



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux