[Xenomai] __get_user/__put_user

[Xenomai] __get_user/__put_user

[Gilles Chanteperdrix]

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() ?

Thanks in advance.

-- 
                                                                Gilles.

Re: [Xenomai] __get_user/__put_user

[Jan Kiszka]

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.

Jan


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20130316/3caec3c4/attachment.pgp>

Re: [Xenomai] __get_user/__put_user

[Gilles Chanteperdrix]

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...

-- 
                                                                Gilles.

Re: [Xenomai] __get_user/__put_user

[Jan Kiszka]

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.

Jan


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20130316/c8d623b0/attachment.pgp>

Re: [Xenomai] __get_user/__put_user

[Gilles Chanteperdrix]

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.

-- 
                                                                Gilles.

Re: [Xenomai] __get_user/__put_user

[Gilles Chanteperdrix]

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. So, I
believe we can simplify __ipipe_uaccess_might_fault to avoid the check
for irqs off.

-- 
                                                                Gilles.

Re: [Xenomai] __get_user/__put_user

[Jan Kiszka]

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...

Jan


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20130509/63c15c40/attachment.pgp>

Re: [Xenomai] __get_user/__put_user

[Gilles Chanteperdrix]

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.

-- 
                                                                Gilles.

Re: [Xenomai] __get_user/__put_user

[Jan Kiszka]

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.

Jan


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20130509/0d5976b3/attachment.pgp>

Re: [Xenomai] __get_user/__put_user

[Gilles Chanteperdrix]

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.

-- 
                                                                Gilles.

Re: [Xenomai] __get_user/__put_user

[Jan Kiszka]

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. After returning from the trap handler, the Linux
kernel handles the fault as normal, including the fix-up.

Jan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20130509/8f1152ff/attachment.pgp>

Re: [Xenomai] __get_user/__put_user

[Gilles Chanteperdrix]

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.

-- 
                                                                Gilles.

Re: [Xenomai] __get_user/__put_user

[Jan Kiszka]

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.

Jan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20130509/20f67881/attachment.pgp>

Re: [Xenomai] __get_user/__put_user

[Gilles Chanteperdrix]

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.

-- 
                                                                Gilles.

Re: [Xenomai] __get_user/__put_user

[Jan Kiszka]

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()

Jan


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20130509/8e7e7936/attachment.pgp>

Re: [Xenomai] __get_user/__put_user

[Gilles Chanteperdrix]

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.

Re: [Xenomai] __get_user/__put_user

[Jan Kiszka]

On 2013-05-09 21:08, Gilles Chanteperdrix wrote:
>> 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

See my other reply: user mistake, it should not matter.

> - I am not sure we do not have some other places where we call
> copy_from/to_user with irqs off.

Might be the case, but I think so far we should have tried to avoid
faulting under the nklock as we weren't considering this safe enough or
at least good for the worst case latency.

Jan


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20130509/5563b2b2/attachment.pgp>

Re: [Xenomai] __get_user/__put_user

[Gilles Chanteperdrix]

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.


-- 
                                                                Gilles.

Re: [Xenomai] __get_user/__put_user

[Jan Kiszka]

On 2013-05-09 21:16, Gilles Chanteperdrix wrote:
>>>> 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.
> 

Only during heap and queue binding. All other native skin syscall
entries fetch from the registry without holding nklock. I suppose the
philosophy is that, if user space generates a race around picking up an
object and destroying / recreating a different one under the same name,
it will get what it deserves.

Jan


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20130509/3834e7d7/attachment.pgp>

Re: [Xenomai] __get_user/__put_user

[Philippe Gerum]

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.

-- 
Philippe.

Re: [Xenomai] __get_user/__put_user

[Jan Kiszka]

On 2013-05-09 21:20, 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.

We would have to re-revalidate the handle then - or leave
__rt_bind_helper with nklock held so that the caller can follow up with
more accesses to the object before releasing the lock again.

It all boils down if we want to relax the constraints for using
copy_to/from_user and allow faults inside IRQ-off sections.

Jan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20130509/b48d3aff/attachment.pgp>

Re: [Xenomai] __get_user/__put_user

[Gilles Chanteperdrix]

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.

Re: [Xenomai] __get_user/__put_user

[Jan Kiszka]

On 2013-05-10 15:16, Gilles Chanteperdrix wrote:
> 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;
>  }
>  
>  /*
> 

That or returning from __rt_bind_helper with nklock still held. In that
case you don't need to revalidate at the caller site as you don't drop
the lock before using the object in __rt_queue/heap_bind. But as these
are not hotpaths, the above approach is probably cleaner.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux