From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [PATCH v6 10/36] ARM: GIC: Add checks for NULL pointer pending_irq's Date: Fri, 7 Apr 2017 15:09:24 -0700 (PDT) Message-ID: References: <20170407173307.9788-1-andre.przywara@arm.com> <20170407173307.9788-11-andre.przywara@arm.com> <955cc9f3-ac37-472f-41f2-a88836e505ec@arm.com> Mime-Version: 1.0 Content-Type: MULTIPART/MIXED; BOUNDARY="8323329-121922306-1491602965=:2759" Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cwc4Q-0007iQ-Fr for xen-devel@lists.xenproject.org; Fri, 07 Apr 2017 22:09:30 +0000 In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Stefano Stabellini Cc: =?UTF-8?Q?Andr=C3=A9_Przywara?= , Julien Grall , Vijay Kilari , Shanker Donthineni , xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323329-121922306-1491602965=:2759 Content-Type: TEXT/PLAIN; charset=UTF-8 Content-Transfer-Encoding: 8BIT On Fri, 7 Apr 2017, Stefano Stabellini wrote: > On Fri, 7 Apr 2017, Julien Grall wrote: > > Hi Andre, > > > > On 04/07/2017 09:46 PM, André Przywara wrote: > > > On 07/04/17 20:07, Stefano Stabellini wrote: > > > > On Fri, 7 Apr 2017, Andre Przywara wrote: > > > > > For LPIs the struct pending_irq's are somewhat dynamically allocated and > > > > > the pointers are stored in a radix tree. While I convinced myself that > > > > > an invalid LPI number can't make it into the core code, people might be > > > > > concerned about NULL pointer dereferences. > > > > > So add checks in some places just to be on the safe side. > > > > > > > > > > Signed-off-by: Andre Przywara > > > > > > > > This approach looks fragile: what if we miss one irq_to_pending call? We > > > > need a way to avoid pending_irq structs being freed when an irq is still > > > > inflight. Only after an irq is not inflight anymore, a struct > > > > pending_irq could be freed. > > > > > > Indeed. I was wondering if a dummy pend_irq could help on the first > > > step: Upon unmapping, we replace the radix-tree member(s) with this one > > > reserved instance (per domain), that would avoid the NULL pointer > > > dereference. > > > > The problem with that is the code will continue to handle it, but as it is a > > dummy pending_irq (I understand there will be only one existing) you will > > corrupt the list. > > > > Does that sound like a possible route? > > > > How about using a reference (either on the device or the pending_irq)? A > > reference would be taken until the vLPI is correctly handled (i.e clear from > > the LRs). > > > > This would prevent complex locking. > > We need to mark the pending_irq "to be freed", like we do with migration > (GIC_IRQ_GUEST_MIGRATING marks an irq to be migrated). Afterwards, when > the irq is ready, it will removed from the tree and freed. I'll clarify. Andre, give a look at how vgic_migrate_irq is implemented: first it tries to migrate the irq immediately, but if it is not possible, it will mark the irq "to be migrated". Then, in gic_update_one_lr, we check for the "to be migrated" flag and do the actual migration. We need to introduce a similar workflow here. If we can remove pending_irq from the tree, we do it immediately. Otherwise we mark it "to be removed" and do it later as soon as we can. We also add check for the "to be removed" flag in other places, where appropriate, to avoid trying to inject another LPI that is already supposed to be removed. I hope that's clearer. --8323329-121922306-1491602965=:2759 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --8323329-121922306-1491602965=:2759--