From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: <518BF425.3070504@xenomai.org> Date: Thu, 09 May 2013 21:08:21 +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> In-Reply-To: <518BEDEC.7060302@web.de> 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: Jan Kiszka Cc: Xenomai 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() I can do that, it avoids the issue with __ipipe_uaccess_might_fault, but: - it is not the same guarantee, nothing tells you that obj is the one associated with the name you passed to xnregistry_fetch, it may have been destroyed and created in the mean time - I am not sure we do not have some other places where we call copy_from/to_user with irqs off. -- Gilles.