Messages in this thread | | | Subject | Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer | From | Tony Krowiak <> | Date | Mon, 23 Aug 2021 09:08:37 -0400 |
| |
On 8/19/21 5:42 PM, Halil Pasic wrote: > On Thu, 19 Aug 2021 09:36:34 -0400 > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >>>>> 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. >> The vcpu->kvm->arch.crypto.pqap_hook is a pointer to a function >> pointer. The actual function pointer is stored in matrix_mdev->pqap_hook, >> the reason being that the handle_pqap function in vfio_ap_ops.c >> retrieves the matrix_mdev via a container_of macro. The dereferencing >> of the vcpu->kvm->arch.crypto.pqap_hook into a local variable was >> to get the function pointer. There may have been a more stylish >> way of doing this, but the functionality is there. > You are right, and I was wrong. But then we do have to distinct pointer > deferences, and we check for NULL only once. > > I still do believe we do not have a potential null pointer dereference > here, but the reason for that is that vfio-ap (the party that manages > these pointers) guarantees that whenever > vcpu->kvm->arch.crypto.pqap_hook != NULL is true, > *vcpu->kvm->arch.crypto.pqap_hook != NULL is also true (and also that > the function pointer is a valid one). Which is the case, because we > set matrix_mdev->pqap_hook in vfio_ap_mdev_create() and don't touch > it any more. > > In my opinion it is worth a comment.
Even I had to look at it again to respond to you, so a comment is probably called for.
> > >>> Regards, >>> Halil >>> >>>> >>>>> + ret = pqap_hook(vcpu); > BTW the second dereference takes place here. > > If we wanted, we could make sure we don't dereference a null pointer > here but I think that would be an overkill.
I agree, it is overkill.
> > Regards, > Halil >>>> [...]
| |