Message ID | 20221219171924.67989-1-seanjc@google.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:e747:0:0:0:0:0 with SMTP id c7csp2521528wrn; Mon, 19 Dec 2022 09:35:16 -0800 (PST) X-Google-Smtp-Source: AA0mqf5QNDomacRQ+BOEl012NLv3eGOGDhjbmjAQmfKjVZSUFomscpYcnSsKX2ERrnJNR78NdzEj X-Received: by 2002:a17:906:70d0:b0:7c0:8371:97aa with SMTP id g16-20020a17090670d000b007c0837197aamr36244307ejk.28.1671471316235; Mon, 19 Dec 2022 09:35:16 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1671471316; cv=none; d=google.com; s=arc-20160816; b=gMezGde53XHGBGJClZvtoz9PyXvZt2C5hlmHfWXiD+jwoCpKckKbYytgBMNl2OItov 9FLg2RunXFh20z0LSoRzJVCic44ARUihxbO2GdN7CDkR8yVr/OJRzm+9Q6yFg/i3NYoR 6a6Feh0SiDwc3RQMmvQlVaj/PNVJSd80TYOEtuOA5BsjQCNCOknlx59viJ9NAqe9O3ls /IDekCsov4ynVgbZJhzKGa0XC5CYMiw6L2USfVLH2v8Rrzu4bXR6+Ug8WKBaSxMFayNk RXjQVsdoRnR/h3FlumTki65RK7MzW8SN70LLWzoHAU4Gl7c9NWs/2oN/OpXAiW3/qATL cTFQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:cc:to:from:subject :message-id:mime-version:date:reply-to:dkim-signature; bh=u1OxrZzOIajVGZwkXAPogH/ue04ueQgByOpRB200gSU=; b=mxi4TZlFV8Us7kSrEXaF7zSuEclAnKpLXp01i18IpUo0BlY5nlheTe1+A77ywLUHZx VZaigxoQQC1a5o+tsCDbYBRCyIovwerrJIbC6Juywdw6zUosWGdP3muB5TguNWiol0Li 9O4esOeqSGP9Q5qaH5IhKC3uabfo8pDyP10nZurWhUosOHHTiWNXnWOUfjAfie7z+phd 25HKG2V/AVZ2DLW2hp2Nmm3rTdeSc0tGbR5O9sm4qyDssOvdgDySVOPZx9XZxowKM37I o0x7niB47GpV0HTCfySQgArhzXYdPM8HP6aKhovDduZiYevYy14mHGe6I2JuyqxPvttQ 4AyQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20210112 header.b=RrzqGfer; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id xa8-20020a170906fd8800b007c0a7bc2854si10562890ejb.410.2022.12.19.09.34.50; Mon, 19 Dec 2022 09:35:16 -0800 (PST) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20210112 header.b=RrzqGfer; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232361AbiLSRTr (ORCPT <rfc822;abdi.embedded@gmail.com> + 99 others); Mon, 19 Dec 2022 12:19:47 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:52344 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232190AbiLSRT2 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 19 Dec 2022 12:19:28 -0500 Received: from mail-pj1-x104a.google.com (mail-pj1-x104a.google.com [IPv6:2607:f8b0:4864:20::104a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9E4E195B1 for <linux-kernel@vger.kernel.org>; Mon, 19 Dec 2022 09:19:27 -0800 (PST) Received: by mail-pj1-x104a.google.com with SMTP id q93-20020a17090a1b6600b0021311ab9082so3769785pjq.7 for <linux-kernel@vger.kernel.org>; Mon, 19 Dec 2022 09:19:27 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=content-transfer-encoding:cc:to:from:subject:message-id :mime-version:date:reply-to:from:to:cc:subject:date:message-id :reply-to; bh=u1OxrZzOIajVGZwkXAPogH/ue04ueQgByOpRB200gSU=; b=RrzqGferbVNclkVYmMFvgCACtExupzd+zXKlb6mqsno4S/mJv3bQ7LCh29DMgCaVEu 2CVeVtbR98sVzCFZ2PrSeFfDjxBsanG7unC6LTZ7Pvf17Vx8sM/YNe1dVT1DS32UDOfh hteFFn+helbDlE1Cubk5wDLAOGDZTao2EiDVP5kj+BdQOCHaKmFNXcNu8HKZCtnL9iyX UvcOAjulhbrTLhfQXqDGVd6ILFbWZpvYCOcYGraPxPkQ5N8U1+IGg20DKKAkhwCg15pi vN8zy17Y1pJ978V2ET2wigwRRg4poqTNOFZW76ogCRe/tD9RrpOZq/XyQhTz3HV37t5n N0tA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:cc:to:from:subject:message-id :mime-version:date:reply-to:x-gm-message-state:from:to:cc:subject :date:message-id:reply-to; bh=u1OxrZzOIajVGZwkXAPogH/ue04ueQgByOpRB200gSU=; b=twtiR+Yp4Jg32U/R0trLGS2G773orZWM/PfYxE1rf9tndXnft26epMvPyW++GxbbBW AcfQrIbcU7WkLsWJUywsufD9tO0v4yq27QR0AMiFw0vzDLYHwBtwyLaxbCb6/KRtvQrY NK/bofdpRIOyGtlAb0dh/MKccn6HLgpqcOnLycJ62F9g6FXAH6V2jlF57lj6kIc4wAf/ AYu5+Xn4oHAFhg01EicjIJCkJAbzC+xClzfTBMQtUqoIpuTd7mCElhlweDYQpNsE4d6N scg+DUqqk15+5fkcPk11ss7blx5C3wpYKDMGKvarrXgaP7XRY6SbAgaQfuaXhvmDOjD5 UrEw== X-Gm-Message-State: ANoB5plwCts5iKYOirJetIzI816R3vYjOvx2PxcpvKbgi9B+njdcqkS9 DFsyBYrrzd0h+kL0tAq3oznZ3kN5FUE= X-Received: from zagreus.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:5c37]) (user=seanjc job=sendgmr) by 2002:a17:902:cf02:b0:189:d193:95e7 with SMTP id i2-20020a170902cf0200b00189d19395e7mr21663279plg.1.1671470367133; Mon, 19 Dec 2022 09:19:27 -0800 (PST) Reply-To: Sean Christopherson <seanjc@google.com> Date: Mon, 19 Dec 2022 17:19:24 +0000 Mime-Version: 1.0 X-Mailer: git-send-email 2.39.0.314.g84b9a713c41-goog Message-ID: <20221219171924.67989-1-seanjc@google.com> Subject: [PATCH] KVM: Destroy target device if coalesced MMIO unregistration fails From: Sean Christopherson <seanjc@google.com> To: Paolo Bonzini <pbonzini@redhat.com> Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, " =?utf-8?b?5p+z6I+B?= =?utf-8?b?5bOw?= " <liujingfeng@qianxin.com>, Sean Christopherson <seanjc@google.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-9.6 required=5.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL 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 X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1752664706687630750?= X-GMAIL-MSGID: =?utf-8?q?1752664706687630750?= |
Series |
KVM: Destroy target device if coalesced MMIO unregistration fails
|
|
Commit Message
Sean Christopherson
Dec. 19, 2022, 5:19 p.m. UTC
Destroy and free the target coalesced MMIO device if unregistering said
device fails. As clearly noted in the code, kvm_io_bus_unregister_dev()
does not destroy the target device.
BUG: memory leak
unreferenced object 0xffff888112a54880 (size 64):
comm "syz-executor.2", pid 5258, jiffies 4297861402 (age 14.129s)
hex dump (first 32 bytes):
38 c7 67 15 00 c9 ff ff 38 c7 67 15 00 c9 ff ff 8.g.....8.g.....
e0 c7 e1 83 ff ff ff ff 00 30 67 15 00 c9 ff ff .........0g.....
backtrace:
[<0000000006995a8a>] kmalloc include/linux/slab.h:556 [inline]
[<0000000006995a8a>] kzalloc include/linux/slab.h:690 [inline]
[<0000000006995a8a>] kvm_vm_ioctl_register_coalesced_mmio+0x8e/0x3d0 arch/x86/kvm/../../../virt/kvm/coalesced_mmio.c:150
[<00000000022550c2>] kvm_vm_ioctl+0x47d/0x1600 arch/x86/kvm/../../../virt/kvm/kvm_main.c:3323
[<000000008a75102f>] vfs_ioctl fs/ioctl.c:46 [inline]
[<000000008a75102f>] file_ioctl fs/ioctl.c:509 [inline]
[<000000008a75102f>] do_vfs_ioctl+0xbab/0x1160 fs/ioctl.c:696
[<0000000080e3f669>] ksys_ioctl+0x76/0xa0 fs/ioctl.c:713
[<0000000059ef4888>] __do_sys_ioctl fs/ioctl.c:720 [inline]
[<0000000059ef4888>] __se_sys_ioctl fs/ioctl.c:718 [inline]
[<0000000059ef4888>] __x64_sys_ioctl+0x6f/0xb0 fs/ioctl.c:718
[<000000006444fa05>] do_syscall_64+0x9f/0x4e0 arch/x86/entry/common.c:290
[<000000009a4ed50b>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
BUG: leak checking failed
Fixes: 5d3c4c79384a ("KVM: Stop looking for coalesced MMIO zones if the bus is destroyed")
Cc: stable@vger.kernel.org
Reported-by: 柳菁峰 <liujingfeng@qianxin.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
virt/kvm/coalesced_mmio.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
base-commit: 9d75a3251adfbcf444681474511b58042a364863
Comments
On Tuesday, December 20, 2022 1:19 AM, Sean Christopherson wrote: > diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c index > 0be80c213f7f..5ef88f5a0864 100644 > --- a/virt/kvm/coalesced_mmio.c > +++ b/virt/kvm/coalesced_mmio.c > @@ -187,15 +187,17 @@ int > kvm_vm_ioctl_unregister_coalesced_mmio(struct kvm *kvm, > r = kvm_io_bus_unregister_dev(kvm, > zone->pio ? KVM_PIO_BUS : KVM_MMIO_BUS, &dev->dev); > > + kvm_iodevice_destructor(&dev->dev); > + > /* > * On failure, unregister destroys all devices on the > * bus _except_ the target device, i.e. coalesced_zones > - * has been modified. No need to restart the walk as > - * there aren't any zones left. > + * has been modified. Bail after destroying the target > + * device, there's no need to restart the walk as there > + * aren't any zones left. > */ > if (r) > break; > - kvm_iodevice_destructor(&dev->dev); > } > } Another option is to let kvm_io_bus_unregister_dev handle this, and no need for callers to make the extra kvm_iodevice_destructor() call. This simplifies the usage for callers (e.g. reducing LOCs and no leakages like this): diff --git a/include/kvm/iodev.h b/include/kvm/iodev.h index d75fc4365746..56619e33251e 100644 --- a/include/kvm/iodev.h +++ b/include/kvm/iodev.h @@ -55,10 +55,4 @@ static inline int kvm_iodevice_write(struct kvm_vcpu *vcpu, : -EOPNOTSUPP; } -static inline void kvm_iodevice_destructor(struct kvm_io_device *dev) -{ - if (dev->ops->destructor) - dev->ops->destructor(dev); -} - #endif /* __KVM_IODEV_H__ */ diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c index 0be80c213f7f..d7135a5e76f8 100644 --- a/virt/kvm/coalesced_mmio.c +++ b/virt/kvm/coalesced_mmio.c @@ -195,7 +195,6 @@ int kvm_vm_ioctl_unregister_coalesced_mmio(struct kvm *kvm, */ if (r) break; - kvm_iodevice_destructor(&dev->dev); } } diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c index 2a3ed401ce46..1b277afb545b 100644 --- a/virt/kvm/eventfd.c +++ b/virt/kvm/eventfd.c @@ -898,7 +898,6 @@ kvm_deassign_ioeventfd_idx(struct kvm *kvm, enum kvm_bus bus_idx, bus = kvm_get_bus(kvm, bus_idx); if (bus) bus->ioeventfd_count--; - ioeventfd_release(p); ret = 0; break; } diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 13e88297f999..582757ebdce6 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -5200,6 +5200,12 @@ static struct notifier_block kvm_reboot_notifier = { .priority = 0, }; +static void kvm_iodevice_destructor(struct kvm_io_device *dev) +{ + if (dev->ops->destructor) + dev->ops->destructor(dev); +} + static void kvm_io_bus_destroy(struct kvm_io_bus *bus) { int i; @@ -5423,7 +5429,7 @@ int kvm_io_bus_register_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr, int kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, struct kvm_io_device *dev) { - int i, j; + int i; struct kvm_io_bus *new_bus, *bus; lockdep_assert_held(&kvm->slots_lock); @@ -5453,18 +5459,18 @@ int kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, rcu_assign_pointer(kvm->buses[bus_idx], new_bus); synchronize_srcu_expedited(&kvm->srcu); - /* Destroy the old bus _after_ installing the (null) bus. */ + /* + * If (null) bus is installed, destroy the old bus, including all the + * attached devices. Otherwise, destroy the caller's device only. + */ if (!new_bus) { pr_err("kvm: failed to shrink bus, removing it completely\n"); - for (j = 0; j < bus->dev_count; j++) { - if (j == i) - continue; - kvm_iodevice_destructor(bus->range[j].dev); - } + kvm_io_bus_destroy(bus); + return -ENOMEM; } - kfree(bus); - return new_bus ? 0 : -ENOMEM; + kvm_iodevice_destructor(dev); + return 0; } struct kvm_io_device *kvm_io_bus_get_dev(struct kvm *kvm, enum kvm_bus bus_idx,
On 12/20/2022 11:04 AM, Wang, Wei W wrote: > On Tuesday, December 20, 2022 1:19 AM, Sean Christopherson wrote: >> diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c index >> 0be80c213f7f..5ef88f5a0864 100644 >> --- a/virt/kvm/coalesced_mmio.c >> +++ b/virt/kvm/coalesced_mmio.c >> @@ -187,15 +187,17 @@ int >> kvm_vm_ioctl_unregister_coalesced_mmio(struct kvm *kvm, >> r = kvm_io_bus_unregister_dev(kvm, >> zone->pio ? KVM_PIO_BUS : KVM_MMIO_BUS, &dev->dev); >> >> + kvm_iodevice_destructor(&dev->dev); >> + >> /* >> * On failure, unregister destroys all devices on the >> * bus _except_ the target device, i.e. coalesced_zones >> - * has been modified. No need to restart the walk as >> - * there aren't any zones left. >> + * has been modified. Bail after destroying the target >> + * device, there's no need to restart the walk as there >> + * aren't any zones left. >> */ >> if (r) >> break; >> - kvm_iodevice_destructor(&dev->dev); >> } >> } > Another option is to let kvm_io_bus_unregister_dev handle this, and no need for callers > to make the extra kvm_iodevice_destructor() call. This simplifies the usage for callers > (e.g. reducing LOCs and no leakages like this): One vote for this option : ) > > diff --git a/include/kvm/iodev.h b/include/kvm/iodev.h > index d75fc4365746..56619e33251e 100644 > --- a/include/kvm/iodev.h > +++ b/include/kvm/iodev.h > @@ -55,10 +55,4 @@ static inline int kvm_iodevice_write(struct kvm_vcpu *vcpu, > : -EOPNOTSUPP; > } > > -static inline void kvm_iodevice_destructor(struct kvm_io_device *dev) > -{ > - if (dev->ops->destructor) > - dev->ops->destructor(dev); > -} > - > #endif /* __KVM_IODEV_H__ */ > diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c > index 0be80c213f7f..d7135a5e76f8 100644 > --- a/virt/kvm/coalesced_mmio.c > +++ b/virt/kvm/coalesced_mmio.c > @@ -195,7 +195,6 @@ int kvm_vm_ioctl_unregister_coalesced_mmio(struct kvm *kvm, > */ > if (r) > break; > - kvm_iodevice_destructor(&dev->dev); > } > } > > diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c > index 2a3ed401ce46..1b277afb545b 100644 > --- a/virt/kvm/eventfd.c > +++ b/virt/kvm/eventfd.c > @@ -898,7 +898,6 @@ kvm_deassign_ioeventfd_idx(struct kvm *kvm, enum kvm_bus bus_idx, > bus = kvm_get_bus(kvm, bus_idx); > if (bus) > bus->ioeventfd_count--; > - ioeventfd_release(p); > ret = 0; > break; > } > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 13e88297f999..582757ebdce6 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -5200,6 +5200,12 @@ static struct notifier_block kvm_reboot_notifier = { > .priority = 0, > }; > > +static void kvm_iodevice_destructor(struct kvm_io_device *dev) > +{ > + if (dev->ops->destructor) > + dev->ops->destructor(dev); > +} > + > static void kvm_io_bus_destroy(struct kvm_io_bus *bus) > { > int i; > @@ -5423,7 +5429,7 @@ int kvm_io_bus_register_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr, > int kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, > struct kvm_io_device *dev) > { > - int i, j; > + int i; > struct kvm_io_bus *new_bus, *bus; > > lockdep_assert_held(&kvm->slots_lock); > @@ -5453,18 +5459,18 @@ int kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, > rcu_assign_pointer(kvm->buses[bus_idx], new_bus); > synchronize_srcu_expedited(&kvm->srcu); > > - /* Destroy the old bus _after_ installing the (null) bus. */ > + /* > + * If (null) bus is installed, destroy the old bus, including all the > + * attached devices. Otherwise, destroy the caller's device only. > + */ > if (!new_bus) { > pr_err("kvm: failed to shrink bus, removing it completely\n"); > - for (j = 0; j < bus->dev_count; j++) { > - if (j == i) > - continue; > - kvm_iodevice_destructor(bus->range[j].dev); > - } > + kvm_io_bus_destroy(bus); > + return -ENOMEM; > } > > - kfree(bus); > - return new_bus ? 0 : -ENOMEM; > + kvm_iodevice_destructor(dev); > + return 0; > } > > struct kvm_io_device *kvm_io_bus_get_dev(struct kvm *kvm, enum kvm_bus bus_idx,
On 12/20/22 04:04, Wang, Wei W wrote: > Another option is to let kvm_io_bus_unregister_dev handle this, and no need for callers > to make the extra kvm_iodevice_destructor() call. This simplifies the usage for callers > (e.g. reducing LOCs and no leakages like this): Can you send this as a patch? Thanks! Paolo > diff --git a/include/kvm/iodev.h b/include/kvm/iodev.h > index d75fc4365746..56619e33251e 100644 > --- a/include/kvm/iodev.h > +++ b/include/kvm/iodev.h > @@ -55,10 +55,4 @@ static inline int kvm_iodevice_write(struct kvm_vcpu *vcpu, > : -EOPNOTSUPP; > } > > -static inline void kvm_iodevice_destructor(struct kvm_io_device *dev) > -{ > - if (dev->ops->destructor) > - dev->ops->destructor(dev); > -} > - > #endif /* __KVM_IODEV_H__ */ > diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c > index 0be80c213f7f..d7135a5e76f8 100644 > --- a/virt/kvm/coalesced_mmio.c > +++ b/virt/kvm/coalesced_mmio.c > @@ -195,7 +195,6 @@ int kvm_vm_ioctl_unregister_coalesced_mmio(struct kvm *kvm, > */ > if (r) > break; > - kvm_iodevice_destructor(&dev->dev); > } > } > > diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c > index 2a3ed401ce46..1b277afb545b 100644 > --- a/virt/kvm/eventfd.c > +++ b/virt/kvm/eventfd.c > @@ -898,7 +898,6 @@ kvm_deassign_ioeventfd_idx(struct kvm *kvm, enum kvm_bus bus_idx, > bus = kvm_get_bus(kvm, bus_idx); > if (bus) > bus->ioeventfd_count--; > - ioeventfd_release(p); > ret = 0; > break; > } > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 13e88297f999..582757ebdce6 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -5200,6 +5200,12 @@ static struct notifier_block kvm_reboot_notifier = { > .priority = 0, > }; > > +static void kvm_iodevice_destructor(struct kvm_io_device *dev) > +{ > + if (dev->ops->destructor) > + dev->ops->destructor(dev); > +} > + > static void kvm_io_bus_destroy(struct kvm_io_bus *bus) > { > int i; > @@ -5423,7 +5429,7 @@ int kvm_io_bus_register_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr, > int kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, > struct kvm_io_device *dev) > { > - int i, j; > + int i; > struct kvm_io_bus *new_bus, *bus; > > lockdep_assert_held(&kvm->slots_lock); > @@ -5453,18 +5459,18 @@ int kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, > rcu_assign_pointer(kvm->buses[bus_idx], new_bus); > synchronize_srcu_expedited(&kvm->srcu); > > - /* Destroy the old bus _after_ installing the (null) bus. */ > + /* > + * If (null) bus is installed, destroy the old bus, including all the > + * attached devices. Otherwise, destroy the caller's device only. > + */ > if (!new_bus) { > pr_err("kvm: failed to shrink bus, removing it completely\n"); > - for (j = 0; j < bus->dev_count; j++) { > - if (j == i) > - continue; > - kvm_iodevice_destructor(bus->range[j].dev); > - } > + kvm_io_bus_destroy(bus); > + return -ENOMEM; > } > > - kfree(bus); > - return new_bus ? 0 : -ENOMEM; > + kvm_iodevice_destructor(dev); > + return 0; > } > > struct kvm_io_device *kvm_io_bus_get_dev(struct kvm *kvm, enum kvm_bus bus_idx,
On Saturday, December 24, 2022 1:14 AM, Paolo Bonzini wrote: > On 12/20/22 04:04, Wang, Wei W wrote: > > Another option is to let kvm_io_bus_unregister_dev handle this, and no > > need for callers to make the extra kvm_iodevice_destructor() call. > > This simplifies the usage for callers (e.g. reducing LOCs and no leakages like > this): > > Can you send this as a patch? Thanks! Sure. I can do it. I'm also fine if Sean would be interested in taking over the code (or anything I should do to keep his credits for the original fixing?)
On Mon, Dec 26, 2022, Wang, Wei W wrote: > On Saturday, December 24, 2022 1:14 AM, Paolo Bonzini wrote: > > On 12/20/22 04:04, Wang, Wei W wrote: > > > Another option is to let kvm_io_bus_unregister_dev handle this, and no > > > need for callers to make the extra kvm_iodevice_destructor() call. > > > This simplifies the usage for callers (e.g. reducing LOCs and no leakages like > > this): > > > > Can you send this as a patch? Thanks! > > Sure. I can do it. I'm also fine if Sean would be interested in taking over the > code No thanks. > (or anything I should do to keep his credits for the original fixing?) No need. If anything, take my patch first so that the fix for stable kernels is trivial. That's Paolo's call though.
On Mon, 19 Dec 2022 17:19:24 +0000, Sean Christopherson wrote: > Destroy and free the target coalesced MMIO device if unregistering said > device fails. As clearly noted in the code, kvm_io_bus_unregister_dev() > does not destroy the target device. > > BUG: memory leak > unreferenced object 0xffff888112a54880 (size 64): > comm "syz-executor.2", pid 5258, jiffies 4297861402 (age 14.129s) > hex dump (first 32 bytes): > 38 c7 67 15 00 c9 ff ff 38 c7 67 15 00 c9 ff ff 8.g.....8.g..... > e0 c7 e1 83 ff ff ff ff 00 30 67 15 00 c9 ff ff .........0g..... > backtrace: > [<0000000006995a8a>] kmalloc include/linux/slab.h:556 [inline] > [<0000000006995a8a>] kzalloc include/linux/slab.h:690 [inline] > [<0000000006995a8a>] kvm_vm_ioctl_register_coalesced_mmio+0x8e/0x3d0 arch/x86/kvm/../../../virt/kvm/coalesced_mmio.c:150 > [<00000000022550c2>] kvm_vm_ioctl+0x47d/0x1600 arch/x86/kvm/../../../virt/kvm/kvm_main.c:3323 > [<000000008a75102f>] vfs_ioctl fs/ioctl.c:46 [inline] > [<000000008a75102f>] file_ioctl fs/ioctl.c:509 [inline] > [<000000008a75102f>] do_vfs_ioctl+0xbab/0x1160 fs/ioctl.c:696 > [<0000000080e3f669>] ksys_ioctl+0x76/0xa0 fs/ioctl.c:713 > [<0000000059ef4888>] __do_sys_ioctl fs/ioctl.c:720 [inline] > [<0000000059ef4888>] __se_sys_ioctl fs/ioctl.c:718 [inline] > [<0000000059ef4888>] __x64_sys_ioctl+0x6f/0xb0 fs/ioctl.c:718 > [<000000006444fa05>] do_syscall_64+0x9f/0x4e0 arch/x86/entry/common.c:290 > [<000000009a4ed50b>] entry_SYSCALL_64_after_hwframe+0x49/0xbe > > [...] Applied to kvm-x86 generic, the plan is that Wei will send the bigger cleanup on top. Thanks! [1/1] KVM: Destroy target device if coalesced MMIO unregistration fails https://github.com/kvm-x86/linux/commit/b1cb1fac22ab -- https://github.com/kvm-x86/linux/tree/next https://github.com/kvm-x86/linux/tree/fixes
diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c index 0be80c213f7f..5ef88f5a0864 100644 --- a/virt/kvm/coalesced_mmio.c +++ b/virt/kvm/coalesced_mmio.c @@ -187,15 +187,17 @@ int kvm_vm_ioctl_unregister_coalesced_mmio(struct kvm *kvm, r = kvm_io_bus_unregister_dev(kvm, zone->pio ? KVM_PIO_BUS : KVM_MMIO_BUS, &dev->dev); + kvm_iodevice_destructor(&dev->dev); + /* * On failure, unregister destroys all devices on the * bus _except_ the target device, i.e. coalesced_zones - * has been modified. No need to restart the walk as - * there aren't any zones left. + * has been modified. Bail after destroying the target + * device, there's no need to restart the walk as there + * aren't any zones left. */ if (r) break; - kvm_iodevice_destructor(&dev->dev); } }