* [PATCH v2] KVM: fix memory leak in kvm_io_bus_unregister_dev() @ 2020-09-02 22:57 Rustam Kovhaev 2020-09-02 23:34 ` Gustavo A. R. Silva 0 siblings, 1 reply; 5+ messages in thread From: Rustam Kovhaev @ 2020-09-02 22:57 UTC (permalink / raw) To: pbonzini, vkuznets, kvm; +Cc: linux-kernel, gregkh, Rustam Kovhaev when kmalloc() fails in kvm_io_bus_unregister_dev(), before removing the bus, we should iterate over all other devices linked to it and call kvm_iodevice_destructor() for them Reported-and-tested-by: syzbot+f196caa45793d6374707@syzkaller.appspotmail.com Link: https://syzkaller.appspot.com/bug?extid=f196caa45793d6374707 Signed-off-by: Rustam Kovhaev <rkovhaev@gmail.com> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- v2: - remove redundant whitespace - remove goto statement and use if/else --- virt/kvm/kvm_main.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 67cd0b88a6b6..cf88233b819a 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -4332,7 +4332,7 @@ int kvm_io_bus_register_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr, void kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, struct kvm_io_device *dev) { - int i; + int i, j; struct kvm_io_bus *new_bus, *bus; bus = kvm_get_bus(kvm, bus_idx); @@ -4349,17 +4349,20 @@ void kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, new_bus = kmalloc(struct_size(bus, range, bus->dev_count - 1), GFP_KERNEL_ACCOUNT); - if (!new_bus) { + if (new_bus) { + memcpy(new_bus, bus, sizeof(*bus) + i * sizeof(struct kvm_io_range)); + new_bus->dev_count--; + memcpy(new_bus->range + i, bus->range + i + 1, + (new_bus->dev_count - i) * sizeof(struct kvm_io_range)); + } else { pr_err("kvm: failed to shrink bus, removing it completely\n"); - goto broken; + for (j = 0; j < bus->dev_count; j++) { + if (j == i) + continue; + kvm_iodevice_destructor(bus->range[j].dev); + } } - memcpy(new_bus, bus, sizeof(*bus) + i * sizeof(struct kvm_io_range)); - new_bus->dev_count--; - memcpy(new_bus->range + i, bus->range + i + 1, - (new_bus->dev_count - i) * sizeof(struct kvm_io_range)); - -broken: rcu_assign_pointer(kvm->buses[bus_idx], new_bus); synchronize_srcu_expedited(&kvm->srcu); kfree(bus); -- 2.28.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] KVM: fix memory leak in kvm_io_bus_unregister_dev() 2020-09-02 22:57 [PATCH v2] KVM: fix memory leak in kvm_io_bus_unregister_dev() Rustam Kovhaev @ 2020-09-02 23:34 ` Gustavo A. R. Silva 2020-09-03 17:22 ` Rustam Kovhaev 0 siblings, 1 reply; 5+ messages in thread From: Gustavo A. R. Silva @ 2020-09-02 23:34 UTC (permalink / raw) To: Rustam Kovhaev, pbonzini, vkuznets, kvm; +Cc: linux-kernel, gregkh Hi, On 9/2/20 17:57, Rustam Kovhaev wrote: > when kmalloc() fails in kvm_io_bus_unregister_dev(), before removing > the bus, we should iterate over all other devices linked to it and call > kvm_iodevice_destructor() for them > > Reported-and-tested-by: syzbot+f196caa45793d6374707@syzkaller.appspotmail.com > Link: https://syzkaller.appspot.com/bug?extid=f196caa45793d6374707 > Signed-off-by: Rustam Kovhaev <rkovhaev@gmail.com> > Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> I think it's worthwhile to add a Fixes tag for this, too. Please, see more comments below... > --- > v2: > - remove redundant whitespace > - remove goto statement and use if/else > --- > virt/kvm/kvm_main.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 67cd0b88a6b6..cf88233b819a 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -4332,7 +4332,7 @@ int kvm_io_bus_register_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr, > void kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, > struct kvm_io_device *dev) > { > - int i; > + int i, j; > struct kvm_io_bus *new_bus, *bus; > > bus = kvm_get_bus(kvm, bus_idx); > @@ -4349,17 +4349,20 @@ void kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, > > new_bus = kmalloc(struct_size(bus, range, bus->dev_count - 1), > GFP_KERNEL_ACCOUNT); > - if (!new_bus) { > + if (new_bus) { > + memcpy(new_bus, bus, sizeof(*bus) + i * sizeof(struct kvm_io_range)); ^^^ It seems that you can use struct_size() here (see the allocation code above)... > + new_bus->dev_count--; > + memcpy(new_bus->range + i, bus->range + i + 1, > + (new_bus->dev_count - i) * sizeof(struct kvm_io_range)); ^^^ ...and, if possible, you can also use flex_array_size() here. Thanks -- Gustavo > + } else { > pr_err("kvm: failed to shrink bus, removing it completely\n"); > - goto broken; > + for (j = 0; j < bus->dev_count; j++) { > + if (j == i) > + continue; > + kvm_iodevice_destructor(bus->range[j].dev); > + } > } > > - memcpy(new_bus, bus, sizeof(*bus) + i * sizeof(struct kvm_io_range)); > - new_bus->dev_count--; > - memcpy(new_bus->range + i, bus->range + i + 1, > - (new_bus->dev_count - i) * sizeof(struct kvm_io_range)); > - > -broken: > rcu_assign_pointer(kvm->buses[bus_idx], new_bus); > synchronize_srcu_expedited(&kvm->srcu); > kfree(bus); > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] KVM: fix memory leak in kvm_io_bus_unregister_dev() 2020-09-02 23:34 ` Gustavo A. R. Silva @ 2020-09-03 17:22 ` Rustam Kovhaev 2020-09-04 12:04 ` Vitaly Kuznetsov 0 siblings, 1 reply; 5+ messages in thread From: Rustam Kovhaev @ 2020-09-03 17:22 UTC (permalink / raw) To: Gustavo A. R. Silva; +Cc: pbonzini, vkuznets, kvm, linux-kernel, gregkh On Wed, Sep 02, 2020 at 06:34:11PM -0500, Gustavo A. R. Silva wrote: > Hi, > > On 9/2/20 17:57, Rustam Kovhaev wrote: > > when kmalloc() fails in kvm_io_bus_unregister_dev(), before removing > > the bus, we should iterate over all other devices linked to it and call > > kvm_iodevice_destructor() for them > > > > Reported-and-tested-by: syzbot+f196caa45793d6374707@syzkaller.appspotmail.com > > Link: https://syzkaller.appspot.com/bug?extid=f196caa45793d6374707 > > Signed-off-by: Rustam Kovhaev <rkovhaev@gmail.com> > > Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > I think it's worthwhile to add a Fixes tag for this, too. > > Please, see more comments below... > > > --- > > v2: > > - remove redundant whitespace > > - remove goto statement and use if/else > > --- > > virt/kvm/kvm_main.c | 21 ++++++++++++--------- > > 1 file changed, 12 insertions(+), 9 deletions(-) > > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index 67cd0b88a6b6..cf88233b819a 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -4332,7 +4332,7 @@ int kvm_io_bus_register_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr, > > void kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, > > struct kvm_io_device *dev) > > { > > - int i; > > + int i, j; > > struct kvm_io_bus *new_bus, *bus; > > > > bus = kvm_get_bus(kvm, bus_idx); > > @@ -4349,17 +4349,20 @@ void kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, > > > > new_bus = kmalloc(struct_size(bus, range, bus->dev_count - 1), > > GFP_KERNEL_ACCOUNT); > > - if (!new_bus) { > > + if (new_bus) { > > + memcpy(new_bus, bus, sizeof(*bus) + i * sizeof(struct kvm_io_range)); > > ^^^ > It seems that you can use struct_size() here (see the allocation code above)... > > > + new_bus->dev_count--; > > + memcpy(new_bus->range + i, bus->range + i + 1, > > + (new_bus->dev_count - i) * sizeof(struct kvm_io_range)); > > ^^^ > ...and, if possible, you can also use flex_array_size() here. > > Thanks > -- > Gustavo > > > + } else { > > pr_err("kvm: failed to shrink bus, removing it completely\n"); > > - goto broken; > > + for (j = 0; j < bus->dev_count; j++) { > > + if (j == i) > > + continue; > > + kvm_iodevice_destructor(bus->range[j].dev); > > + } > > } > > > > - memcpy(new_bus, bus, sizeof(*bus) + i * sizeof(struct kvm_io_range)); > > - new_bus->dev_count--; > > - memcpy(new_bus->range + i, bus->range + i + 1, > > - (new_bus->dev_count - i) * sizeof(struct kvm_io_range)); > > - > > -broken: > > rcu_assign_pointer(kvm->buses[bus_idx], new_bus); > > synchronize_srcu_expedited(&kvm->srcu); > > kfree(bus); > > hi Gustavo, thank you for the review, i'll send the new patch. Vitaly, i think i will need to drop your "Reviewed-by", because there is going to be a bit more changes ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] KVM: fix memory leak in kvm_io_bus_unregister_dev() 2020-09-03 17:22 ` Rustam Kovhaev @ 2020-09-04 12:04 ` Vitaly Kuznetsov 2020-09-04 14:37 ` Gustavo A. R. Silva 0 siblings, 1 reply; 5+ messages in thread From: Vitaly Kuznetsov @ 2020-09-04 12:04 UTC (permalink / raw) To: Rustam Kovhaev, Gustavo A. R. Silva; +Cc: pbonzini, kvm, linux-kernel, gregkh Rustam Kovhaev <rkovhaev@gmail.com> writes: > On Wed, Sep 02, 2020 at 06:34:11PM -0500, Gustavo A. R. Silva wrote: >> Hi, >> >> On 9/2/20 17:57, Rustam Kovhaev wrote: >> > when kmalloc() fails in kvm_io_bus_unregister_dev(), before removing >> > the bus, we should iterate over all other devices linked to it and call >> > kvm_iodevice_destructor() for them >> > >> > Reported-and-tested-by: syzbot+f196caa45793d6374707@syzkaller.appspotmail.com >> > Link: https://syzkaller.appspot.com/bug?extid=f196caa45793d6374707 >> > Signed-off-by: Rustam Kovhaev <rkovhaev@gmail.com> >> > Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> >> >> I think it's worthwhile to add a Fixes tag for this, too. >> >> Please, see more comments below... >> >> > --- >> > v2: >> > - remove redundant whitespace >> > - remove goto statement and use if/else >> > --- >> > virt/kvm/kvm_main.c | 21 ++++++++++++--------- >> > 1 file changed, 12 insertions(+), 9 deletions(-) >> > >> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c >> > index 67cd0b88a6b6..cf88233b819a 100644 >> > --- a/virt/kvm/kvm_main.c >> > +++ b/virt/kvm/kvm_main.c >> > @@ -4332,7 +4332,7 @@ int kvm_io_bus_register_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr, >> > void kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, >> > struct kvm_io_device *dev) >> > { >> > - int i; >> > + int i, j; >> > struct kvm_io_bus *new_bus, *bus; >> > >> > bus = kvm_get_bus(kvm, bus_idx); >> > @@ -4349,17 +4349,20 @@ void kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, >> > >> > new_bus = kmalloc(struct_size(bus, range, bus->dev_count - 1), >> > GFP_KERNEL_ACCOUNT); >> > - if (!new_bus) { >> > + if (new_bus) { >> > + memcpy(new_bus, bus, sizeof(*bus) + i * sizeof(struct kvm_io_range)); >> >> ^^^ >> It seems that you can use struct_size() here (see the allocation code above)... >> >> > + new_bus->dev_count--; >> > + memcpy(new_bus->range + i, bus->range + i + 1, >> > + (new_bus->dev_count - i) * sizeof(struct kvm_io_range)); >> >> ^^^ >> ...and, if possible, you can also use flex_array_size() here. >> >> Thanks >> -- >> Gustavo >> >> > + } else { >> > pr_err("kvm: failed to shrink bus, removing it completely\n"); >> > - goto broken; >> > + for (j = 0; j < bus->dev_count; j++) { >> > + if (j == i) >> > + continue; >> > + kvm_iodevice_destructor(bus->range[j].dev); >> > + } >> > } >> > >> > - memcpy(new_bus, bus, sizeof(*bus) + i * sizeof(struct kvm_io_range)); >> > - new_bus->dev_count--; >> > - memcpy(new_bus->range + i, bus->range + i + 1, >> > - (new_bus->dev_count - i) * sizeof(struct kvm_io_range)); >> > - >> > -broken: >> > rcu_assign_pointer(kvm->buses[bus_idx], new_bus); >> > synchronize_srcu_expedited(&kvm->srcu); >> > kfree(bus); >> > > > hi Gustavo, thank you for the review, i'll send the new patch. > Vitaly, i think i will need to drop your "Reviewed-by", because there is > going to be a bit more changes > Personally, I'd prefer to make struct_size()/flex_array_size() a separate preparatory patch so the real fix is small but I don't have a strong opinion. I'll take look at v3 so feel free to drop R-b if you decide to make a combined patch and feel free to keep it if you make the preparatory changes separate :-) -- Vitaly ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] KVM: fix memory leak in kvm_io_bus_unregister_dev() 2020-09-04 12:04 ` Vitaly Kuznetsov @ 2020-09-04 14:37 ` Gustavo A. R. Silva 0 siblings, 0 replies; 5+ messages in thread From: Gustavo A. R. Silva @ 2020-09-04 14:37 UTC (permalink / raw) To: Vitaly Kuznetsov Cc: Rustam Kovhaev, Gustavo A. R. Silva, pbonzini, kvm, linux-kernel, gregkh On Fri, Sep 04, 2020 at 02:04:23PM +0200, Vitaly Kuznetsov wrote: > Rustam Kovhaev <rkovhaev@gmail.com> writes: > > > On Wed, Sep 02, 2020 at 06:34:11PM -0500, Gustavo A. R. Silva wrote: > >> Hi, > >> > >> On 9/2/20 17:57, Rustam Kovhaev wrote: > >> > when kmalloc() fails in kvm_io_bus_unregister_dev(), before removing > >> > the bus, we should iterate over all other devices linked to it and call > >> > kvm_iodevice_destructor() for them > >> > > >> > Reported-and-tested-by: syzbot+f196caa45793d6374707@syzkaller.appspotmail.com > >> > Link: https://syzkaller.appspot.com/bug?extid=f196caa45793d6374707 > >> > Signed-off-by: Rustam Kovhaev <rkovhaev@gmail.com> > >> > Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> > >> > >> I think it's worthwhile to add a Fixes tag for this, too. > >> > >> Please, see more comments below... [..] > > > > hi Gustavo, thank you for the review, i'll send the new patch. > > Vitaly, i think i will need to drop your "Reviewed-by", because there is > > going to be a bit more changes > > > > Personally, I'd prefer to make struct_size()/flex_array_size() a > separate preparatory patch so the real fix is small but I don't have a > strong opinion. I'll take look at v3 so feel free to drop R-b if you > decide to make a combined patch and feel free to keep it if you make the > preparatory changes separate :-) > I agree. A two-patch series is much better in this case. Rustam - please add a Fixes tag to the first patch and see if it can be applied to -stable. If so, you should Cc stable@vger.kernel.org, too. Thanks -- Gustavo ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-09-04 14:31 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-02 22:57 [PATCH v2] KVM: fix memory leak in kvm_io_bus_unregister_dev() Rustam Kovhaev 2020-09-02 23:34 ` Gustavo A. R. Silva 2020-09-03 17:22 ` Rustam Kovhaev 2020-09-04 12:04 ` Vitaly Kuznetsov 2020-09-04 14:37 ` Gustavo A. R. Silva
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).