Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer

[Halil Pasic <pasic@linux.ibm.com>]

On Wed, 18 Aug 2021 19:03:33 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 19.07.21 21:35, Tony Krowiak wrote:
> > The function pointer to the interception handler for the PQAP instruction
> > can get changed during the interception process. Let's add a
> > semaphore to struct kvm_s390_crypto to control read/write access to the
> > function pointer contained therein.
> > 
> > The semaphore must be locked for write access by the vfio_ap device driver
> > when notified that the KVM pointer has been set or cleared. It must be
> > locked for read access by the interception framework when the PQAP
> > instruction is intercepted.
> > 
> > Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> >   arch/s390/include/asm/kvm_host.h      |  8 +++-----
> >   arch/s390/kvm/kvm-s390.c              |  1 +
> >   arch/s390/kvm/priv.c                  | 10 ++++++----
> >   drivers/s390/crypto/vfio_ap_ops.c     | 23 +++++++++++++++++------
> >   drivers/s390/crypto/vfio_ap_private.h |  2 +-
> >   5 files changed, 28 insertions(+), 16 deletions(-)
> > 
> > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> > index 9b4473f76e56..f18849d259e6 100644
> > --- a/arch/s390/include/asm/kvm_host.h
> > +++ b/arch/s390/include/asm/kvm_host.h
> > @@ -798,14 +798,12 @@ struct kvm_s390_cpu_model {
> >   	unsigned short ibc;
> >   };
> >   
> > -struct kvm_s390_module_hook {
> > -	int (*hook)(struct kvm_vcpu *vcpu);
> > -	struct module *owner;
> > -};
> > +typedef int (*crypto_hook)(struct kvm_vcpu *vcpu);
> >   
> >   struct kvm_s390_crypto {
> >   	struct kvm_s390_crypto_cb *crycb;
> > -	struct kvm_s390_module_hook *pqap_hook;
> > +	struct rw_semaphore pqap_hook_rwsem;
> > +	crypto_hook *pqap_hook;
> >   	__u32 crycbd;
> >   	__u8 aes_kw;
> >   	__u8 dea_kw;
> > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > index b655a7d82bf0..a08f242a9f27 100644
> > --- a/arch/s390/kvm/kvm-s390.c
> > +++ b/arch/s390/kvm/kvm-s390.c
> > @@ -2630,6 +2630,7 @@ static void kvm_s390_crypto_init(struct kvm *kvm)
> >   {
> >   	kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb;
> >   	kvm_s390_set_crycb_format(kvm);
> > +	init_rwsem(&kvm->arch.crypto.pqap_hook_rwsem);
> >   
> >   	if (!test_kvm_facility(kvm, 76))
> >   		return;
> > diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> > index 9928f785c677..6bed9406c1f3 100644
> > --- a/arch/s390/kvm/priv.c
> > +++ b/arch/s390/kvm/priv.c
> > @@ -610,6 +610,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
> >   static int handle_pqap(struct kvm_vcpu *vcpu)
> >   {
> >   	struct ap_queue_status status = {};
> > +	crypto_hook pqap_hook;
> >   	unsigned long reg0;
> >   	int ret;
> >   	uint8_t fc;
> > @@ -657,15 +658,16 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
> >   	 * Verify that the hook callback is registered, lock the owner
> >   	 * and call the hook.
> >   	 */
> > +	down_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem);
> >   	if (vcpu->kvm->arch.crypto.pqap_hook) {                     <--- HERE
> > -		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);
> > +		pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook;  
> 
> Dont we have to check for NULL here? If not can you add a comment why?

I believe we did the necessary check on the line I just marked with
"<--- HERE". 

I find that "*" operator confusing in this context as it doesn't do
any good for us. I believe this situation is described in 6.5.3.2.4 of
the c11 standard. For convenience I will cite from the corresponding
draft:
"The unary * operator denotes indirection. If the operand points to a
function, the result is a function designator; if it points to an
object, the result is an lvalue designating the object. If the operand
has type ‘‘pointer to type’’, the result has type ‘‘type’’. If an
invalid value has been assigned to the pointer, the behavior of the
unary * operator is undefined."

Frankly I also fail to see the benefit of introducing the local variable
named "pqap_hook", but back then I decided to not complain about style.

Regards,
Halil

> 
> 
> > +		ret = pqap_hook(vcpu);  
> [...]