On Wed, Aug 23, 2017 at 01:43:08AM +0000, 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 <nixiaoming@xxxxxxxxxx>
> >>> ---
> >>> 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(&kvm->lock);
> >>>
> >>> - return anon_inode_getfd("kvm-spapr-tce", &kvm_spapr_tce_fops,
> >>> + ret = anon_inode_getfd("kvm-spapr-tce", &kvm_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(&kvm->lock);
>
> - return anon_inode_getfd("kvm-spapr-tce", &kvm_spapr_tce_fops,
> + ret = anon_inode_getfd("kvm-spapr-tce", &kvm_spapr_tce_fops,
> stt, O_RDWR | O_CLOEXEC);
> + if (ret < 0) {
> + mutex_lock(&kvm->lock);
> + list_del_rcu(&stt->list);
> + mutex_unlock(&kvm->lock);
> + kvm_put_kvm(kvm);
> + goto fail;
> + }
> + return ret;
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.
Paul.
