From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: <518CF311.60805@xenomai.org> Date: Fri, 10 May 2013 15:16:01 +0200 From: Gilles Chanteperdrix MIME-Version: 1.0 References: <5144D3E9.2050205@xenomai.org> <5144D524.4000302@web.de> <5144D5AB.3080004@xenomai.org> <5144D5E3.9050601@web.de> <5144DB0F.9050901@xenomai.org> <518BDD9D.10804@xenomai.org> <518BE3C5.4090203@web.de> <518BE49D.2010404@xenomai.org> <518BE4CE.6020107@web.de> <518BE4FC.6060507@xenomai.org> <518BE6D2.4090304@web.de> <518BEAA2.1060703@xenomai.org> <518BEC1B.7080000@web.de> <518BECA5.3020602@xenomai.org> <518BEDEC.7060302@web.de> <518BF5F7.5070205@xenomai.org> <518BF710.9030500@xenomai.org> In-Reply-To: <518BF710.9030500@xenomai.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Xenomai] __get_user/__put_user List-Id: Discussions about the Xenomai project List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Philippe Gerum Cc: Jan Kiszka , Xenomai On 05/09/2013 09:20 PM, Philippe Gerum wrote: > On 05/09/2013 09:16 PM, Gilles Chanteperdrix wrote: >> On 05/09/2013 08:41 PM, Jan Kiszka wrote: >> >>> On 2013-05-09 20:36, Gilles Chanteperdrix wrote: >>>> On 05/09/2013 08:34 PM, Jan Kiszka wrote: >>>> >>>>> On 2013-05-09 20:27, Gilles Chanteperdrix wrote: >>>>>> On 05/09/2013 08:11 PM, Jan Kiszka wrote: >>>>>> >>>>>>> On 2013-05-09 20:03, Gilles Chanteperdrix wrote: >>>>>>>> On 05/09/2013 08:02 PM, Jan Kiszka wrote: >>>>>>>> >>>>>>>>> On 2013-05-09 20:02, Gilles Chanteperdrix wrote: >>>>>>>>>> On 05/09/2013 07:58 PM, Jan Kiszka wrote: >>>>>>>>>> >>>>>>>>>>> On 2013-05-09 19:32, Gilles Chanteperdrix wrote: >>>>>>>>>>>> On 03/16/2013 09:50 PM, Gilles Chanteperdrix wrote: >>>>>>>>>>>> >>>>>>>>>>>>> On 03/16/2013 09:28 PM, Jan Kiszka wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> On 2013-03-16 21:27, Gilles Chanteperdrix wrote: >>>>>>>>>>>>>>> On 03/16/2013 09:25 PM, Jan Kiszka wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> On 2013-03-16 21:19, Gilles Chanteperdrix wrote: >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> xenomai asm-generic/syscall.h defines __xn_put_user/__xn_get_user as >>>>>>>>>>>>>>>>> aliases for __put_user/__get_user, which implementation, for the ARM >>>>>>>>>>>>>>>>> architecture calls might_fault() which triggers an ipipe_root_only() check. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> So, the question is, what is the best way of avoiding this issue? Remove >>>>>>>>>>>>>>>>> the call to might_fault() when compiling with CONFIG_IPIPE enabled? >>>>>>>>>>>>>>>>> Define __ipipe_safe_put_user which avoids the call to might_fault() ? >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Adjust might_fault() in a way that it only considers atomic non-root >>>>>>>>>>>>>>>> contexts as problematic. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> atomic meaning with hardware irqs off? or within an irq handler? This >>>>>>>>>>>>>>> requires xenomai... >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>>> Hard irqs off || head domain stalled, I would say. >>>>>>>>>>>>> >>>>>>>>>>>>> ok, done, thanks. >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Actually, it does not work, the WARN_ON triggers in __rt_heap_bind for >>>>>>>>>>>> instance, which does some copies from user with hard irqs off. >>>>>>>>>>> >>>>>>>>>>> Hmm, isn't that a real bug? The nucleus should then try to migrate to >>>>>>>>>>> Linux while the nklock is held - ouch... >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> In theory, the fault should be fixed up without requiring a migration. I >>>>>>>>>> will test this. >>>>>>>>> >>>>>>>>> In theory, user space could also provide an invalid pointer. >>>>>>>> >>>>>>>> >>>>>>>> Yes, this is what I am going to test, but an invalid pointer cause the >>>>>>>> fault to be fixed up so that the copy_from_user return -EFAULT. >>>>>>>> >>>>>>> >>>>>>> I don't think we fix-up over the head domain. Rather, the nucleus >>>>>>> receives the information that a fault has happened over the head domain >>>>>>> and migrates to root. >>>>>> >>>>>> >>>>>> It depends on where the notification happens in the I-pipe patch. You >>>>>> could allow the kernel to fix-up the fault without notifying the >>>>>> nucleus. I thought I did that in the past, but apparently not in the >>>>>> latest patches as a segfault in rt_heap_bind causes a switch to >>>>>> secondary mode, but the nklock is automatically released as part of the >>>>>> migration to secondary mode. >>>>>> >>>>>> I am not sure the fix-up code is completely unsafe to be called over non >>>>>> root domain, as it only uses preempt_disable, preempt_enable, >>>>>> list_for_each_entry_rcu and only if the exception is found in a kernel >>>>>> module. >>>>>> >>>>> >>>>> Maybe. >>>>> >>>>> But why do we need to hold the nklock across the __rt_bind_helper in >>>>> __rt_queue_bind at all? Other binding services apparently don't have >>>>> this need. >>>> >>>> >>>> I suppose because we do not want the heap vanishing under our feet while >>>> we access its contents with the xnheap_* accessors. >>> >>> Right, but the general pattern for this is >>> >>> obj = xnregistry_fetch(handle) >>> get_nklock() >>> if (valid(obj)) >>> do_something_with(obj) >>> put_nklock() >> >> >> Seems rather to be get_nklock before xnregistry_fetch. >> >> > > Yes. The other pattern is only used when the fetched handle shall be > revalidated before actual use. __rt_bind_helper does not revalidate, so > the handle is fetched under lock. > I am not sure I understand what you mean, since __rt_bind_helper seems to revalidate. Is this patch what you have in mind? diff --git a/ksrc/skins/native/syscall.c b/ksrc/skins/native/syscall.c index a88a517..b9862e7 100644 --- a/ksrc/skins/native/syscall.c +++ b/ksrc/skins/native/syscall.c @@ -2098,15 +2098,19 @@ static int __rt_queue_bind(struct pt_regs *regs) int err; spl_t s; - xnlock_get_irqsave(&nklock, s); - err = __rt_bind_helper(p, regs, &ph.opaque, XENO_QUEUE_MAGIC, (void **)&q, 0); if (err) - goto unlock_and_exit; + return err; + + xnlock_get_irqsave(&nklock, s); + if (xeno_test_magic(q, XENO_QUEUE_MAGIC) == 0) { + xnlock_put_irqrestore(&nklock, s); + return -EACCES; + } ph.opaque2 = &q->bufpool; ph.mapsize = xnheap_extentsize(&q->bufpool); ph.area = xnheap_base_memory(&q->bufpool); @@ -2123,12 +2127,6 @@ static int __rt_queue_bind(struct pt_regs *regs) xnshadow_relax(0, 0); return 0; - - unlock_and_exit: - - xnlock_put_irqrestore(&nklock, s); - - return err; } /* @@ -2622,15 +2620,19 @@ static int __rt_heap_bind(struct pt_regs *regs) int err; spl_t s; - xnlock_get_irqsave(&nklock, s); - err = __rt_bind_helper(p, regs, &ph.opaque, XENO_HEAP_MAGIC, (void **)&heap, 0); if (err) - goto unlock_and_exit; + return err; + + xnlock_get_irqsave(&nklock, s); + if (xeno_test_magic(heap, XENO_HEAP_MAGIC) == 0) { + xnlock_put_irqrestore(&nklock, s); + return -EACCES; + } ph.opaque2 = &heap->heap_base; ph.mapsize = xnheap_extentsize(&heap->heap_base); ph.area = xnheap_base_memory(&heap->heap_base); @@ -2648,12 +2650,6 @@ static int __rt_heap_bind(struct pt_regs *regs) xnshadow_relax(0, 0); return 0; - - unlock_and_exit: - - xnlock_put_irqrestore(&nklock, s); - - return err; } /* -- Gilles.