From:   "Tian, Kevin" <kevin.tian@intel.com>
To:     Alex Williamson <alex.williamson@redhat.com>,
        Matthew Rosato <mjrosato@linux.ibm.com>
CC:     "pbonzini@redhat.com" <pbonzini@redhat.com>,
        "Liu, Yi L" <yi.l.liu@intel.com>,
        "jgg@nvidia.com" <jgg@nvidia.com>,
        "cohuck@redhat.com" <cohuck@redhat.com>,
        "farman@linux.ibm.com" <farman@linux.ibm.com>,
        "pmorel@linux.ibm.com" <pmorel@linux.ibm.com>,
        "borntraeger@linux.ibm.com" <borntraeger@linux.ibm.com>,
        "frankja@linux.ibm.com" <frankja@linux.ibm.com>,
        "imbrenda@linux.ibm.com" <imbrenda@linux.ibm.com>,
        "david@redhat.com" <david@redhat.com>,
        "akrowiak@linux.ibm.com" <akrowiak@linux.ibm.com>,
        "jjherne@linux.ibm.com" <jjherne@linux.ibm.com>,
        "pasic@linux.ibm.com" <pasic@linux.ibm.com>,
        "zhenyuw@linux.intel.com" <zhenyuw@linux.intel.com>,
        "Wang, Zhi A" <zhi.a.wang@intel.com>,
        "Christopherson,, Sean" <seanjc@google.com>,
        "linux-s390@vger.kernel.org" <linux-s390@vger.kernel.org>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "intel-gvt-dev@lists.freedesktop.org" 
        <intel-gvt-dev@lists.freedesktop.org>,
        "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v3] vfio: fix deadlock between group lock and kvm lock
Thread-Topic: [PATCH v3] vfio: fix deadlock between group lock and kvm lock
Thread-Index: AQHZNyMj/DyFlYgOy0urx1B0EjoKIa68DpoAgAA1mqA=
Date:   Thu, 2 Feb 2023 23:04:10 +0000
Message-ID: <BN9PR11MB527618E281BEB8E479ABB0418CD69@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <20230202162442.78216-1-mjrosato@linux.ibm.com>
 <20230202124210.476adaf8.alex.williamson@redhat.com>
In-Reply-To: <20230202124210.476adaf8.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?zcNDVPKE6Lhp0zSTQtQ9dBUbmntlY+ONfpNFqmlMZxTDEsNRougQGGjPJ/ai?=
 =?us-ascii?Q?PLAuEnO2Rf1/GIcNF0CPLP32D30xO7y/BM41KN/ErVbdiRftETTe1Iy4bpb/?=
 =?us-ascii?Q?K5dK8doAew0s7OkqRio0c9z8hH5HMmXKSUP46wDcmC61SxECEtncasrDjnc3?=
 =?us-ascii?Q?+zdt5GUsy4soY9fn0lAnJiQH6i5J+WTVCawSDJvlUiuRb218Rchn4jyRA/Dr?=
 =?us-ascii?Q?mX36SN/AQpDL/yvjwbgQLWkvSTZEIKuhpapgEFIyHjuNZiHJ108raqxo7BHR?=
 =?us-ascii?Q?v51zssdRxdAuwzfCtQrNtDvdpnzaj0l06Zp0c7i9vFtj9y18Q/xKDFG7zZI3?=
 =?us-ascii?Q?g7RMeTSqExYOT7Sg+2JRUZ+MUCzGESd+47TlSPOk4tgiwLZB0V6xapLNVY5j?=
 =?us-ascii?Q?E9ZLbIz+RjgUsF0nt5Q/ICGX+12BIE98vpApmo8MIb7Zta4uob9RwVmzfYM1?=
 =?us-ascii?Q?jj9nMcMuWqWAnvfqIM9R8eQf8+1EAcajvmF1TuzLVXH9QHmme/SiQMu8clus?=
 =?us-ascii?Q?unuj0B8UXH6hNOH5Oq9uzWH5jMCdyPUj0IV2A1CQfFw6ZywJkQLD4Bcv7BFm?=
 =?us-ascii?Q?65txeOnxHG+CJJHreaaIQJGgJAEL64k9GbtZy32dpIiGTztP0ThaS0Se6359?=
 =?us-ascii?Q?tLO3oIJXMI6+kXpmY+mckF2n1WZQd+eZmU9pYXI/Yc2bg6GJtzMybHrbfucv?=
 =?us-ascii?Q?4Jy/kfXTBreaqfzRtDObeKwmqzG3wvRzjRRn/w5q+FMQO58EfwmGJ3Z60R/e?=
 =?us-ascii?Q?oOGEkRWoc3G/hz2PEAOi0e8YUrE/YqLM8jyrguHYee0fOtxXYUSr4Cpp2jnS?=
 =?us-ascii?Q?TLOUvy5vfhXT7YcF0ckta7mppFm/mVz5VlUCbHPdyKiyzohiFHxOCc7qTu/E?=
 =?us-ascii?Q?I/bPboJKI12nJCb4SN9dMtcD07mkHa8swaJRahF4gj7G+icglFQ5PZkK1SMw?=
 =?us-ascii?Q?PTysxpIIEuLXBjlxuOptMA0D32AplANrf+cDB0I3DErYPmUDYYVxhDMN8YWV?=
 =?us-ascii?Q?pSGqjuhd95XtXKeQhb1hzXffo8ambvOShG8w6XaRKDeLoVzYB2GjHm9XwhKb?=
 =?us-ascii?Q?d2cnc4fMIr5nULnEYLYyYShrw4TBLbRbuYIrB9rqJAH/lv51UW+i5MAS31CN?=
 =?us-ascii?Q?rXhCcR3GO+nd7gteYVYkQC7RDIpEqOSo8yn1lqkqWZ/vk0siOo5QSttmsXgP?=
 =?us-ascii?Q?OY4Y7Lj+Yxe39TMFi13lQ2Yructt4LMc3q1LFvxpSBdxr4h6lixUSC9McrIM?=
 =?us-ascii?Q?fLPBUxpbOkC1m5vTirlJ6WITMvJUg6LCusBpQJRkip2ddZpIzligc/wPQ3U8?=
 =?us-ascii?Q?kq/pIoZe5kLeHS9zI0rZ6+tsMX/iOnA4s7LpqJMygnCBeMkIAE1+oAOZzzGV?=
 =?us-ascii?Q?R0Hj/vsRnKzacJ9YH/KoDGj2HJLMHEGA35Qb2677ee5i0Cz8SLRIFMqdQryW?=
 =?us-ascii?Q?s5KHuYbsXuo/+hMSRDXE/idxmAH8mSNSqaE8TYuokkx28BEdfaud8zBcAYu5?=
 =?us-ascii?Q?rmSIf7dIoaaKoaUGMqf4MZ6KdbCbbqnJkYXtJrMpFQyusVYOGWDNPl15Nviq?=
 =?us-ascii?Q?SbSnY15c/Zg6oBFs94MUZYepr+Js2zyD18s2yIHm?=
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: da9353e5-7eb1-41c3-4517-08db0571cb2d
X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Feb 2023 23:04:10.0617
 (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: 6O95gKukaSnRu6TBdcPRyB1SEXbO/HiRqbehD7aBKeOo1NIVzUluFHzHIDrZ7oz8bT5C36t9DQx1Ac0FlnSKDw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7146
X-OriginatorOrg: intel.com
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: Friday, February 3, 2023 3:42 AM
>=20
> On Thu,  2 Feb 2023 11:24:42 -0500
> Matthew Rosato <mjrosato@linux.ibm.com> wrote:
>=20
> > After 51cdc8bc120e, we have another deadlock scenario between the
> > kvm->lock and the vfio group_lock with two different codepaths acquirin=
g
> > the locks in different order.  Specifically in vfio_open_device, vfio
> > holds the vfio group_lock when issuing device->ops->open_device but
> some
> > drivers (like vfio-ap) need to acquire kvm->lock during their open_devi=
ce
> > routine;  Meanwhile, kvm_vfio_release will acquire the kvm->lock first
> > before calling vfio_file_set_kvm which will acquire the vfio group_lock=
.
> >
> > To resolve this, let's remove the need for the vfio group_lock from the
> > kvm_vfio_release codepath.  This is done by introducing a new spinlock =
to
> > protect modifications to the vfio group kvm pointer, and acquiring a kv=
m
> > ref from within vfio while holding this spinlock, with the reference he=
ld
> > until the last close for the device in question.
> >
> > Fixes: 51cdc8bc120e ("kvm/vfio: Fix potential deadlock on vfio group_lo=
ck")
> > Reported-by: Anthony Krowiak <akrowiak@linux.ibm.com>
> > Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> > ---
> > Changes from v2:
> > * Relocate the new functions back to vfio_main and externalize to call
> >   from group (Kevin) since cdev will need this too
> > * s/vfio_kvm_*_kvm/vfio_device_*_kvm/ and only pass device as input.
> >   Handle new kvm_ref_lock directly inside vfio_device_get_kvm (Alex)
> > * Add assert_lockdep_held for dev_set lock (Alex)
> > * Internalize error paths for vfio_device_get_kvm_safe and now return
> >   void - either device->kvm is set with a ref taken or is NULL (Alex)
> > * Other flow suggestions to make the call path cleaner - Thanks! (Alex)
> > * Can't pass group->kvm to vfio_device_open, as it references the value
> >   outside of new lock.  Pass device->kvm to minimize changes in this
> >   fix (Alex, Yi)
> > Changes from v1:
> > * use spin_lock instead of spin_lock_irqsave (Jason)
> > * clear device->kvm_put as part of vfio_kvm_put_kvm (Yi)
> > * Re-arrange code to avoid referencing the group contents from within
> >   vfio_main (Kevin) which meant moving most of the code in this patch
> >   to group.c along with getting/dropping of the dev_set lock
> > ---
> >  drivers/vfio/group.c     | 32 ++++++++++++++----
> >  drivers/vfio/vfio.h      | 14 ++++++++
> >  drivers/vfio/vfio_main.c | 70 ++++++++++++++++++++++++++++++++++++--
> --
> >  include/linux/vfio.h     |  2 +-
> >  4 files changed, 103 insertions(+), 15 deletions(-)
>=20
> LGTM.  I'm not sure moving the functions to vfio_main really buys us
> anything since we're making so much use of group fields.  The cdev
> approach will necessarily be different, so the bulk of the get code will
> likely need to move back to group.c anyway.
>=20

well my last comment was based on Matthew's v2 where the get code
gets a kvm passed in instead of implicitly retrieving group ref_lock
internally. In that case the get/put helpers only contain device logic
thus fit in vfio_main.c.

with v3 then they have to be in group.c since we don't want to use
group fields in vfio_main.c.

but I still think v2 of the helpers is slightly better. The only difference
between cdev and group when handling this race is using different
ref_lock. the symbol get/put part is exactly same. So even if we
merge v3 like this, very likely Yi has to change it back to v2 style
to share the get/put helpers while just leaving the ref_lock part
handled differently between the two path.

Thanks
Kevin