On 5/25/21 9:03 AM, Halil Pasic wrote:
On Fri, 21 May 2021 15:36:47 -0400 Tony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote:The mdev remove callback for the vfio_ap device driver bails out with -EBUSY if the mdev is in use by a KVM guest. The intended purpose was to prevent the mdev from being removed while in use; however, returning a non-zero rc does not prevent removal. This could result in a memory leak of the resources allocated when the mdev was created. In addition, the KVM guest will still have access to the AP devices assigned to the mdev even though the mdev no longer exists. To prevent this scenario, cleanup will be done - including unplugging the AP adapters, domains and control domains - regardless of whether the mdev is in use by a KVM guest or not. Fixes: 258287c994de ("s390: vfio-ap: implement mediated device open callback") Cc: stable@xxxxxxxxxxxxxxx Signed-off-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxx> Reviewed-by: Cornelia Huck <cohuck@xxxxxxxxxx>AFAIU we all agree that, after patch there is a possibility for an use after free error. I'm a little confused by the fact that we want this one for stable, but the patch that fixes the use after free as no Cc stable (it can't have a proper fixes tag, because this one is not yet merged). Actually I'm not a big fan of splitting up patches to the extent that when not all patches of the series are applied we get bugous behavior (e.g. patch n breaks something that is live at patch n level, but it is supposed to be OK, because patch n+m is going to fix it (where n,m \in \Z^{+}).
After thinking about this some more, this patch does not really fix a memory leak and should probably not be flagged as a fix for 258287c994. Memory is not leaked because the remove callback returns -EBUSY without freeing mdev storage or resetting the queues. Under normal circumstances, if the mdev is removed before the mdev fd is closed (i.e., the guest is shut down), the process will wait until the fd is closed, at which time the release callback will get invoked. Since the release callback clears the KVM pointer from the matrix_mdev, the remove callback will not return -EBUSY and will in fact free the mdev storage when it is subsequently invoked. I am going to change the subject and remove the 'Fixes' tag as well as the 'Cc' of stable. I'll change the subject to something like: "s390/vfio-ap: always free storage for mdev in remove callback"
Do we want to squash these? Is the use after free possible prior to this patch? Regards, Halil
