Re: [PATCH 06/12] vfio/ap_ops: Convert to use vfio_register_group_dev()

From: Jason Gunthorpe <jgg@nvidia.com>
To: Cornelia Huck <cohuck@redhat.com>
Cc: Tony Krowiak <akrowiak@linux.ibm.com>,
	Christian Borntraeger <borntraeger@de.ibm.com>,
	Harald Freudenberger <freude@linux.ibm.com>,
	Vasily Gorbik <gor@linux.ibm.com>,
	Heiko Carstens <hca@linux.ibm.com>,
	linux-s390@vger.kernel.org, Halil Pasic <pasic@linux.ibm.com>,
	Pierre Morel <pmorel@linux.ibm.com>,
	"Raj, Ashok" <ashok.raj@intel.com>,
	Dan Williams <dan.j.williams@intel.com>,
	Daniel Vetter <daniel@ffwll.ch>, Christoph Hellwig <hch@lst.de>,
	Leon Romanovsky <leonro@nvidia.com>,
	Max Gurtovoy <mgurtovoy@nvidia.com>,
	Tarun Gupta <targupta@nvidia.com>
Subject: Re: [PATCH 06/12] vfio/ap_ops: Convert to use vfio_register_group_dev()
Date: Mon, 26 Apr 2021 15:10:57 -0300	[thread overview]
Message-ID: <20210426181057.GX1370958@nvidia.com> (raw)
In-Reply-To: <20210426194859.1665730d.cohuck@redhat.com>

On Mon, Apr 26, 2021 at 07:48:59PM +0200, Cornelia Huck wrote:
> On Fri, 23 Apr 2021 20:03:03 -0300
> Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > This is straightforward conversion, the ap_matrix_mdev is actually serving
> > as the vfio_device and we can replace all the mdev_get_drvdata()'s with a
> > simple container_of().
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> >  drivers/s390/crypto/vfio_ap_ops.c     | 137 ++++++++++++++++----------
> >  drivers/s390/crypto/vfio_ap_private.h |   2 +
> >  2 files changed, 89 insertions(+), 50 deletions(-)
> > 
> 
> (...)
> 
> > -static int vfio_ap_mdev_remove(struct mdev_device *mdev)
> > +static void vfio_ap_mdev_remove(struct mdev_device *mdev)
> >  {
> > -	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
> > +	struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(&mdev->dev);
> >  
> > -	if (matrix_mdev->kvm)
> > -		return -EBUSY;
> > +	/* FIXME: Remove isn't allowed to fail */
> > +	if (WARN_ON(matrix_mdev->kvm))
> > +		return;
> 
> This is a pre-existing problem, but the rework now makes it more
> obvious.
> 
> Previously, the mdev code would only print a warning and then continue
> with device removal, even if a ->remove() callback returned an
> error.

This does mostly the same, the warning was just moved from
mdev_device_remove_common() to here and changed to a WARN_ON() because
it means we are permanently leaking kernel memory. I think in this
case the vfio_device is not deleted - though I could re-order this to
make that happen.

> Now, it's quite clear that we'll end up in a weird half-dead state.

I don't think it changes, after we print the WARN_ON we return to the
driver core which does the same put_device()/etc as
mdev_device_remove_common() was doing.

> IIRC, the check for matrix_mdev->kvm is intended to protect against
> ripping out the device under a running guest (I think it needs to
> manipulate some crypto control blocks?)

In that case it is missing locking too.

Jason