Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Tian, Kevin" <kevin.tian@intel.com>
To:     Alex Williamson <alex.williamson@redhat.com>
CC:     Zhenyu Wang <zhenyuw@linux.intel.com>,
        "Wang, Zhi A" <zhi.a.wang@intel.com>,
        Jani Nikula <jani.nikula@linux.intel.com>,
        "Joonas Lahtinen" <joonas.lahtinen@linux.intel.com>,
        "Vivi, Rodrigo" <rodrigo.vivi@intel.com>,
        Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Eric Farman <farman@linux.ibm.com>,
        Matthew Rosato <mjrosato@linux.ibm.com>,
        Halil Pasic <pasic@linux.ibm.com>,
        Vineeth Vijayan <vneethv@linux.ibm.com>,
        "Peter Oberparleiter" <oberpar@linux.ibm.com>,
        Heiko Carstens <hca@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Alexander Gordeev <agordeev@linux.ibm.com>,
        Christian Borntraeger <borntraeger@linux.ibm.com>,
        Sven Schnelle <svens@linux.ibm.com>,
        Tony Krowiak <akrowiak@linux.ibm.com>,
        Jason Herne <jjherne@linux.ibm.com>,
        Harald Freudenberger <freude@linux.ibm.com>,
        Diana Craciun <diana.craciun@oss.nxp.com>,
        "Cornelia Huck" <cohuck@redhat.com>,
        Longfang Liu <liulongfang@huawei.com>,
        "Shameer Kolothum" <shameerali.kolothum.thodi@huawei.com>,
        Jason Gunthorpe <jgg@ziepe.ca>,
        Yishai Hadas <yishaih@nvidia.com>,
        Eric Auger <eric.auger@redhat.com>,
        Kirti Wankhede <kwankhede@nvidia.com>,
        "Leon Romanovsky" <leon@kernel.org>,
        Abhishek Sahu <abhsahu@nvidia.com>,
        "Christoph Hellwig" <hch@infradead.org>,
        "intel-gvt-dev@lists.freedesktop.org" 
        <intel-gvt-dev@lists.freedesktop.org>,
        "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>,
        "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-s390@vger.kernel.org" <linux-s390@vger.kernel.org>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "Liu, Yi L" <yi.l.liu@intel.com>
Subject: RE: [PATCH v3 06/15] vfio/mtty: Use the new device life cycle helpers
Thread-Topic: [PATCH v3 06/15] vfio/mtty: Use the new device life cycle
 helpers
Thread-Index: AQHYw/jlk/cF5qgZnEulnxqLlAKI863ow0eAgAA6zIA=
Date:   Tue, 20 Sep 2022 22:52:57 +0000
Message-ID: <BN9PR11MB5276F8872ABDFAEB8A8C2AE78C4C9@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <20220909102247.67324-1-kevin.tian@intel.com>
        <20220909102247.67324-7-kevin.tian@intel.com>
 <20220920131723.2541b7e8.alex.williamson@redhat.com>
In-Reply-To: <20220920131723.2541b7e8.alex.williamson@redhat.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?pCMg2KVcq4pw3FaK8coDO1/YSdEnCWsDadzLBOlfe+a2pZXhnbybY3/03TT4?=
 =?us-ascii?Q?jpHFnKqVMVf8i8B+ciXMJOITUVl22y9+3U+MSicaOSgCxAxAf2xBC3Dy7g+P?=
 =?us-ascii?Q?wzjCLSzMk0/LO1RE648eARBuY538DT8zk0iClaMzKls4PT8GLnBfIEaJg67r?=
 =?us-ascii?Q?MGKcRcfxP/8FS+4fqggKuLPb/V+qX/0MoiOWyYsQTC85BRg0+9I0cBN2XEMm?=
 =?us-ascii?Q?Gn0QyCMlkJ4cwHQU7ELumw0KeouC6cbzsdymRoaJABIyvFE0GsyMe3zosW49?=
 =?us-ascii?Q?ETtt0UZ2IUT6qWq3vDldFPAD9n4VyfkR0PLbU1Gw/z6tdDgbpJJ7SIP/T3nK?=
 =?us-ascii?Q?Q9B9zUspy4/vmlwYPvhxRSlCdkHPi1sHu3OYLEOW8KIwRGF0+0I54bNmaIWp?=
 =?us-ascii?Q?DGUcW2B2ciFsyeYnIvE6U7bQ8LeO+EMZ58XlcKmYiGnXcQFOvxJcwOu428kl?=
 =?us-ascii?Q?0THDIFp3Ou/4KgsV0iaFrEnQO8NVq0m4mFQa7j+BNGfZDNqQpXugxaAIPREF?=
 =?us-ascii?Q?+JNr3h9gObYWjdtxPy1GwuRhcQMPupN9RlmnaG1yFrcGcW1c7PCRzfxAGta/?=
 =?us-ascii?Q?gDLPPkhXha2dxyhnVW/SIwbNPUmodjkACnrk15lVpqV4JDw3NS9XfCSTMWKG?=
 =?us-ascii?Q?J/8T3PB1mObDOPUAmymPNjTRwoA8kuepNqrlb/B9nk6nGr3+TuYCkCwsOx5A?=
 =?us-ascii?Q?KmdTFfVa6svL9T5Fg+dSKWFnOqdtJm+QhGf3zoTt3fH//SqxNcD2ZibPaGkg?=
 =?us-ascii?Q?J2oyiYuTpQ7zZR34apXfZNDnlcOTB/Y8j81s3oS2Tq4jBzzIM91pwBLEtu3k?=
 =?us-ascii?Q?yBy+S3qf8lb5JR+144235ZQ1q5bRQ+wNbv3WxeTHMII+c+oSgg1Rnq02C1Kc?=
 =?us-ascii?Q?T9kdnsVYmVnWUdLJJ1ehQjO1ur9+uZn1AVlnJQE3ij3SyG+OGeGnj4uRnUYu?=
 =?us-ascii?Q?ZqUGHe1BVM/JSbsSr5uHBjpeqgnLxLXgr5bsO6BBBaV0b6NOOESQG+HcxqqH?=
 =?us-ascii?Q?/8DYXIgFRUT+ikVy3X+yQuYcbh5bLd6Vjzsml1ss9iaBxOaB0Ix/PX87cH5f?=
 =?us-ascii?Q?4mHd6daIaC+rtIlYuvrngimLvSn2utSQ29zfSeM0w08LUatsdjdm7JbZMdwN?=
 =?us-ascii?Q?LsCgXGaf4gVqVAWK4bZikQW6ceiL6pG72wc4ELr073yFJ+KxoKnY/WpesSQm?=
 =?us-ascii?Q?zBo0dUWm9VwKQnfrr5aWDLDD1Lzs+vQPFxYzkySOYEgrF7GrFM3bWr5ZIpYS?=
 =?us-ascii?Q?7IcboInkHSRB26CLrfRQ9U8F/qa72Wt3Kz6xA8ivAQYCvasgN/3GJZ2nKX4I?=
 =?us-ascii?Q?XUVvnGAQh7FsXKzWy418rh3ovBGz+BR7rWp2RC2l9wBfaotfCkllq3I8zmSw?=
 =?us-ascii?Q?0lmpzYJxZx0RCrnctirsw79GyplS+fdXEfyIMoCwXKyyFpILn4j4H5n/i4R1?=
 =?us-ascii?Q?YssVLhp3Neeck/zTi4F1vDXoLETZxTzHg4kYKTIPhqLuFM8W9c0HStKXpz1t?=
 =?us-ascii?Q?/LILLY24C8lCb/PxE0xExt5eZu2WDgfID0e+ednENjSH18j/UtVkJJOBh3OE?=
 =?us-ascii?Q?8CyD5XjfnyIgBvKL4FoX5qFemSYRkYf6Quroo+RH?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN9PR11MB5276.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c28dd072-8239-4b06-15db-08da9b5adc9f
X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Sep 2022 22:52:57.6809
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 8XVnvjlRAvyWUgFS6YaGk2enEuJdQsvYtFFHBvDhTt/Ayi4E75IqNUK2y+037DeAPgd/lIqene+gqfZ9Qe8iyg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5404
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,
        SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> From: Alex Williamson <alex.williamson@redhat.com>
> Sent: Wednesday, September 21, 2022 3:17 AM
>=20
> On Fri,  9 Sep 2022 18:22:38 +0800
> Kevin Tian <kevin.tian@intel.com> wrote:
>=20
> > From: Yi Liu <yi.l.liu@intel.com>
> >
> > and manage available ports inside @init/@release.
> >
> > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > Signed-off-by: Kevin Tian <kevin.tian@intel.com>
> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> >  samples/vfio-mdev/mtty.c | 67 +++++++++++++++++++++++-----------------
> >  1 file changed, 39 insertions(+), 28 deletions(-)
> >
> > diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
> > index f42a59ed2e3f..41301d50b247 100644
> > --- a/samples/vfio-mdev/mtty.c
> > +++ b/samples/vfio-mdev/mtty.c
> ...
> > +static int mtty_probe(struct mdev_device *mdev)
> > +{
> > +	struct mdev_state *mdev_state;
> > +	int ret;
> > +
> > +	mdev_state =3D vfio_alloc_device(mdev_state, vdev, &mdev->dev,
> > +				       &mtty_dev_ops);
> > +	if (IS_ERR(mdev_state))
> > +		return PTR_ERR(mdev_state);
> >
> >  	ret =3D vfio_register_emulated_iommu_dev(&mdev_state->vdev);
> >  	if (ret)
> > -		goto err_vconfig;
> > +		goto err_put_vdev;
> >  	dev_set_drvdata(&mdev->dev, mdev_state);
> >  	return 0;
> >
> > -err_vconfig:
> > -	kfree(mdev_state->vconfig);
> > -err_state:
> > -	vfio_uninit_group_dev(&mdev_state->vdev);
> > -	kfree(mdev_state);
> > -err_nr_ports:
> > -	atomic_add(nr_ports, &mdev_avail_ports);
> > +err_put_vdev:
> > +	vfio_put_device(&mdev_state->vdev);
> >  	return ret;
> >  }
> >
> > +static void mtty_release_dev(struct vfio_device *vdev)
> > +{
> > +	struct mdev_state *mdev_state =3D
> > +		container_of(vdev, struct mdev_state, vdev);
> > +
> > +	kfree(mdev_state->vconfig);
> > +	vfio_free_device(vdev);
> > +	atomic_add(mdev_state->nr_ports, &mdev_avail_ports);
>=20
> I must be missing something, isn't this a use-after-free?

Yes, it's a use-after-free indeed. Thanks for catching it!

>=20
> mdev_state is allocated via vfio_alloc_device(), where vdev is the
> first entry in that structure, so this is equivalent to
> kvfree(mdev_state).  mbochs has the same issue.  mdpy and vfio-ap
> adjust global counters after vfio_free_device(), which I think muddies
> the situation.  Shouldn't we look suspiciously at any .release callback
> where vfio_free_device() isn't the last thing executed?  Thanks,
>=20

Yes. I'll scrutinize it again. To be consistent I'll make sure the free
is the last line in all .release callbacks though not required for
some (e.g. mdpy and vfio-ap).