Messages in this thread | | | Subject | Re: [PATCH v4 0/2] s390/vfio-ap: fix memory leak in mdev remove callback | From | Christian Borntraeger <> | Date | Mon, 14 Jun 2021 09:51:12 +0200 |
| |
On 21.05.21 21:36, Tony Krowiak wrote: > Fixes a memory leak in the mdev remove callback when invoked while the > mdev is in use by a KVM guest. Instead of returning -EBUSY from the > callback, a full cleanup of the resources allocated to the mdev is > performed because regardless of the value returned from the function, the > mdev is removed from sysfs. > > The cleanup of resources allocated to the mdev may coincide with the > interception of the PQAP(AQIC) instruction in which case data needed to > handle the interception may get removed. A patch is included in this series > to synchronize access to resources needed by the interception handler to > protect against invalid memory accesses. > > The first pass (PATCH v3) at trying to synchronize access to the pqap > function pointer employed RCU. The problem is, the RCU read-side critical > section would have to include the execution of the pqap function which > sleeps; RCU disallows sleeping inside an RCU region. When I subsequently > tried to encompass the pqap function within the > rcu_read_lock/rcu_read_unlock, I ended up seeing lockdep warnings in the > syslog. > > It was suggested that we use an rw_semaphore to synchronize access to > the pqap hook, but I also ran into similar lockdep complaints something > like the following: > > Possible unsafe locking scenario: > > CPU0 CPU1 > ---- ---- > down_read(&rwsem) > in handle_pqap (priv.c); > > lock(&matrix_dev->lock); > in vfio_ap_mdev_set_kvm (vfio_ap_ops.c) > > down_write(&rwsem; > in vfio_ap_mdev_set_kvm (vfio_ap_ops.c) > > lock(&matrix_dev->lock); > in handle_pqap(vfio_ap_ops.c) > > Access to the mdev must be done under the matrix_dev->lock to ensure that > it doesn't get freed via the remove callback while in use. This appears > to be mutually exclusive with setting/unsetting the pqap_hook pointer > due to lockdep issues. > > The solution: > ------------ > The lifetime of the handle_pqap function (vfio_ap_ops) is syncrhonous > with the lifetime of the vfio_ap module, so there really is not reason > to tie the setting/clearing of its function pointer with the lifetime > of a guest or even an mdev. If the function pointer is set when the > vfio_ap module is loaded and cleared when the vfio_ap module is unloaded, > then access to it can be protected independently from mdev creation or > removal as well as the starting or shutdown of a guest. As long as > access to the mdev is always controlled by the matrix_dev->lock, the > mdev can not be freed without other functions being aware. > > Change log: > v3 -> v4: > -------- > * Created a registry for crypto hooks in priv.c with functions for > registering/unregistering function pointers in kvm_host.h (for s390). > > * Register the function pointer for handling the PQAP instruction when > the vfio_ap module is loaded and unregister it when the module is > unloaded.
Was there a v5? I cannot find it.
| |