lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 11 Jan 2022 16:58:13 -0500 From: Tony Krowiak <akrowiak@...ux.ibm.com> To: Halil Pasic <pasic@...ux.ibm.com> Cc: linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org, kvm@...r.kernel.org, jjherne@...ux.ibm.com, freude@...ux.ibm.com, borntraeger@...ibm.com, cohuck@...hat.com, mjrosato@...ux.ibm.com, alex.williamson@...hat.com, kwankhede@...dia.com, fiuczy@...ux.ibm.com Subject: Re: [PATCH v17 08/15] s390/vfio-ap: keep track of active guests On 12/29/21 22:33, Halil Pasic wrote: > On Thu, 21 Oct 2021 11:23:25 -0400 > Tony Krowiak <akrowiak@...ux.ibm.com> wrote: > >> The vfio_ap device driver registers for notification when the pointer to >> the KVM object for a guest is set. Let's store the KVM pointer as well as >> the pointer to the mediated device when the KVM pointer is set. > [..] > > >> struct ap_matrix_dev { >> ... >> struct rw_semaphore guests_lock; >> struct list_head guests; >> ... >> } >> >> The 'guests_lock' field is a r/w semaphore to control access to the >> 'guests' field. The 'guests' field is a list of ap_guest >> structures containing the KVM and matrix_mdev pointers for each active >> guest. An ap_guest structure will be stored into the list whenever the >> vfio_ap device driver is notified that the KVM pointer has been set and >> removed when notified that the KVM pointer has been cleared. >> > Is this about the field or about the list including all the nodes? This > reads lie guests_lock only protects the head element, which makes no > sense to me. Because of how these lists work. It locks the list, I can rewrite the description. > > The narrowest scope that could make sense is all the list_head stuff > in the entire list. I.e. one would only need the lock to traverse or > manipulate the list, while the payload would still be subject to > the matrix_dev->lock mutex. The matrix_dev->guests lock is needed whenever the kvm->lock is needed because the struct ap_guest object is created and the struct kvm assigned to it when the kvm pointer is set (vfio_ap_mdev_set_kvm function). So, in order to access the ap_guest object and retrieve the kvm pointer, we have to ensure the ap_guest_object is still available. The fact we can get the kvm pointer from the ap_matrix_mdev object just makes things more efficient - i.e., we won't have to traverse the list. Whenever the kvm->lock and matrix_dev->lock mutexes must be held, the order is: matrix_dev->guests_lock matrix_dev->guests->kvm->lock matrix_dev->lock There are times where all three locks are not required; for example, the handle_pqap and vfio_ap_mdev_probe/remove functions only require the matrix_dev->lock because it does not need to lock kvm. > > [..] > >> +struct ap_guest { >> + struct kvm *kvm; >> + struct list_head node; >> +}; >> + >> /** >> * struct ap_matrix_dev - Contains the data for the matrix device. >> * >> @@ -39,6 +44,9 @@ >> * single ap_matrix_mdev device. It's quite coarse but we don't >> * expect much contention. >> * @vfio_ap_drv: the vfio_ap device driver >> + * @guests_lock: r/w semaphore for protecting access to @guests >> + * @guests: list of guests (struct ap_guest) using AP devices bound to the >> + * vfio_ap device driver. > Please compare the above. Also if it is only about the access to the > list, then you could drop the lock right after create, and not keep it > till the very end of vfio_ap_mdev_set_kvm(). Right? That would be true if it only controlled access to the list, but as I explained above, that is not its sole purpose. > > In any case I'm skeptical about this whole struct ap_guest business. To > me, it looks like something that just makes things more obscure and > complicated without any real benefit. I'm open to other ideas, but you'll have to come up with a way to take the kvm->lock before the matrix_mdev->lock in the vfio_ap_mdev_probe_queue and vfio_ap_mdev_remove_queue functions where we don't have access to the ap_matrix_mdev object to which the APQN is assigned and has the pointer to the kvm object. In order to retrieve the matrix_mdev, we need the matrix_dev->lock. In order to hot plug/unplug the queue, we need the kvm->lock. There's your catch-22 that needs to be solved. This design is my attempt to solve that. > > Regards, > Halil > >> */ >> struct ap_matrix_dev { >> struct device device; >> @@ -47,6 +55,8 @@ struct ap_matrix_dev { >> struct list_head mdev_list; >> struct mutex lock; >> struct ap_driver *vfio_ap_drv; >> + struct rw_semaphore guests_lock; >> + struct list_head guests; >> }; >> >> extern struct ap_matrix_dev *matrix_dev;
Powered by blists - more mailing lists