[PATCH] [RESEND] KVM:PPC:Book3s: fix memory leak in kvm_vm_ioctl_get_htab_fd
ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
but no free when anon_inode_getfd return fail
so, add kfree(ctx) to fix memory leak
Signed-off-by: nixiaoming <nixiaom...@huawei.com>
Reviewed-by: Paolo Bonzini <pbonz...@redhat.com>
---
arch/powerpc/kvm/book3s_64_mmu_hv.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c
b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index b42812e..be3d08f 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -1940,6 +1940,7 @@ int kvm_vm_ioctl_get_htab_fd(struct kvm *kvm, struct
kvm_get_htab_fd *ghf)
rwflag = (ghf->flags & KVM_GET_HTAB_WRITE) ? O_WRONLY : O_RDONLY;
ret = anon_inode_getfd("kvm-htab", _htab_fops, ctx, rwflag |
O_CLOEXEC);
if (ret < 0) {
+ kfree(ctx);
kvm_put_kvm(kvm);
return ret;
}
--
2.11.0.1
RE: [PATCH really v2] KVM: PPC: Book3S: Fix race and leak in kvm_vm_ioctl_create_spapr_tce()
On 24.08.2017 11:14, Paul Mackerras wrote: > Nixiaoming pointed out that there is a memory leak in > kvm_vm_ioctl_create_spapr_tce() if the call to anon_inode_getfd() > fails; the memory allocated for the kvmppc_spapr_tce_table struct is > not freed, and nor are the pages allocated for the iommu tables. In > addition, we have already incremented the process's count of locked > memory pages, and this doesn't get restored on error. > > David Hildenbrand pointed out that there is a race in that the > function checks early on that there is not already an entry in the > stt->iommu_tables list with the same LIOBN, but an entry with the > same LIOBN could get added between then and when the new entry is > added to the list. > > This fixes all three problems. To simplify things, we now call > anon_inode_getfd() before placing the new entry in the list. The > check for an existing entry is done while holding the kvm->lock mutex, > immediately before adding the new entry to the list. > Finally, on failure we now call kvmppc_account_memlimit to decrement > the process's count of locked memory pages. > > Reported-by: Nixiaoming <nixiaom...@huawei.com> > Reported-by: David Hildenbrand <da...@redhat.com> > Signed-off-by: Paul Mackerras <pau...@ozlabs.org> > --- > v2: Don't overwrite stt in loop over spapr_tce_tables > Reviewed-by: nixiaoming <nixiaom...@huawei.com>
RE: [PATCH] KVM: PPC: Book3S: Fix race and leak in kvm_vm_ioctl_create_spapr_tce()
>From: Paul Mackerras [mailto:pau...@ozlabs.org] Thursday, August 24, 2017
>11:40 AM
>
>Nixiaoming pointed out that there is a memory leak in
>kvm_vm_ioctl_create_spapr_tce() if the call to anon_inode_getfd() fails; the
>memory allocated for the kvmppc_spapr_tce_table struct is not freed, and nor
>are the pages allocated for the iommu tables. In addition, we have already
>incremented the process's count of locked memory pages, and this doesn't get
>restored on error.
>
>David Hildenbrand pointed out that there is a race in that the function checks
>early on that there is not already an entry in the
>stt->iommu_tables list with the same LIOBN, but an entry with the
>same LIOBN could get added between then and when the new entry is added to the
>list.
>
>This fixes all three problems. To simplify things, we now call
>anon_inode_getfd() before placing the new entry in the list. The check for an
>existing entry is done while holding the kvm->lock mutex, immediately before
>adding the new entry to the list.
>Finally, on failure we now call kvmppc_account_memlimit to decrement the
>process's count of locked memory pages.
>
>Reported-by: Nixiaoming <nixiaom...@huawei.com>
>Reported-by: David Hildenbrand <da...@redhat.com>
>Signed-off-by: Paul Mackerras <pau...@ozlabs.org>
>---
> arch/powerpc/kvm/book3s_64_vio.c | 55
> 1 file changed, 33 insertions(+), 22 deletions(-)
>
>diff --git a/arch/powerpc/kvm/book3s_64_vio.c
>b/arch/powerpc/kvm/book3s_64_vio.c
>index a160c14304eb..d463c1cd0d8d 100644
>--- a/arch/powerpc/kvm/book3s_64_vio.c
>+++ b/arch/powerpc/kvm/book3s_64_vio.c
>@@ -297,29 +297,22 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,
> unsigned long npages, size;
> int ret = -ENOMEM;
> int i;
>+ int fd = -1;
>
> if (!args->size)
> return -EINVAL;
>
>- /* Check this LIOBN hasn't been previously allocated */
>- list_for_each_entry(stt, >arch.spapr_tce_tables, list) {
>- if (stt->liobn == args->liobn)
>- return -EBUSY;
>- }
>-
> size = _ALIGN_UP(args->size, PAGE_SIZE >> 3);
> npages = kvmppc_tce_pages(size);
> ret = kvmppc_account_memlimit(kvmppc_stt_pages(npages), true);
>- if (ret) {
>- stt = NULL;
>- goto fail;
>- }
>+ if (ret)
>+ return ret;
>
> ret = -ENOMEM;
> stt = kzalloc(sizeof(*stt) + npages * sizeof(struct page *),
> GFP_KERNEL);
> if (!stt)
>- goto fail;
>+ goto fail_acct;
>
> stt->liobn = args->liobn;
> stt->page_shift = args->page_shift;
>@@ -334,24 +327,42 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,
> goto fail;
> }
>
>- kvm_get_kvm(kvm);
>+ ret = fd = anon_inode_getfd("kvm-spapr-tce", _spapr_tce_fops,
>+ stt, O_RDWR | O_CLOEXEC);
>+ if (ret < 0)
>+ goto fail;
>
> mutex_lock(>lock);
>- list_add_rcu(>list, >arch.spapr_tce_tables);
>+
>+ /* Check this LIOBN hasn't been previously allocated */
>+ ret = 0;
>+ list_for_each_entry(stt, >arch.spapr_tce_tables, list) {
I think stt can not be used here
need a new value for list_for_each_entry
>+ if (stt->liobn == args->liobn) {
>+ ret = -EBUSY;
>+ break;
>+ }
>+ }
>+
>+ if (!ret) {
>+ list_add_rcu(>list, >arch.spapr_tce_tables);
>+ kvm_get_kvm(kvm);
>+ }
>
> mutex_unlock(>lock);
>
>- return anon_inode_getfd("kvm-spapr-tce", _spapr_tce_fops,
>- stt, O_RDWR | O_CLOEXEC);
>+ if (!ret)
>+ return fd;
>
>-fail:
>- if (stt) {
>- for (i = 0; i < npages; i++)
>- if (stt->pages[i])
>- __free_page(stt->pages[i]);
>+ put_unused_fd(fd);
>
>- kfree(stt);
>- }
>+ fail:
>+ for (i = 0; i < npages; i++)
>+ if (stt->pages[i])
>+ __free_page(stt->pages[i]);
>+
>+ kfree(stt);
>+ fail_acct:
>+ kvmppc_account_memlimit(kvmppc_stt_pages(npages), false);
> return ret;
> }
>
>--
>2.11.0
Thanks
Re: [PATCH] fix memory leak on kvm_vm_ioctl_create_spapr_tce
>On 23.08.2017 08:06, Paul Mackerras wrote:
>> On Wed, Aug 23, 2017 at 01:43:08AM +, Nixiaoming wrote:
>>>> On 22.08.2017 17:15, David Hildenbrand wrote:
>>>>> On 22.08.2017 16:28, nixiaoming wrote:
>>>>>> miss kfree(stt) when anon_inode_getfd return fail so add check
>>>>>> anon_inode_getfd return val, and kfree stt
>>>>>>
>>>>>> Signed-off-by: nixiaoming <nixiaom...@huawei.com>
>>>>>> ---
>>>>>> arch/powerpc/kvm/book3s_64_vio.c | 5 -
>>>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/arch/powerpc/kvm/book3s_64_vio.c
>>>>>> b/arch/powerpc/kvm/book3s_64_vio.c
>>>>>> index a160c14..a0b4459 100644
>>>>>> --- a/arch/powerpc/kvm/book3s_64_vio.c
>>>>>> +++ b/arch/powerpc/kvm/book3s_64_vio.c
>>>>>> @@ -341,8 +341,11 @@ long kvm_vm_ioctl_create_spapr_tce(struct
>>>>>> kvm *kvm,
>>>>>>
>>>>>> mutex_unlock(>lock);
>>>>>>
>>>>>> -return anon_inode_getfd("kvm-spapr-tce", _spapr_tce_fops,
>>>>>> +ret = anon_inode_getfd("kvm-spapr-tce", _spapr_tce_fops,
>>>>>> stt, O_RDWR | O_CLOEXEC);
>>>>>> +if (ret < 0)
>>>>>> +goto fail;
>>>>>> +return ret;
>>>>>>
>>>>>> fail:
>>>>>> if (stt) {
>>>>>>
>>>>>
>>>>>
>>>>> stt has already been added to kvm->arch.spapr_tce_tables, so
>>>>> freeing it is evil IMHO. I don't know that code, so I don't know
>>>>> if there is some other place that will make sure that everything
>>>>> in
>>>>> kvm->arch.spapr_tce_tables will properly get freed, even when no
>>>>> kvm->release
>>>>> function has been called (kvm_spapr_tce_release).
>>>>>
>>>>
>>>> If it is really not freed, than also kvm_put_kvm(stt->kvm) is missing.
>>>>
>>>> --
>>>>
>>>> Thanks,
>>>>
>>>> David
>>>>
>>>
>>> if (!stt) return -ENOMEM;
>>> kvm_get_kvm(kvm);
>>> if anon_inode_getfd return -ENOMEM
>>> The user can not determine whether kvm_get_kvm has been called so
>>> need add kvm_pet_kvm when anon_inode_getfd fail
>>>
>>> stt has already been added to kvm->arch.spapr_tce_tables, but if
>>> anon_inode_getfd fail, stt is unused val, so call list_del_rcu, and
>>> free as quickly as possible
>>>
>>> new patch:
>>>
>>> ---
>>> arch/powerpc/kvm/book3s_64_vio.c | 10 +-
>>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/powerpc/kvm/book3s_64_vio.c
>>> b/arch/powerpc/kvm/book3s_64_vio.c
>>> index a160c14..e2228f1 100644
>>> --- a/arch/powerpc/kvm/book3s_64_vio.c
>>> +++ b/arch/powerpc/kvm/book3s_64_vio.c
>>> @@ -341,8 +341,16 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm
>>> *kvm,
>>>
>>> mutex_unlock(>lock);
>>>
>>> - return anon_inode_getfd("kvm-spapr-tce", _spapr_tce_fops,
>>> + ret = anon_inode_getfd("kvm-spapr-tce", _spapr_tce_fops,
>>> stt, O_RDWR | O_CLOEXEC);
>>> + if (ret < 0) {
>>> + mutex_lock(>lock);
>>> + list_del_rcu(>list);
>
>... don't we have to take care of rcu synchronization before freeing it?
>
>>> + mutex_unlock(>lock);
>>> + kvm_put_kvm(kvm);
>>> + goto fail;
>>> + }
>>> + return ret;
>
>of simply
>
>if (!ret)
> return 0;
>
>mutex_lock(>lock);
>list_del_rcu(>list);
>mutex_unlock(>lock);
>kvm_put_kvm(kvm);
>
>
>>
>> It seems to me that it would be better to do the anon_inode_getfd()
>> call before the kvm_get_kvm() call, and go to the fail label if it
>> fails.
>
>I would have suggested to not add it to the list before it has been
>fully created (so nobody can have access to it). But I guess than we
>need another level of protection(e.g. kvm->lock).
>
>Am I missing
答复: [PATCH] fix memory leak on kvm_vm_ioctl_create_spapr_tce
>On 23.08.2017 08:06, Paul Mackerras wrote:
>> On Wed, Aug 23, 2017 at 01:43:08AM +, Nixiaoming wrote:
>>>> On 22.08.2017 17:15, David Hildenbrand wrote:
>>>>> On 22.08.2017 16:28, nixiaoming wrote:
>>>>>> miss kfree(stt) when anon_inode_getfd return fail so add check
>>>>>> anon_inode_getfd return val, and kfree stt
>>>>>>
>>>>>> Signed-off-by: nixiaoming <nixiaom...@huawei.com>
>>>>>> ---
>>>>>> arch/powerpc/kvm/book3s_64_vio.c | 5 -
>>>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/arch/powerpc/kvm/book3s_64_vio.c
>>>>>> b/arch/powerpc/kvm/book3s_64_vio.c
>>>>>> index a160c14..a0b4459 100644
>>>>>> --- a/arch/powerpc/kvm/book3s_64_vio.c
>>>>>> +++ b/arch/powerpc/kvm/book3s_64_vio.c
>>>>>> @@ -341,8 +341,11 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm
>>>>>> *kvm,
>>>>>>
>>>>>> mutex_unlock(>lock);
>>>>>>
>>>>>> -return anon_inode_getfd("kvm-spapr-tce", _spapr_tce_fops,
>>>>>> +ret = anon_inode_getfd("kvm-spapr-tce", _spapr_tce_fops,
>>>>>> stt, O_RDWR | O_CLOEXEC);
>>>>>> +if (ret < 0)
>>>>>> +goto fail;
>>>>>> +return ret;
>>>>>>
>>>>>> fail:
>>>>>> if (stt) {
>>>>>>
>>>>>
>>>>>
>>>>> stt has already been added to kvm->arch.spapr_tce_tables, so freeing
>>>>> it is evil IMHO. I don't know that code, so I don't know if there is
>>>>> some other place that will make sure that everything in
>>>>> kvm->arch.spapr_tce_tables will properly get freed, even when no
>>>>> kvm->release
>>>>> function has been called (kvm_spapr_tce_release).
>>>>>
>>>>
>>>> If it is really not freed, than also kvm_put_kvm(stt->kvm) is missing.
>>>>
>>>> --
>>>>
>>>> Thanks,
>>>>
>>>> David
>>>>
>>>
>>> if (!stt) return -ENOMEM;
>>> kvm_get_kvm(kvm);
>>> if anon_inode_getfd return -ENOMEM
>>> The user can not determine whether kvm_get_kvm has been called
>>> so need add kvm_pet_kvm when anon_inode_getfd fail
>>>
>>> stt has already been added to kvm->arch.spapr_tce_tables,
>>> but if anon_inode_getfd fail, stt is unused val,
>>> so call list_del_rcu, and free as quickly as possible
>>>
>>> new patch:
>>>
>>> ---
>>> arch/powerpc/kvm/book3s_64_vio.c | 10 +-
>>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/powerpc/kvm/book3s_64_vio.c
>>> b/arch/powerpc/kvm/book3s_64_vio.c
>>> index a160c14..e2228f1 100644
>>> --- a/arch/powerpc/kvm/book3s_64_vio.c
>>> +++ b/arch/powerpc/kvm/book3s_64_vio.c
>>> @@ -341,8 +341,16 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,
>>>
>>> mutex_unlock(>lock);
>>>
>>> - return anon_inode_getfd("kvm-spapr-tce", _spapr_tce_fops,
>>> + ret = anon_inode_getfd("kvm-spapr-tce", _spapr_tce_fops,
>>> stt, O_RDWR | O_CLOEXEC);
>>> + if (ret < 0) {
>>> + mutex_lock(>lock);
>>> + list_del_rcu(>list);
>
>... don't we have to take care of rcu synchronization before freeing it?
>
>>> + mutex_unlock(>lock);
>>> + kvm_put_kvm(kvm);
>>> + goto fail;
>>> + }
>>> + return ret;
>
>of simply
>
>if (!ret)
> return 0;
>
>mutex_lock(>lock);
>list_del_rcu(>list);
>mutex_unlock(>lock);
>kvm_put_kvm(kvm);
>
>
>>
>> It seems to me that it would be better to do the anon_inode_getfd()
>> call before the kvm_get_kvm() call, and go to the fail label if it
>> fails.
>
>I would have suggested to not add it to the list before it has been
>fully created (so nobody can have access to it). But I guess than we
>need another level of protection(e.g. kvm->lock).
>
>Am I missing something, or is kvm_vm_ioctl_create_spapr_tc
Re:Re: [PATCH] fix memory leak on kvm_vm_ioctl_create_spapr_tce
>On 22.08.2017 17:15, David Hildenbrand wrote:
>> On 22.08.2017 16:28, nixiaoming wrote:
>>> miss kfree(stt) when anon_inode_getfd return fail so add check
>>> anon_inode_getfd return val, and kfree stt
>>>
>>> Signed-off-by: nixiaoming <nixiaom...@huawei.com>
>>> ---
>>> arch/powerpc/kvm/book3s_64_vio.c | 5 -
>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/powerpc/kvm/book3s_64_vio.c
>>> b/arch/powerpc/kvm/book3s_64_vio.c
>>> index a160c14..a0b4459 100644
>>> --- a/arch/powerpc/kvm/book3s_64_vio.c
>>> +++ b/arch/powerpc/kvm/book3s_64_vio.c
>>> @@ -341,8 +341,11 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm
>>> *kvm,
>>>
>>> mutex_unlock(>lock);
>>>
>>> - return anon_inode_getfd("kvm-spapr-tce", _spapr_tce_fops,
>>> + ret = anon_inode_getfd("kvm-spapr-tce", _spapr_tce_fops,
>>> stt, O_RDWR | O_CLOEXEC);
>>> + if (ret < 0)
>>> + goto fail;
>>> + return ret;
>>>
>>> fail:
>>> if (stt) {
>>>
>>
>>
>> stt has already been added to kvm->arch.spapr_tce_tables, so freeing
>> it is evil IMHO. I don't know that code, so I don't know if there is
>> some other place that will make sure that everything in
>> kvm->arch.spapr_tce_tables will properly get freed, even when no
>> kvm->release
>> function has been called (kvm_spapr_tce_release).
>>
>
>If it is really not freed, than also kvm_put_kvm(stt->kvm) is missing.
>
>--
>
>Thanks,
>
>David
>
if (!stt) return -ENOMEM;
kvm_get_kvm(kvm);
if anon_inode_getfd return -ENOMEM
The user can not determine whether kvm_get_kvm has been called
so need add kvm_pet_kvm when anon_inode_getfd fail
stt has already been added to kvm->arch.spapr_tce_tables,
but if anon_inode_getfd fail, stt is unused val,
so call list_del_rcu, and free as quickly as possible
new patch:
---
arch/powerpc/kvm/book3s_64_vio.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c
index a160c14..e2228f1 100644
--- a/arch/powerpc/kvm/book3s_64_vio.c
+++ b/arch/powerpc/kvm/book3s_64_vio.c
@@ -341,8 +341,16 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,
mutex_unlock(>lock);
- return anon_inode_getfd("kvm-spapr-tce", _spapr_tce_fops,
+ ret = anon_inode_getfd("kvm-spapr-tce", _spapr_tce_fops,
stt, O_RDWR | O_CLOEXEC);
+ if (ret < 0) {
+ mutex_lock(>lock);
+ list_del_rcu(>list);
+ mutex_unlock(>lock);
+ kvm_put_kvm(kvm);
+ goto fail;
+ }
+ return ret;
fail:
if (stt) {
--
2.11.0.1
[PATCH] fix memory leak on kvm_vm_ioctl_create_spapr_tce
miss kfree(stt) when anon_inode_getfd return fail
so add check anon_inode_getfd return val, and kfree stt
Signed-off-by: nixiaoming <nixiaom...@huawei.com>
---
arch/powerpc/kvm/book3s_64_vio.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c
index a160c14..a0b4459 100644
--- a/arch/powerpc/kvm/book3s_64_vio.c
+++ b/arch/powerpc/kvm/book3s_64_vio.c
@@ -341,8 +341,11 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,
mutex_unlock(>lock);
- return anon_inode_getfd("kvm-spapr-tce", _spapr_tce_fops,
+ ret = anon_inode_getfd("kvm-spapr-tce", _spapr_tce_fops,
stt, O_RDWR | O_CLOEXEC);
+ if (ret < 0)
+ goto fail;
+ return ret;
fail:
if (stt) {
--
2.11.0.1
[PATCH] fix memory leak on kvm_vm_ioctl_get_htab_fd
ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
but no free when anon_inode_getfd return fail
so, add kfree(ctx) to fix memory leak
Signed-off-by: nixiaoming <nixiaom...@huawei.com>
---
arch/powerpc/kvm/book3s_64_mmu_hv.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c
b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index b42812e..be3d08f 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -1940,6 +1940,7 @@ int kvm_vm_ioctl_get_htab_fd(struct kvm *kvm, struct
kvm_get_htab_fd *ghf)
rwflag = (ghf->flags & KVM_GET_HTAB_WRITE) ? O_WRONLY : O_RDONLY;
ret = anon_inode_getfd("kvm-htab", _htab_fops, ctx, rwflag |
O_CLOEXEC);
if (ret < 0) {
+ kfree(ctx);
kvm_put_kvm(kvm);
return ret;
}
--
2.11.0.1
