Xen project Mailing List

On Fri, Feb 22, 2019 at 11:42:22AM +0100, Roger Pau Monné wrote: > On Thu, Feb 21, 2019 at 06:40:40PM +0100, Marek Marczykowski-Górecki wrote: > > On Thu, Feb 21, 2019 at 05:47:51PM +0100, Roger Pau Monné wrote: > > > On Fri, Feb 08, 2019 at 11:17:05AM +0100, Marek Marczykowski-Górecki > > > wrote: > > > > { > > > > int irq, ret; > > > > struct irq_desc *desc; > > > > @@ -190,19 +190,19 @@ int create_irq(nodeid_t node) > > > > desc->arch.used = IRQ_UNUSED; > > > > irq = ret; > > > > } > > > > - else if ( hardware_domain ) > > > > + else if ( dm_domain ) > > > > > > Can you guarantee that dm_domain is always current->domain? > > > > No, in some cases it will be hardware_domain. > > Right, but in that case current->domain == hardware_domain I guess? > > > > > > I think you need to assert for this, or else take a reference to > > > dm_domain (get_domain) before accessing it's fields, or else you risk > > > the domain being destroyed while modifying it's fields. > > > > Can hardware_domain be destroyed without panicking Xen? If so, > > get_domain would be indeed needed. > > What about other callers that are not the hardware_domain? You need to > make sure those domains are not destroyed while {create/destroy}_irq > is changing the permissions. > > If you can guarantee that dm_domain == current->domain then you are > safe, if not you need to get a reference before modifying any fields > of the domain struct. > > So IMO you either need to add an assert or a get_domain depending on > the answer to the question above. I've added an assert, and I think I have the answer to the above question: (XEN) Assertion 'd == current->domain' failed at irq.c:232 (XEN) ----[ Xen-4.12.0-rc x86_64 debug=y Not tainted ]---- (XEN) CPU: 2 (XEN) RIP: e008:[<ffff82d08028f545>] destroy_irq+0x126/0x143 (XEN) RFLAGS: 0000000000010206 CONTEXT: hypervisor (...) (XEN) Xen call trace: (XEN) [<ffff82d08028f545>] destroy_irq+0x126/0x143 (XEN) [<ffff82d08028ce1e>] msi_free_irq+0x51/0x1b8 (XEN) [<ffff82d0802923e1>] unmap_domain_pirq+0x487/0x4d4 (XEN) [<ffff82d08029249f>] free_domain_pirqs+0x71/0x8f (XEN) [<ffff82d0802819e0>] arch_domain_destroy+0x41/0xa1 (XEN) [<ffff82d080207d22>] domain.c#complete_domain_destroy+0x86/0x159 (XEN) [<ffff82d08022a658>] rcupdate.c#rcu_process_callbacks+0xa5/0x1cc (XEN) [<ffff82d08023c4fa>] softirq.c#__do_softirq+0x78/0x9a (XEN) [<ffff82d08023c566>] do_softirq+0x13/0x15 (XEN) [<ffff82d080280532>] domain.c#idle_loop+0x63/0xb9 In this case, current->domain obviously isn't the stubdomain, because at this point it is already destroyed (it keeps reference to the target domain, so target domain couldn't be destroyed before its stubdomain). In fact, in this case get_dm_domain() returns wrong value, since it isn't called by device model. At the point where stubdomain is already destroyed, I think it should return NULL, but it returns hardware_domain. But it isn't that easy, because target domain don't have any reference to its stubdomain. Especially already destroyed one. I's defined as: static struct domain *get_dm_domain(struct domain *d) { return current->domain->target == d ? current->domain : hardware_domain; } Since hardware domain couldn't be destroyed(*) while the system is running, in practice this wrong return value it isn't a problem. Hardware didn't have permission over this IRQ (if stubdomain have created it), so irq_deny_access is a no-op. So, I would adjust assert in destroy_irq to allow also hardware_domain, and add a comment that get_dm_domain may return hardware_domain during domain destruction. Is that ok? (*) is this actually true? I see shutdown_domain(hardware_domain) cause Xen to shutdown. But I don't see anything like this in domain_kill/domain_destroy. Normally no other domain than dom0 is able to destroy other domains (and domain can't destroy itself), so it should be ok. But with some XSM policy, I think it could be possible to allow other domain to destroy hardware domain. In fact, just having hardware domain != dom0 is enough to violate this assumption. Am I missing anything? Full crash message: (XEN) Assertion 'd == current->domain' failed at irq.c:232 (XEN) ----[ Xen-4.12.0-rc x86_64 debug=y Not tainted ]---- (XEN) CPU: 2 (XEN) RIP: e008:[<ffff82d08028f545>] destroy_irq+0x126/0x143 (XEN) RFLAGS: 0000000000010206 CONTEXT: hypervisor (XEN) rax: ffff830422264000 rbx: 000000000000001e rcx: 0000000000000000 (XEN) rdx: ffff8304222de000 rsi: ffff830422235000 rdi: 000000000000001e (XEN) rbp: ffff83042225fcc0 rsp: ffff83042225fc90 r8: 0000000000000000 (XEN) r9: ffff830385868880 r10: 0000000000000004 r11: ffff8304222ba010 (XEN) r12: ffff830422235000 r13: 000000000000001e r14: ffff830422280000 (XEN) r15: ffff83042d8d1000 cr0: 000000008005003b cr4: 00000000000426e0 (XEN) cr3: 00000000ca49d000 cr2: ffff88011a179c00 (XEN) fsb: 0000000000000000 gsb: ffff880135640000 gss: 0000000000000000 (XEN) ds: 002b es: 002b fs: 0000 gs: 0000 ss: e010 cs: e008 (XEN) Xen code around <ffff82d08028f545> (destroy_irq+0x126/0x143): (XEN) 41 5e 41 5f 5d c3 0f 0b <0f> 0b 41 0f b7 34 24 89 c1 44 89 ea 48 8d 3d b0 (XEN) Xen stack trace from rsp=ffff83042225fc90: (XEN) ffff83042225fca0 0000000000000000 ffff83025f33a860 ffff830422235000 (XEN) ffff83025f33a860 ffff83042d8d1000 ffff83042225fd00 ffff82d08028ce1e (XEN) ffff83025f33a700 ffff83025f33a860 0000000000000000 ffff830422281e00 (XEN) 000000000000001e ffff83042d8d1000 ffff83042225fd90 ffff82d0802923e1 (XEN) ffffffffffffff26 ffffffffffffffc9 ffffffffffffffc9 ffff83042d8d1858 (XEN) 000000002d8d1000 0000000000000286 0000000000000037 ffff83025f33a860 (XEN) ffff83042d8d10f0 0000003700000001 ffff83042225fd80 0000000000000037 (XEN) ffff83042d8d1000 ffff83042d8d1b28 ffff83042d8d10f0 ffff83042d8d10cc (XEN) ffff83042225fdd0 ffff82d08029249f 0000000000000013 ffff83042d8d1000 (XEN) 00000000ffffffff ffff83042d8d1b28 ffff83042d8d1000 fffffffffffffff8 (XEN) ffff83042225fdf0 ffff82d0802819e0 ffff83042d8d1b28 ffff8303d2db1000 (XEN) ffff83042225fe30 ffff82d080207d22 000008f2621ab810 ffff830422262040 (XEN) 0000000000000000 0000000000000001 ffff83042225ffff ffff82d0805c3880 (XEN) ffff83042225fe60 ffff82d08022a658 000000000000001d ffff82d0805bb980 (XEN) ffff82d0805bb880 ffffffffffffffff ffff83042225fea0 ffff82d08023c4fa (XEN) ffff82d0805bb980 0000000000000002 ffff82d0805e9af0 ffff82d0805bb880 (XEN) ffff82d0805d4a20 ffff83042225ffff ffff83042225feb0 ffff82d08023c566 (XEN) ffff83042225fef0 ffff82d080280532 ffff830422264000 ffff830422264000 (XEN) ffff830422217000 0000000000000002 ffff830422235000 ffff8304222de000 (XEN) ffff83042225fd98 0000000000000000 0000000000000000 0000000000000000 (XEN) Xen call trace: (XEN) [<ffff82d08028f545>] destroy_irq+0x126/0x143 (XEN) [<ffff82d08028ce1e>] msi_free_irq+0x51/0x1b8 (XEN) [<ffff82d0802923e1>] unmap_domain_pirq+0x487/0x4d4 (XEN) [<ffff82d08029249f>] free_domain_pirqs+0x71/0x8f (XEN) [<ffff82d0802819e0>] arch_domain_destroy+0x41/0xa1 (XEN) [<ffff82d080207d22>] domain.c#complete_domain_destroy+0x86/0x159 (XEN) [<ffff82d08022a658>] rcupdate.c#rcu_process_callbacks+0xa5/0x1cc (XEN) [<ffff82d08023c4fa>] softirq.c#__do_softirq+0x78/0x9a (XEN) [<ffff82d08023c566>] do_softirq+0x13/0x15 (XEN) [<ffff82d080280532>] domain.c#idle_loop+0x63/0xb9 (XEN) (XEN) (XEN) **************************************** (XEN) Panic on CPU 2: (XEN) Assertion 'd == current->domain' failed at irq.c:232 (XEN) **************************************** -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing?

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.

Re: [Xen-devel] [PATCH v4.1 4/6] xen/x86: Allow stubdom access to irq created for msi.