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

[Tony Krowiak]

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