Messages in this thread | | | Subject | Re: [PATCH v2] s390/vfio-ap: fix memory leak in mdev remove callback | From | Christian Borntraeger <> | Date | Wed, 19 May 2021 10:17:49 +0200 |
| |
On 19.05.21 01:27, Halil Pasic wrote: > On Tue, 18 May 2021 19:01:42 +0200 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> On 18.05.21 17:33, Halil Pasic wrote: >>> On Tue, 18 May 2021 15:59:36 +0200 >>> Christian Borntraeger <borntraeger@de.ibm.com> wrote: > [..] >>>>>> >>>>>> 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? >>> >>> Unfortunately just "the hook" is ambiguous in this context. We >>> have kvm->arch.crypto.pqap_hook that is supposed to point to >>> a struct kvm_s390_module_hook member of struct ap_matrix_mdev >>> which is also called pqap_hook. And struct kvm_s390_module_hook >>> has function pointer member named "hook". >> >> I was referring to the full struct. >>> >>>>> >>>>> 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. >>> >>> In my opinion, we should make the accesses to the >>> kvm->arch.crypto.pqap_hook pointer properly synchronized. I'm >>> not sure if that is what you are proposing. How do we usually >>> do synchronisation on the stuff that lives in kvm->arch? >>> >> >> RCU is a method of synchronization. We make sure that structure >> pqap_hook is still valid as long as we are inside the rcu read >> lock. So the idea is: clear pointer, wait until all old readers >> have finished and the proceed with getting rid of the structure. > > Yes I know that RCU is a method of synchronization, but I'm not > very familiar with it. I'm a little confused by "read the hook > once and then work on a copy". I guess, I would have to read up > on the RCU again to get clarity. I intend to brush up my RCU knowledge > once the patch comes along. I would be glad to have your help when > reviewing an RCU based solution for this.
Just had a quick look. Its not trivial, as the hook function itself takes a mutex and an rcu section must not sleep. Will have a deeper look.
| |