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: 9260305 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id AB2026048B for ; Tue, 2 Aug 2016 19:55:52 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 9CF8628346 for ; Tue, 2 Aug 2016 19:55:52 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 8F72C284F9; Tue, 2 Aug 2016 19:55:52 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 7DD5B28346 for ; Tue, 2 Aug 2016 19:55:51 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755307AbcHBTzs (ORCPT ); Tue, 2 Aug 2016 15:55:48 -0400 Received: from mail-wm0-f49.google.com ([74.125.82.49]:37741 "EHLO mail-wm0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751514AbcHBTzq (ORCPT ); Tue, 2 Aug 2016 15:55:46 -0400 Received: by mail-wm0-f49.google.com with SMTP id i5so305770165wmg.0 for ; Tue, 02 Aug 2016 12:55:45 -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=kMVtrAtZoYZizObMI9TYmIUrOBGPRWRa4dl8eybgOe32IffbXbWQB0EgZl7c/ipsv4 PiVa0IFga5hU4LfgXCBf1kvO8yflP/MwIvfBCk6XvOVlRA2ZqzmKejckqQPR0MnjIsyE C4P38Qw4x8Kf8wSjFY+eaT75n+j0nq25TWNcmEGA7+7jIPT5XkbKEriAwoGMGuoZkGtK TMrS86fQggyWJqLELl5dNeMclYm4t/rPQb6/0hQcv4C0ViL6MpzL8NaSizX/a4xWs3Qt HMSwtU5ct1ObAxvlYWdggmXVH1OxFKOYHIzAmkayo65V7++eiBi2G0FrBsvhbrhTqGVU byew== X-Gm-Message-State: AEkooutmGl4pCBnxuJQX9Dt8cpdBEfulq/Ef4Bdvho0+XCwHy+8DAw9jghCfZQrGuN9IMTf9 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 Cc: kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org 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) Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 --- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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); + } } /**