Re: [PATCH v4 0/2] s390/vfio-ap: fix memory leak in mdev remove callback

[Tony Krowiak <akrowiak@linux.ibm.com>]

On 6/14/21 3:51 AM, Christian Borntraeger wrote:
>
> 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.

I'm sorry, it morphed into a different set of patches due to the 
addition of a
patch precipitated by review comments of an unrelated issue. I pushed a
v5 today that contains only the relevant patches. I believe that set can be
integrated.