From mboxrd@z Thu Jan 1 00:00:00 1970 From: Cyrill Gorcunov Subject: Re: [PATCH] kvm tool: Don't close not yet opened files and SIGSEV fix Date: Mon, 6 Feb 2012 00:19:02 +0400 Message-ID: <20120205201902.GA13095@moon> References: <20120204163204.GB15888@moon> <20120204180219.GD14818@moon> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii To: Pekka Enberg , Sasha Levin , Asias He , Ingo Molnar , KVM-ML Return-path: Received: from mail-bk0-f46.google.com ([209.85.214.46]:59187 "EHLO mail-bk0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750970Ab2BEUTH (ORCPT ); Sun, 5 Feb 2012 15:19:07 -0500 Received: by bkcjm19 with SMTP id jm19so4220746bkc.19 for ; Sun, 05 Feb 2012 12:19:05 -0800 (PST) Content-Disposition: inline In-Reply-To: <20120204180219.GD14818@moon> Sender: kvm-owner@vger.kernel.org List-ID: On Sat, Feb 04, 2012 at 10:02:19PM +0400, Cyrill Gorcunov wrote: > > Strictly speaking, kvm__init need more serious rewrite together with > kvm__arch_init/kvm_ipc__start/kvm_ipc__register_handler ret. vals tests, > i'll do this a bit late. > Sorry for delay, was busy. Anyway, here is a quickfix for sigsev and close()s. Cyrill --- Subject: kvm tool: Don't close not yet opened files and SIGSEV fix In case if there error happened in kvm__init and we have no files opened -- we should not try to close them. Also once kvm failed to init the caller should not try to dereference a pointer obtained, otherwise we might get SIGSEV | [cyrill@moon kvm]$ ./lkvm run ... | Error: '/dev/kvm' not found. Please make sure your kernel has CONFIG_KVM | enabled and that the KVM modules are loaded. | Segmentation fault (core dumped) | [cyrill@moon kvm]$ Signed-off-by: Cyrill Gorcunov --- tools/kvm/builtin-run.c | 4 ++++ tools/kvm/kvm.c | 41 +++++++++++++++++++++-------------------- 2 files changed, 25 insertions(+), 20 deletions(-) Index: linux-2.6.git/tools/kvm/builtin-run.c =================================================================== --- linux-2.6.git.orig/tools/kvm/builtin-run.c +++ linux-2.6.git/tools/kvm/builtin-run.c @@ -997,6 +997,10 @@ static int kvm_cmd_run_init(int argc, co } kvm = kvm__init(dev, hugetlbfs_path, ram_size, guest_name); + if (IS_ERR(kvm)) { + r = PTR_ERR(kvm); + goto fail; + } kvm->single_step = single_step; Index: linux-2.6.git/tools/kvm/kvm.c =================================================================== --- linux-2.6.git.orig/tools/kvm/kvm.c +++ linux-2.6.git/tools/kvm/kvm.c @@ -123,10 +123,12 @@ static int kvm__check_extensions(struct static struct kvm *kvm__new(void) { struct kvm *kvm = calloc(1, sizeof(*kvm)); - if (!kvm) return ERR_PTR(-ENOMEM); + kvm->sys_fd = -1; + kvm->vm_fd = -1; + return kvm; } @@ -341,47 +343,42 @@ struct kvm *kvm__init(const char *kvm_de } kvm = kvm__new(); - if (IS_ERR_OR_NULL(kvm)) + if (IS_ERR(kvm)) return kvm; kvm->sys_fd = open(kvm_dev, O_RDWR); if (kvm->sys_fd < 0) { - if (errno == ENOENT) { + if (errno == ENOENT) pr_err("'%s' not found. Please make sure your kernel has CONFIG_KVM " - "enabled and that the KVM modules are loaded.", kvm_dev); - ret = -errno; - goto cleanup; - } - if (errno == ENODEV) { - die("'%s' KVM driver not available.\n # (If the KVM " - "module is loaded then 'dmesg' may offer further clues " - "about the failure.)", kvm_dev); - ret = -errno; - goto cleanup; - } + "enabled and that the KVM modules are loaded.", kvm_dev); + else if (errno == ENODEV) + pr_err("'%s' KVM driver not available.\n # (If the KVM " + "module is loaded then 'dmesg' may offer further clues " + "about the failure.)", kvm_dev); + else + pr_err("Could not open %s: ", kvm_dev); - pr_err("Could not open %s: ", kvm_dev); ret = -errno; - goto cleanup; + goto err_free; } ret = ioctl(kvm->sys_fd, KVM_GET_API_VERSION, 0); if (ret != KVM_API_VERSION) { pr_err("KVM_API_VERSION ioctl"); ret = -errno; - goto cleanup; + goto err_sys_fd; } kvm->vm_fd = ioctl(kvm->sys_fd, KVM_CREATE_VM, 0); if (kvm->vm_fd < 0) { ret = kvm->vm_fd; - goto cleanup; + goto err_sys_fd; } kvm->name = strdup(name); if (!kvm->name) { ret = -ENOMEM; - goto cleanup; + goto err; } if (kvm__check_extensions(kvm)) { @@ -393,10 +390,14 @@ struct kvm *kvm__init(const char *kvm_de kvm_ipc__start(kvm__create_socket(kvm)); kvm_ipc__register_handler(KVM_IPC_PID, kvm__pid); + return kvm; -cleanup: + +err: close(kvm->vm_fd); +err_sys_fd: close(kvm->sys_fd); +err_free: free(kvm); return ERR_PTR(ret);