From patchwork Tue Aug 2 19:49:30 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christoffer Dall X-Patchwork-Id: 73181 Delivered-To: patch@linaro.org Received: by 10.140.29.52 with SMTP id a49csp338803qga; Tue, 2 Aug 2016 12:50:01 -0700 (PDT) X-Received: by 10.98.34.151 with SMTP id p23mr109606378pfj.102.1470167401567; Tue, 02 Aug 2016 12:50:01 -0700 (PDT) Return-Path: Received: from bombadil.infradead.org (bombadil.infradead.org. [2001:1868:205::9]) by mx.google.com with ESMTPS id h127si4433301pfb.251.2016.08.02.12.50.01 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 02 Aug 2016 12:50:01 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org designates 2001:1868:205::9 as permitted sender) client-ip=2001:1868:205::9; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@linaro.org; spf=pass (google.com: best guess record for domain of linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org designates 2001:1868:205::9 as permitted sender) smtp.mailfrom=linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org; dmarc=fail (p=NONE dis=NONE) header.from=linaro.org Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.85_2 #1 (Red Hat Linux)) id 1bUffp-0007A3-6F; Tue, 02 Aug 2016 19:48:21 +0000 Received: from mail-wm0-x231.google.com ([2a00:1450:400c:c09::231]) by bombadil.infradead.org with esmtps (Exim 4.85_2 #1 (Red Hat Linux)) id 1bUffi-00077f-Eh for linux-arm-kernel@lists.infradead.org; Tue, 02 Aug 2016 19:48:16 +0000 Received: by mail-wm0-x231.google.com with SMTP id i5so305508070wmg.0 for ; Tue, 02 Aug 2016 12:47:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=mJzjHnQu2+cTev/nPpoOWskf1oRxoUKZgPfAfN+jupg=; b=idddkUVRU2EkiY/76vet225thpoglpC0wbpgMgZOwMUZ5DEqOMvgNcrGu51EWy1LkF /CBfIpoSYpioh6WRcRewhdPtJwy57Hx7ElPLuJnqbf5o0eXUaeER9bsnEJLgXkziWpke 2R4tFvrfFyTxMEcMJlJ5d7QbL8hQTurAn4bOY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=mJzjHnQu2+cTev/nPpoOWskf1oRxoUKZgPfAfN+jupg=; b=FdghxlCs0pVgyhhcKcGfpw/WjB3/pS+AQi0YasFPvpwigivuVTgOWehJR0+tFxVjY0 y95J5xcg9S0sgSgYS/t7n9G++LN+3sp9pQOs2hrRiUV3+SgrEvDnnmUtO2FDxsFHQ6c9 Nx4V4oMH1pV1xOQHMSochwjZ7j4pPqu7Y7/MxdPzQTg+wyY22SAx1m/uLKv0HszLOHok Vi110W9/JE0qihPyFD+T1c6KJ5lVeXoYwWAZNLV3rgsr6/KxPaAa7bAinHgjba5JJpZU jShBmGiQu36uHuAzYLrjVExkUgrLha+RK/4eJg+pubIvvhFv5B9Sy1HdwddJTFl3RQas +L6w== X-Gm-Message-State: AEkooutvlY3ck9xn0qc2K3p+b+KcfRHmsQtPbXcvUxoDP/mUvfqHqfl8zwby53CO3ocyxvQD X-Received: by 10.28.39.134 with SMTP id n128mr20197339wmn.60.1470167271711; Tue, 02 Aug 2016 12:47:51 -0700 (PDT) Received: from localhost ([94.18.191.146]) by smtp.gmail.com with ESMTPSA id b186sm4473162wmg.23.2016.08.02.12.47.50 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Tue, 02 Aug 2016 12:47:51 -0700 (PDT) Date: Tue, 2 Aug 2016 21:49:30 +0200 From: Christoffer Dall To: Marc Zyngier , Andre Przywara Subject: Re: [PATCH] KVM: arm64: vgic-its: Handle errors from vgic_add_lpi Message-ID: <20160802194930.GV32244@cbox> References: <20160801182952.3005-1-christoffer.dall@linaro.org> <57A0776F.5070401@arm.com> <20160802150834.GT32244@cbox> <57A0B9D6.8070304@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <57A0B9D6.8070304@arm.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20160802_124814_996672_0666E5AF X-CRM114-Status: GOOD ( 33.26 ) X-Spam-Score: -2.7 (--) X-Spam-Report: SpamAssassin version 3.4.0 on bombadil.infradead.org summary: Content analysis details: (-2.7 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [2a00:1450:400c:c09:0:0:0:231 listed in] [list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org On Tue, Aug 02, 2016 at 04:18:46PM +0100, Marc Zyngier wrote: > On 02/08/16 16:08, Christoffer Dall wrote: > > On Tue, Aug 02, 2016 at 11:35:27AM +0100, Marc Zyngier wrote: > >> On 01/08/16 19:29, Christoffer Dall wrote: > >>> During low memory conditions, we could be dereferencing a NULL pointer > >>> when vgic_add_lpi fails to allocate memory. > >>> > >>> Consider for example this call sequence: > >>> > >>> vgic_its_cmd_handle_mapi > >>> itte->irq = vgic_add_lpi(kvm, lpi_nr); > >>> update_lpi_config(kvm, itte->irq, NULL); > >>> ret = kvm_read_guest(kvm, propbase + irq->intid > >>> ^^^^ > >>> kaboom? > >>> > >>> Instead, return an error pointer from vgic_add_lpi and check the return > >>> value from its single caller. > >>> > >>> Signed-off-by: Christoffer Dall > >>> --- > >>> virt/kvm/arm/vgic/vgic-its.c | 42 +++++++++++++++++++++++++++++------------- > >>> 1 file changed, 29 insertions(+), 13 deletions(-) > >>> > >>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c > >>> index 07411cf..3515bdb 100644 > >>> --- a/virt/kvm/arm/vgic/vgic-its.c > >>> +++ b/virt/kvm/arm/vgic/vgic-its.c > >>> @@ -51,7 +51,7 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid) > >>> > >>> irq = kzalloc(sizeof(struct vgic_irq), GFP_KERNEL); > >>> if (!irq) > >>> - return NULL; > >>> + return ERR_PTR(-ENOMEM); > >>> > >>> INIT_LIST_HEAD(&irq->lpi_list); > >>> INIT_LIST_HEAD(&irq->ap_list); > >>> @@ -697,24 +697,33 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its, > >>> struct its_device *device; > >>> struct its_collection *collection, *new_coll = NULL; > >>> int lpi_nr; > >>> - > >>> - device = find_its_device(its, device_id); > >>> - if (!device) > >>> - return E_ITS_MAPTI_UNMAPPED_DEVICE; > >>> + struct vgic_irq *irq = NULL; > >>> + int err = 0; > >>> > >>> if (its_cmd_get_command(its_cmd) == GITS_CMD_MAPTI) > >>> lpi_nr = its_cmd_get_physical_id(its_cmd); > >>> else > >>> lpi_nr = event_id; > >>> + > >>> if (lpi_nr < GIC_LPI_OFFSET || > >>> lpi_nr >= max_lpis_propbaser(kvm->arch.vgic.propbaser)) > >>> return E_ITS_MAPTI_PHYSICALID_OOR; > >>> > >>> + irq = vgic_add_lpi(kvm, lpi_nr); > >>> + if (IS_ERR(irq)) > >>> + return PTR_ERR(irq); > >>> + > >>> + device = find_its_device(its, device_id); > >>> + if (!device) { > >>> + err = E_ITS_MAPTI_UNMAPPED_DEVICE; > >>> + goto out; > >>> + } > >>> + > >>> collection = find_collection(its, coll_id); > >>> if (!collection) { > >>> - int ret = vgic_its_alloc_collection(its, &collection, coll_id); > >>> - if (ret) > >>> - return ret; > >>> + err = vgic_its_alloc_collection(its, &collection, coll_id); > >>> + if (err) > >>> + goto out; > >>> new_coll = collection; > >>> } > >>> > >>> @@ -722,9 +731,8 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its, > >>> if (!itte) { > >>> itte = kzalloc(sizeof(struct its_itte), GFP_KERNEL); > >>> if (!itte) { > >>> - if (new_coll) > >>> - vgic_its_free_collection(its, coll_id); > >>> - return -ENOMEM; > >>> + err = -ENOMEM; > >>> + goto out; > >>> } > >>> > >>> itte->event_id = event_id; > >>> @@ -733,7 +741,8 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its, > >>> > >>> itte->collection = collection; > >>> itte->lpi = lpi_nr; > >>> - itte->irq = vgic_add_lpi(kvm, lpi_nr); > >>> + vgic_get_irq_kref(irq); > >>> + itte->irq = irq; > >>> update_affinity_itte(kvm, itte); > >>> > >>> /* > >>> @@ -742,8 +751,15 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its, > >>> * the respective config data from memory here upon mapping the LPI. > >>> */ > >>> update_lpi_config(kvm, itte->irq, NULL); > >>> + new_coll = NULL; > >>> + irq = NULL; > >>> > >>> - return 0; > >>> +out: > >>> + if (new_coll) > >>> + vgic_its_free_collection(its, coll_id); > >>> + if (irq) > >>> + vgic_put_irq(kvm, irq); > >> > >> Ah, it took me a moment to understand why you had the > >> vgic_irq_get_kref() above. Maybe a small comment? > >> > > > > actually, now I'm confused. > > > > vgic_add_lpi returns a struct vgic_irq with a reference count for the > > reference in the itte struct, right? > > It either returns a vgic_irq with a refcount set to 1 (freshly > allocated), or a previously allocated one with the refcount already > incremented. > > > So we never get a reference for the irq->lpi_list/dist->lpi_list_head ? > > I don't think so. being part of the lpi_list is a part of the LPI > "existence". hmm, ok, but since we're not holding the lock while decrementing the ref count(only after we evaluate kref_put) it looks to me like you can end up with a reference to a freed structure. Basically, I think we need this: Thoughts? > > > In which case my code above is wrong and I should not be getting the > > extra reference. Right? > > Ah, I just noticed that you are nullifying "irq". Either you don't > nullify it (and keep the kref_get), or remove both kref_get/kref_put, > but that makes your error handling a bit mot complicated. > I'll rewrite this patch. > > Now, this made me think of another issue. vgic_add_lpi has a blurb in > > there about racing with another vgic_add_lpi and then it returns an irq > > with an additional reference. But I don't understand how this can > > happen, given that the function only has a single caller which has to > > run with a mutex held??? > > The mutex is per-ITS, and you can have several ITSs mapping the same LPI > (provided that they are generated by the same devid/evid). > > > Can two different itte's point to the same struct vgic_irq ? > > Definitely, if they are on different ITSs. > > Does it help? > Yes, this helps, and gives meaning to the comment in the function. Thanks! -Christoffer _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c index e7aeac7..fb8c0ab 100644 --- a/virt/kvm/arm/vgic/vgic.c +++ b/virt/kvm/arm/vgic/vgic.c @@ -117,22 +117,22 @@ static void vgic_irq_release(struct kref *ref) void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq) { - struct vgic_dist *dist; + struct vgic_dist *dist = &kvm->arch.vgic; if (irq->intid < VGIC_MIN_LPI) return; - if (!kref_put(&irq->refcount, vgic_irq_release)) - return; - - dist = &kvm->arch.vgic; - spin_lock(&dist->lpi_list_lock); - list_del(&irq->lpi_list); - dist->lpi_list_count--; - spin_unlock(&dist->lpi_list_lock); + if (!kref_put(&irq->refcount, vgic_irq_release)) { + spin_unlock(&dist->lpi_list_lock); + return; + } else { + list_del(&irq->lpi_list); + dist->lpi_list_count--; + spin_unlock(&dist->lpi_list_lock); - kfree(irq); + kfree(irq); + } } /**