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>,
        Matthew Rosato <mjrosato@linux.ibm.com>
CC:     "akrowiak@linux.ibm.com" <akrowiak@linux.ibm.com>,
        "jjherne@linux.ibm.com" <jjherne@linux.ibm.com>,
        "farman@linux.ibm.com" <farman@linux.ibm.com>,
        "imbrenda@linux.ibm.com" <imbrenda@linux.ibm.com>,
        "frankja@linux.ibm.com" <frankja@linux.ibm.com>,
        "pmorel@linux.ibm.com" <pmorel@linux.ibm.com>,
        "david@redhat.com" <david@redhat.com>,
        "Christopherson,, Sean" <seanjc@google.com>,
        "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>,
        "cohuck@redhat.com" <cohuck@redhat.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "zhenyuw@linux.intel.com" <zhenyuw@linux.intel.com>,
        "pasic@linux.ibm.com" <pasic@linux.ibm.com>,
        "jgg@nvidia.com" <jgg@nvidia.com>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "pbonzini@redhat.com" <pbonzini@redhat.com>,
        "linux-s390@vger.kernel.org" <linux-s390@vger.kernel.org>,
        "borntraeger@linux.ibm.com" <borntraeger@linux.ibm.com>,
        "Liu, Yi L" <yi.l.liu@intel.com>,
        "intel-gvt-dev@lists.freedesktop.org" 
        <intel-gvt-dev@lists.freedesktop.org>,
        "Wang, Zhi A" <zhi.a.wang@intel.com>
Subject: RE: [PATCH v4] vfio: fix potential deadlock on vfio group lock
Thread-Topic: [PATCH v4] vfio: fix potential deadlock on vfio group lock
Thread-Index: AQHZJ6vANRoQ9x2rxUaXBLM6X5ngE66jJF8AgADCPMA=
Date:   Wed, 18 Jan 2023 09:03:30 +0000
Message-ID: <BN9PR11MB52763D861C254248FD33F65C8CC79@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <20230114000351.115444-1-mjrosato@linux.ibm.com>
 <20230117142252.70cc85c7.alex.williamson@redhat.com>
In-Reply-To: <20230117142252.70cc85c7.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?w16QGxzuI6P3GQ9yGqXexPojEepje9l3Q5puR+NO9Z6cU9OgVEkE3R/GDA4B?=
 =?us-ascii?Q?lVp7FXyyYILcsd8p5QW3tq4xEKXISRMrVsBAYTYhmVImyrP55W5TVPSbUos6?=
 =?us-ascii?Q?Pr3NqQYeRqW4Ye3D2YpolsdnVaIe1lHk0PoB4sa5Zx3200Mlmc+2WJYSnPl8?=
 =?us-ascii?Q?JXpBaIhd2Uykw+Xk1kHIp7MsC6mHAl8XzzjP+P5MRc5YozuqWGlsRhtdmIcg?=
 =?us-ascii?Q?8fSbmwtXniZ34jB03Q899mBqNeDYMQWFsJGs2s1TwtRPXf2IweMlhmuZr1E9?=
 =?us-ascii?Q?h4mVUPCJLvEc9IH0EsbOGmLz54Lo99id2MSddWXKO0NMxT8xp6SmrB5DLFDJ?=
 =?us-ascii?Q?8e3di8mbMheIgWeA6frevWtGBSoExi1KzYDEQ/meyqexldpC0e4QNG1KcveZ?=
 =?us-ascii?Q?mXzVIpAL9+SlHSEp+ja9Y7iRPwzHrnE3mGoQx71gTC/5K0kyPb2J2fOdKOTr?=
 =?us-ascii?Q?8UaMUpUPIBsHT2IZlKXgL9c3Bhpogh7dZyQRPdGAkqfu0ct0kzDj/fVMovoD?=
 =?us-ascii?Q?rI7+HdSQW+kRLkwjFVUtD3lkRSh70wHU3aA7l23sLwCCCnTmf8rF8qW/iV4z?=
 =?us-ascii?Q?+2V25MsH6aM10l82TuLowsXwH3MvIYTcUwHjXqF9PjjijSPi151NM31C6Ash?=
 =?us-ascii?Q?mXEqVjDrBEm6VYXvBwkasPQqpUWo18YpBIighi7x/qdyJqw+O+hCANq412v7?=
 =?us-ascii?Q?n5T0gSpuMsBClRnvrHPUJ6g87RZTj5nuY69Xsu2/Mtu/l3rvISRHoODXMq1A?=
 =?us-ascii?Q?1/gzv3pl8FiGYTB7kyoODPDpTgxMNWOuMqjCBlNXTU4W1Rx7vQlCkuna+kJH?=
 =?us-ascii?Q?qACk91TgYbdD82k7WKqy1L+6s7Vj7KANQzLKh6Wdbsx7whp92V9rLPsk1giS?=
 =?us-ascii?Q?4zmwTFd/juas91SVIOtYDgu3k/bqDfxj2q0w7C12VI0VtC2UqFdyDxdmPGlT?=
 =?us-ascii?Q?q6o/AzKocqHxcMjljh3DpvMcvE4T6Wk0SHVwAaf92XbqmDKnr8hC36K5eVNQ?=
 =?us-ascii?Q?I9Y06OLB39htpA7VbrKZzKGb5OJiTkpFPLFe7dTlpXAmRttzliUZ1In7KkCp?=
 =?us-ascii?Q?gQODpVvgAPqe/1xv+kV8EpUaLKLsxsiNUTUtjm8j39vYwLcWGSkTDWQKu0nG?=
 =?us-ascii?Q?91mt6P1k//3004O2JN1HSQwiC9QPCbm1iaQkfxX+4QSFZirIntNVfMxmXhgq?=
 =?us-ascii?Q?6CKvx53Xh9GncqrAOBTFOkKcLIYbO0fdMIatcM2RBaASgv6/sZgqy+LEui17?=
 =?us-ascii?Q?+c1LA0LzMNIrB7tcXs5Yi2uZEc9qSFe9RWuVWG7+bWxbcZZ50rtMhTlV/1Tx?=
 =?us-ascii?Q?6pzeGxiG7zB3T4eHCLWeql8gPfo2sEdugyB1HUU+Tj3xrGyaPUmGH6Z8E80T?=
 =?us-ascii?Q?cXjzGs3WhT9ZAlFeRGXrGyxYZ5bVFIE7D367D80ZTcj69dLwLurkz3/E022Z?=
 =?us-ascii?Q?qDmJHZr7u0HOGkPBTDix7Jkdkr/jKoWroUGAJLLFvYkKjoImtYLlpfa/ziKc?=
 =?us-ascii?Q?6zsj78Riq8MGq3Eh4s1OxeedxGe0o5IQIMS/63TndVuopMNwGqpbEYJa56s1?=
 =?us-ascii?Q?D3OOv2Kjxb/6RC0CEeikAGzY7qAXhwsEEEtFOGeR?=
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: c6fbaf06-41dd-45cc-1238-08daf932de9e
X-MS-Exchange-CrossTenant-originalarrivaltime: 18 Jan 2023 09:03:30.4772
 (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: elifOfvBBwlgjVVY5I6EnPM0MjpHo6XHlBTiujoXoI2Z23M9biv45Ubh8gQqGDxMiPECbeqkKzcOCSAuJWrJug==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB4831
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        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
> Sent: Wednesday, January 18, 2023 5:23 AM
>=20
> On Fri, 13 Jan 2023 19:03:51 -0500
> Matthew Rosato <mjrosato@linux.ibm.com> wrote:
>=20
> >  void vfio_device_group_close(struct vfio_device *device)
> >  {
> > +	void (*put_kvm)(struct kvm *kvm);
> > +	struct kvm *kvm;
> > +
> >  	mutex_lock(&device->group->group_lock);
> > +	kvm =3D device->kvm;
> > +	put_kvm =3D device->put_kvm;
> >  	vfio_device_close(device, device->group->iommufd);
> > +	if (kvm =3D=3D device->kvm)
> > +		kvm =3D NULL;
>=20
> Hmm, so we're using whether the device->kvm pointer gets cleared in
> last_close to detect whether we should put the kvm reference.  That's a
> bit obscure.  Our get and put is also asymmetric.
>=20
> Did we decide that we couldn't do this via a schedule_work() from the
> last_close function, ie. implementing our own version of an async put?
> It seems like that potentially has a cleaner implementation, symmetric
> call points, handling all the storing and clearing of kvm related
> pointers within the get/put wrappers, passing only a vfio_device to the
> put wrapper, using the "vfio_device_" prefix for both.  Potentially
> we'd just want an unconditional flush outside of lock here for
> deterministic release.
>=20
> What's the downside?  Thanks,
>=20

btw I guess this can be also fixed by Yi's work here:

https://lore.kernel.org/kvm/20230117134942.101112-6-yi.l.liu@intel.com/

with set_kvm(NULL) moved to the release callback of kvm_vfio device,
such circular lock dependency can be avoided too.