From:   "Liu, Yi L" <yi.l.liu@intel.com>
To:     "Tian, Kevin" <kevin.tian@intel.com>,
        Alex Williamson <alex.williamson@redhat.com>
CC:     Matthew Rosato <mjrosato@linux.ibm.com>,
        "pbonzini@redhat.com" <pbonzini@redhat.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: AQHZNyMjAB7EXHsi0Ue5s99XWjtHT668DpoAgAA4cQCAAAKAgIAALrmAgAC0yrA=
Date:   Fri, 3 Feb 2023 13:32:09 +0000
Message-ID: <DS0PR11MB7529050661FCE4A5AC4B17C3C3D79@DS0PR11MB7529.namprd11.prod.outlook.com>
References: <20230202162442.78216-1-mjrosato@linux.ibm.com>
        <20230202124210.476adaf8.alex.williamson@redhat.com>
        <BN9PR11MB527618E281BEB8E479ABB0418CD69@BN9PR11MB5276.namprd11.prod.outlook.com>
 <20230202161307.0c6aa23e.alex.williamson@redhat.com>
 <BN9PR11MB5276017F9CEBB4BAE58C40E88CD79@BN9PR11MB5276.namprd11.prod.outlook.com>
In-Reply-To: <BN9PR11MB5276017F9CEBB4BAE58C40E88CD79@BN9PR11MB5276.namprd11.prod.outlook.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?MMqvv9lX8DfALrqTBktzbO2c+vx8doS6qLbc+tIJyupJPb1Ogu9diKaZ5dj8?=
 =?us-ascii?Q?xhv0X+JszbTWjbZaLEawhwn7nZykTKcuO9aNToKuw33Td0ZhexV21yVkjlD+?=
 =?us-ascii?Q?tNOHb9QbpBV6DPp3a0qrWeXBtRNtLL60ts77lHEiE29bPYk7+ksQ4PjpNlTO?=
 =?us-ascii?Q?FF8Z1g1Jdj7YkR+ZulE5D3+74s1crNUEzoun9X868w0dkeLtShTtEDuDr2/O?=
 =?us-ascii?Q?JpcnGCTS9xiUAbmXsu7cJ47rDSPnwQgCQROJ2nW4aNHoT0gkgGrWRNJE8q0x?=
 =?us-ascii?Q?tye2g3JqomZ1urTRg5a1xovW7Wg7WrJFfD6m1xZE/+HeJQza4lSBoTfAN2LN?=
 =?us-ascii?Q?37BsyWri2obEiYa9Rx0NzYCy76GQYN5UrzCHlhJ+IJYN3HAj06sLdB4pGzOJ?=
 =?us-ascii?Q?6Ea5sflhvgMFiS2Owqw2v0hacoYtEt9dL7TrH4bqEdBJZqvgtFn341UmqkzQ?=
 =?us-ascii?Q?85+Z/hd0pDrvhssdzIxzYe62xHb0ihSUCjsL9l2TssZuOffTR3c5WkM/kpYU?=
 =?us-ascii?Q?2ByCpU9FhXjH4DRbM6FPdivjdueg10JtSUta6cN76CygC4FORQ+5V2raeSGs?=
 =?us-ascii?Q?Nf8TOsvttBbwNjUMukj1QvoTyiCgJeVCcf1g4V7TVI0OVWPvanGZL0jQVLgh?=
 =?us-ascii?Q?ISLUd0L6uwUdv7quOHtSf+hZAanz885tFnaUp7i5V8aI1pHmHXCwbTDerdvi?=
 =?us-ascii?Q?a/o2vPKYEIlAjM0NPhrhF0XCVrcUDRXWKS18Nfk61SKxe8BWcLCKOYu5IWjR?=
 =?us-ascii?Q?AYjAFCq362HuKeVHPE2YlSFuQPv5NyagbtZA57RJAxuPRleCwPSIinXzs7wc?=
 =?us-ascii?Q?A2paRB2wmCLupvLS0X9hEh3Z41IZRZ/ut5Dj37EccPY3tR2MZ9c4v+85GaXy?=
 =?us-ascii?Q?aJGRqM4k7fyCYw6h0cnO/21qU5BuDBcPB64rjyrJ1BdbQGGmexF6aD7kkJ/y?=
 =?us-ascii?Q?DAuttIuwYvQqHHnzdTX8is+nP/Cqhf2qyppV3pQ9lW0Q2Y8ocaHeQSN3uEEH?=
 =?us-ascii?Q?6FVa23Q66ujafv2rNIsRAtTns8qlv3e/yc0XT9z50cocV+RGLbQRtfLIb1f1?=
 =?us-ascii?Q?DMD/PDycJrrqyhdrhAajz4p0iDuZ+kXdfDQ2OjFoYOFLFD5IWsTlONwHUokK?=
 =?us-ascii?Q?BaJr2BXS5Tf72Y3hDAMM/pdvL8OWxPrLMDXnuXp2Zj5GnEPeZ0fSaxGeCOSn?=
 =?us-ascii?Q?92DDFjeQo/lIIwsQ6KpCv/5GgfAroh7p2Mg8YB/5q1oikRH4FNE54n8zQuT+?=
 =?us-ascii?Q?6X6q2acemtVLU0FB7rdTmk8tXoM+gRx3AE0AUcP3iiaUsaLG8hYd7Cn1njTz?=
 =?us-ascii?Q?DMCVxZ5jjk+9CpvXKNASIl0qdTJO9QIs4W6PqfwT1pGkKLF81PaRssnuEkK/?=
 =?us-ascii?Q?j5zxACC8+RJFefYlKJxA/daa5989JGurqLb6dhdses8QJ/dcD59J2KpFNQxO?=
 =?us-ascii?Q?xsBuJ1zpvCinT2R3HZucI8JH4uWP5HIpFDlMcT6LnGaaDf4hyhTAA6Ig1so0?=
 =?us-ascii?Q?GZV4OQS/pdYt3SWKXMqYyLnyz10xdGdxbKZR+SNkL5qLr0qCNJzZB3uEjr4r?=
 =?us-ascii?Q?9XMQDbvR/JbfoVo4N2JZRe4Kwj9hdpnoSv5w7QRi?=
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: DS0PR11MB7529.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 830811e4-64ee-44f8-25e1-08db05eb0d36
X-MS-Exchange-CrossTenant-originalarrivaltime: 03 Feb 2023 13:32:10.0006
 (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: eOWyhbNkXAFLBtsk5ScOUuzT+x3cze8LH7JkVT3pmsJQGNlIIhh6MXLB6WLb4hvsG8/guqIclKjQjrhanMG5lw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR11MB7799
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> From: Tian, Kevin <kevin.tian@intel.com>
> Sent: Friday, February 3, 2023 10:00 AM
>=20
> > From: Alex Williamson <alex.williamson@redhat.com>
> > Sent: Friday, February 3, 2023 7:13 AM
> >
> > On Thu, 2 Feb 2023 23:04:10 +0000
> > "Tian, Kevin" <kevin.tian@intel.com> wrote:
> >
> > > > From: Alex Williamson <alex.williamson@redhat.com>
> > > > Sent: Friday, February 3, 2023 3:42 AM
> > > >
> > > >
> > > > LGTM.  I'm not sure moving the functions to vfio_main really buys u=
s
> > > > 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.
> > > >
> > >
> > > 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 logi=
c
> > > 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 diff=
erence
> > > 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.
> >
> > I'm not really a fan of the asymmetry of the v2 version where the get
> > helper needs to be called under the new kvm_ref_lock, but the put
> > helper does not.  Having the get helper handle that makes the caller
> > much cleaner.  Thanks,
> >
>=20
> What about passing the lock pointer into the helper? it's still slightly
> asymmetry as the put helper doesn't carry the lock pointer but it
> could also be interpreted as if the pointer has been saved in the get
> then if it needs to be referenced by the put there is no need to pass
> it in again.

For cdev, I may modify vfio_device_get_kvm_safe() to accept
struct kvm and let its caller hold a kvm_ref_lock (field within
struct vfio_device_file). Meanwhile, the group path holds
the group->kvm_ref_lock before invoking vfio_device_get_kvm_safe().
vfio_device_get_kvm_safe() just includes the symbol get/put and
the device->kvm and put_kvm set.

Regards,
Yi Liu