| Message ID | 20230714224538.404793-1-dmitry.torokhov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/2] kvm/vfio: ensure kvg instance stays around in kvm_vfio_group_add() | expand |
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Sent: Saturday, July 15, 2023 6:46 AM > > kvm_vfio_group_add() creates kvg instance, links it to kv->group_list, > and calls kvm_vfio_file_set_kvm() with kvg->file as an argument after > dropping kv->lock. If we race group addition and deletion calls, kvg > instance may get freed by the time we get around to calling > kvm_vfio_file_set_kvm(). > > Previous iterations of the code did not reference kvg->file outside of > the critical section, but used a temporary variable. Still, they had > similar problem of the file reference being owned by kvg structure and > potential for kvm_vfio_group_del() dropping it before > kvm_vfio_group_add() had a chance to complete. > > Fix this by moving call to kvm_vfio_file_set_kvm() under the protection > of kv->lock. We already call it while holding the same lock when vfio > group is being deleted, so it should be safe here as well. > > Fixes: 2fc1bec15883 ("kvm: set/clear kvm to/from vfio_group when group > add/delete") > Reviewed-by: Alex Williamson <alex.williamson@redhat.com> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Hey Paolo, I'll pull this through the vfio tree unless you have a particular interest. Thanks, Alex On Fri, 14 Jul 2023 15:45:32 -0700 Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > kvm_vfio_group_add() creates kvg instance, links it to kv->group_list, > and calls kvm_vfio_file_set_kvm() with kvg->file as an argument after > dropping kv->lock. If we race group addition and deletion calls, kvg > instance may get freed by the time we get around to calling > kvm_vfio_file_set_kvm(). > > Previous iterations of the code did not reference kvg->file outside of > the critical section, but used a temporary variable. Still, they had > similar problem of the file reference being owned by kvg structure and > potential for kvm_vfio_group_del() dropping it before > kvm_vfio_group_add() had a chance to complete. > > Fix this by moving call to kvm_vfio_file_set_kvm() under the protection > of kv->lock. We already call it while holding the same lock when vfio > group is being deleted, so it should be safe here as well. > > Fixes: 2fc1bec15883 ("kvm: set/clear kvm to/from vfio_group when group add/delete") > Reviewed-by: Alex Williamson <alex.williamson@redhat.com> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > > v3: added Alex's reviewed-by > > v2: updated commit description with the correct "Fixes" tag (per Alex), > expanded commit description to mention issues with the earlier > implementation of kvm_vfio_group_add(). > > virt/kvm/vfio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c > index 9584eb57e0ed..cd46d7ef98d6 100644 > --- a/virt/kvm/vfio.c > +++ b/virt/kvm/vfio.c > @@ -179,10 +179,10 @@ static int kvm_vfio_group_add(struct kvm_device *dev, unsigned int fd) > list_add_tail(&kvg->node, &kv->group_list); > > kvm_arch_start_assignment(dev->kvm); > + kvm_vfio_file_set_kvm(kvg->file, dev->kvm); > > mutex_unlock(&kv->lock); > > - kvm_vfio_file_set_kvm(kvg->file, dev->kvm); > kvm_vfio_update_coherency(dev); > > return 0;
On Fri, 14 Jul 2023 15:45:32 -0700 Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > kvm_vfio_group_add() creates kvg instance, links it to kv->group_list, > and calls kvm_vfio_file_set_kvm() with kvg->file as an argument after > dropping kv->lock. If we race group addition and deletion calls, kvg > instance may get freed by the time we get around to calling > kvm_vfio_file_set_kvm(). > > Previous iterations of the code did not reference kvg->file outside of > the critical section, but used a temporary variable. Still, they had > similar problem of the file reference being owned by kvg structure and > potential for kvm_vfio_group_del() dropping it before > kvm_vfio_group_add() had a chance to complete. > > Fix this by moving call to kvm_vfio_file_set_kvm() under the protection > of kv->lock. We already call it while holding the same lock when vfio > group is being deleted, so it should be safe here as well. > > Fixes: 2fc1bec15883 ("kvm: set/clear kvm to/from vfio_group when group add/delete") > Reviewed-by: Alex Williamson <alex.williamson@redhat.com> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- Applied series to vfio next branch for v6.6. There's a minor rebase involved, so please double check the results: https://github.com/awilliam/linux-vfio/commits/next Thanks, Alex > > v3: added Alex's reviewed-by > > v2: updated commit description with the correct "Fixes" tag (per Alex), > expanded commit description to mention issues with the earlier > implementation of kvm_vfio_group_add(). > > virt/kvm/vfio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c > index 9584eb57e0ed..cd46d7ef98d6 100644 > --- a/virt/kvm/vfio.c > +++ b/virt/kvm/vfio.c > @@ -179,10 +179,10 @@ static int kvm_vfio_group_add(struct kvm_device *dev, unsigned int fd) > list_add_tail(&kvg->node, &kv->group_list); > > kvm_arch_start_assignment(dev->kvm); > + kvm_vfio_file_set_kvm(kvg->file, dev->kvm); > > mutex_unlock(&kv->lock); > > - kvm_vfio_file_set_kvm(kvg->file, dev->kvm); > kvm_vfio_update_coherency(dev); > > return 0;
On Thu, Aug 03, 2023 at 01:38:12PM -0600, Alex Williamson wrote: > On Fri, 14 Jul 2023 15:45:32 -0700 > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > kvm_vfio_group_add() creates kvg instance, links it to kv->group_list, > > and calls kvm_vfio_file_set_kvm() with kvg->file as an argument after > > dropping kv->lock. If we race group addition and deletion calls, kvg > > instance may get freed by the time we get around to calling > > kvm_vfio_file_set_kvm(). > > > > Previous iterations of the code did not reference kvg->file outside of > > the critical section, but used a temporary variable. Still, they had > > similar problem of the file reference being owned by kvg structure and > > potential for kvm_vfio_group_del() dropping it before > > kvm_vfio_group_add() had a chance to complete. > > > > Fix this by moving call to kvm_vfio_file_set_kvm() under the protection > > of kv->lock. We already call it while holding the same lock when vfio > > group is being deleted, so it should be safe here as well. > > > > Fixes: 2fc1bec15883 ("kvm: set/clear kvm to/from vfio_group when group add/delete") > > Reviewed-by: Alex Williamson <alex.williamson@redhat.com> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > --- > > Applied series to vfio next branch for v6.6. There's a minor rebase > involved, so please double check the results: > > https://github.com/awilliam/linux-vfio/commits/next Looks good to me, thanks!
diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c index 9584eb57e0ed..cd46d7ef98d6 100644 --- a/virt/kvm/vfio.c +++ b/virt/kvm/vfio.c @@ -179,10 +179,10 @@ static int kvm_vfio_group_add(struct kvm_device *dev, unsigned int fd) list_add_tail(&kvg->node, &kv->group_list); kvm_arch_start_assignment(dev->kvm); + kvm_vfio_file_set_kvm(kvg->file, dev->kvm); mutex_unlock(&kv->lock); - kvm_vfio_file_set_kvm(kvg->file, dev->kvm); kvm_vfio_update_coherency(dev); return 0;
kvm_vfio_group_add() creates kvg instance, links it to kv->group_list, and calls kvm_vfio_file_set_kvm() with kvg->file as an argument after dropping kv->lock. If we race group addition and deletion calls, kvg instance may get freed by the time we get around to calling kvm_vfio_file_set_kvm(). Previous iterations of the code did not reference kvg->file outside of the critical section, but used a temporary variable. Still, they had similar problem of the file reference being owned by kvg structure and potential for kvm_vfio_group_del() dropping it before kvm_vfio_group_add() had a chance to complete. Fix this by moving call to kvm_vfio_file_set_kvm() under the protection of kv->lock. We already call it while holding the same lock when vfio group is being deleted, so it should be safe here as well. Fixes: 2fc1bec15883 ("kvm: set/clear kvm to/from vfio_group when group add/delete") Reviewed-by: Alex Williamson <alex.williamson@redhat.com> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- v3: added Alex's reviewed-by v2: updated commit description with the correct "Fixes" tag (per Alex), expanded commit description to mention issues with the earlier implementation of kvm_vfio_group_add(). virt/kvm/vfio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)