Re: [Xenomai-core] [Xenomai-commits] r3147 - __xn_access_ok changes

Re: [Xenomai-core] [Xenomai-commits] r3147 - __xn_access_ok changes

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 998 bytes --]

[Let's discuss this without bothering users :)]

Philippe Gerum wrote:
> Author: rpm
> Date: Sun Nov  4 18:18:39 2007
> New Revision: 3147
> 
> URL: http://svn.gna.org/viewcvs/xenomai?rev=3147&view=rev
> Log:
> Make __xn_access_ok() return false for addresses lower than the natural page size.
> 
> Modified:
>     trunk/ChangeLog
>     trunk/include/asm-x86/syscall_32.h
>     trunk/include/asm-x86/syscall_64.h

Could it be that you meant "PAGE_SIZE" instead of "PAGE_OFFSET"? Because
the current version is "slightly" broken, tagging any address in user
land as invalid.

And if this test was meant to catch NULL page accesses early, is the
intention to cope with all those current i-pipe patches that do not yet
include the discussed domain switch on non-root faults? If yes, this
test would be a workaround for legacy code and should not become default
(pure overhead for later versions).

Last question: We need this for the other archs as well, don't we?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 249 bytes --]

Re: [Xenomai-core] [Xenomai-commits] r3147 - __xn_access_ok changes

[Gilles Chanteperdrix]

Jan Kiszka wrote:
 > [Let's discuss this without bothering users :)]
 > 
 > Philippe Gerum wrote:
 > > Author: rpm
 > > Date: Sun Nov  4 18:18:39 2007
 > > New Revision: 3147
 > > 
 > > URL: http://svn.gna.org/viewcvs/xenomai?rev=3147&view=rev
 > > Log:
 > > Make __xn_access_ok() return false for addresses lower than the natural page size.
 > > 
 > > Modified:
 > >     trunk/ChangeLog
 > >     trunk/include/asm-x86/syscall_32.h
 > >     trunk/include/asm-x86/syscall_64.h
 > 
 > Could it be that you meant "PAGE_SIZE" instead of "PAGE_OFFSET"? Because
 > the current version is "slightly" broken, tagging any address in user
 > land as invalid.
 > 
 > And if this test was meant to catch NULL page accesses early, is the
 > intention to cope with all those current i-pipe patches that do not yet
 > include the discussed domain switch on non-root faults? If yes, this
 > test would be a workaround for legacy code and should not become default
 > (pure overhead for later versions).

We can reduce the overhead of the two tests by testing 
(unsigned long) (addr - PAGE_SIZE) < (PAGE_OFFSET - PAGE_SIZE)

 > 
 > Last question: We need this for the other archs as well, don't we?
 > 
 > Jan
 > 
 > _______________________________________________
 > Xenomai-core mailing list
 > Xenomai-core@domain.hid
 > https://mail.gna.org/listinfo/xenomai-core


-- 


					    Gilles Chanteperdrix.

Re: [Xenomai-core] [Xenomai-commits] r3147 - __xn_access_ok changes

[Philippe Gerum]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>  > [Let's discuss this without bothering users :)]
>  > 
>  > Philippe Gerum wrote:
>  > > Author: rpm
>  > > Date: Sun Nov  4 18:18:39 2007
>  > > New Revision: 3147
>  > > 
>  > > URL: http://svn.gna.org/viewcvs/xenomai?rev=3147&view=rev
>  > > Log:
>  > > Make __xn_access_ok() return false for addresses lower than the natural page size.
>  > > 
>  > > Modified:
>  > >     trunk/ChangeLog
>  > >     trunk/include/asm-x86/syscall_32.h
>  > >     trunk/include/asm-x86/syscall_64.h
>  > 
>  > Could it be that you meant "PAGE_SIZE" instead of "PAGE_OFFSET"? Because
>  > the current version is "slightly" broken, tagging any address in user
>  > land as invalid.
>  > 

Another example that committing last minute "fixes" before leaving for a 
trip is just as safe as checking for your parachute _after_ you jumped 
out of the plane. Fixed the trivial way for now. Sorry for this.

>  > And if this test was meant to catch NULL page accesses early, is the
>  > intention to cope with all those current i-pipe patches that do not yet
>  > include the discussed domain switch on non-root faults? If yes, this
>  > test would be a workaround for legacy code and should not become default
>  > (pure overhead for later versions).
> 
> We can reduce the overhead of the two tests by testing 
> (unsigned long) (addr - PAGE_SIZE) < (PAGE_OFFSET - PAGE_SIZE)
> 

Yep. I'd rather keep the reference to the actual task's segment limit in 
this expression though, instead of hardcoding PAGE_OFFSET.

Aside of this, we should not need to pass the current task pointer to 
each and every syscall anymore, so we may rely on the original uaccess code.

__xn_access_ok was there to allow for passing the task pointer
explicitly in order to test the address range against the task's segment
limit, at times when Adeos would switch the underlying stack to some
private, non-Linux one.

With modern I-pipe, domains don't have any private stack, but simply
reuse the current one, which means that any task running a syscall
over the Xenomai domain may always be referenced by "current", including
on archs using the stack pointer trick to determine this value.

I'll do this change early in the 2.5 series; it's too late for 2.4.

This said, we'll still need __xn_access_ok from now on, to pre-test the 
address for obvious spuriousness, like the NULL case which started this 
discussion.

>  > 
>  > Last question: We need this for the other archs as well, don't we?
>  > 

True. We also badly need to add the fixup code to the I-pipe for those 
archs.

>  > Jan
>  > 
>  > _______________________________________________
>  > Xenomai-core mailing list
>  > Xenomai-core@domain.hid
>  > https://mail.gna.org/listinfo/xenomai-core
> 
> 

Re: [Xenomai-core] [Xenomai-commits] r3147 - __xn_access_ok changes

[Jan Kiszka]

Philippe Gerum wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>  > [Let's discuss this without bothering users :)]
>>  > 
>>  > Philippe Gerum wrote:
>>  > > Author: rpm
>>  > > Date: Sun Nov  4 18:18:39 2007
>>  > > New Revision: 3147
>>  > > 
>>  > > URL: http://svn.gna.org/viewcvs/xenomai?rev=3147&view=rev
>>  > > Log:
>>  > > Make __xn_access_ok() return false for addresses lower than the natural page size.
>>  > > 
>>  > > Modified:
>>  > >     trunk/ChangeLog
>>  > >     trunk/include/asm-x86/syscall_32.h
>>  > >     trunk/include/asm-x86/syscall_64.h
>>  > 
>>  > Could it be that you meant "PAGE_SIZE" instead of "PAGE_OFFSET"? Because
>>  > the current version is "slightly" broken, tagging any address in user
>>  > land as invalid.
>>  > 
> 
> Another example that committing last minute "fixes" before leaving for a 
> trip is just as safe as checking for your parachute _after_ you jumped 
> out of the plane. Fixed the trivial way for now. Sorry for this.
> 
>>  > And if this test was meant to catch NULL page accesses early, is the
>>  > intention to cope with all those current i-pipe patches that do not yet
>>  > include the discussed domain switch on non-root faults? If yes, this
>>  > test would be a workaround for legacy code and should not become default
>>  > (pure overhead for later versions).
>>
>> We can reduce the overhead of the two tests by testing 
>> (unsigned long) (addr - PAGE_SIZE) < (PAGE_OFFSET - PAGE_SIZE)
>>
> 
> Yep. I'd rather keep the reference to the actual task's segment limit in 
> this expression though, instead of hardcoding PAGE_OFFSET.
> 
> Aside of this, we should not need to pass the current task pointer to 
> each and every syscall anymore, so we may rely on the original uaccess code.
> 
> __xn_access_ok was there to allow for passing the task pointer
> explicitly in order to test the address range against the task's segment
> limit, at times when Adeos would switch the underlying stack to some
> private, non-Linux one.
> 
> With modern I-pipe, domains don't have any private stack, but simply
> reuse the current one, which means that any task running a syscall
> over the Xenomai domain may always be referenced by "current", including
> on archs using the stack pointer trick to determine this value.
> 
> I'll do this change early in the 2.5 series; it's too late for 2.4.
> 
> This said, we'll still need __xn_access_ok from now on, to pre-test the 
> address for obvious spuriousness, like the NULL case which started this 
> discussion.

...which still doesn't answer my original question: Is the current
(virtually fixed - please fix in real-life soon!) version a legacy
wrapper for you? Because I still don't see the need for it with, e.g.,
my latest patch for i386-ipipe.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [Xenomai-commits] r3147 - __xn_access_ok changes

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2969 bytes --]

Jan Kiszka wrote:
> Philippe Gerum wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>  > [Let's discuss this without bothering users :)]
>>>  > 
>>>  > Philippe Gerum wrote:
>>>  > > Author: rpm
>>>  > > Date: Sun Nov  4 18:18:39 2007
>>>  > > New Revision: 3147
>>>  > > 
>>>  > > URL: http://svn.gna.org/viewcvs/xenomai?rev=3147&view=rev
>>>  > > Log:
>>>  > > Make __xn_access_ok() return false for addresses lower than the natural page size.
>>>  > > 
>>>  > > Modified:
>>>  > >     trunk/ChangeLog
>>>  > >     trunk/include/asm-x86/syscall_32.h
>>>  > >     trunk/include/asm-x86/syscall_64.h
>>>  > 
>>>  > Could it be that you meant "PAGE_SIZE" instead of "PAGE_OFFSET"? Because
>>>  > the current version is "slightly" broken, tagging any address in user
>>>  > land as invalid.
>>>  > 
>> Another example that committing last minute "fixes" before leaving for a 
>> trip is just as safe as checking for your parachute _after_ you jumped 
>> out of the plane. Fixed the trivial way for now. Sorry for this.
>>
>>>  > And if this test was meant to catch NULL page accesses early, is the
>>>  > intention to cope with all those current i-pipe patches that do not yet
>>>  > include the discussed domain switch on non-root faults? If yes, this
>>>  > test would be a workaround for legacy code and should not become default
>>>  > (pure overhead for later versions).
>>>
>>> We can reduce the overhead of the two tests by testing 
>>> (unsigned long) (addr - PAGE_SIZE) < (PAGE_OFFSET - PAGE_SIZE)
>>>
>> Yep. I'd rather keep the reference to the actual task's segment limit in 
>> this expression though, instead of hardcoding PAGE_OFFSET.
>>
>> Aside of this, we should not need to pass the current task pointer to 
>> each and every syscall anymore, so we may rely on the original uaccess code.
>>
>> __xn_access_ok was there to allow for passing the task pointer
>> explicitly in order to test the address range against the task's segment
>> limit, at times when Adeos would switch the underlying stack to some
>> private, non-Linux one.
>>
>> With modern I-pipe, domains don't have any private stack, but simply
>> reuse the current one, which means that any task running a syscall
>> over the Xenomai domain may always be referenced by "current", including
>> on archs using the stack pointer trick to determine this value.
>>
>> I'll do this change early in the 2.5 series; it's too late for 2.4.
>>
>> This said, we'll still need __xn_access_ok from now on, to pre-test the 
>> address for obvious spuriousness, like the NULL case which started this 
>> discussion.
> 
> ...which still doesn't answer my original question: Is the current
> (virtually fixed - please fix in real-life soon!) version a legacy

Saw it too late: already done in #3150.

> wrapper for you? Because I still don't see the need for it with, e.g.,
> my latest patch for i386-ipipe.
> 
> Jan
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 249 bytes --]

Re: [Xenomai-core] [Xenomai-commits] r3147 - __xn_access_ok changes

[Philippe Gerum]

Jan Kiszka wrote:
> Philippe Gerum wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>  > [Let's discuss this without bothering users :)]
>>>  > 
>>>  > Philippe Gerum wrote:
>>>  > > Author: rpm
>>>  > > Date: Sun Nov  4 18:18:39 2007
>>>  > > New Revision: 3147
>>>  > > 
>>>  > > URL: http://svn.gna.org/viewcvs/xenomai?rev=3147&view=rev
>>>  > > Log:
>>>  > > Make __xn_access_ok() return false for addresses lower than the natural page size.
>>>  > > 
>>>  > > Modified:
>>>  > >     trunk/ChangeLog
>>>  > >     trunk/include/asm-x86/syscall_32.h
>>>  > >     trunk/include/asm-x86/syscall_64.h
>>>  > 
>>>  > Could it be that you meant "PAGE_SIZE" instead of "PAGE_OFFSET"? Because
>>>  > the current version is "slightly" broken, tagging any address in user
>>>  > land as invalid.
>>>  > 
>> Another example that committing last minute "fixes" before leaving for a 
>> trip is just as safe as checking for your parachute _after_ you jumped 
>> out of the plane. Fixed the trivial way for now. Sorry for this.
>>
>>>  > And if this test was meant to catch NULL page accesses early, is the
>>>  > intention to cope with all those current i-pipe patches that do not yet
>>>  > include the discussed domain switch on non-root faults? If yes, this
>>>  > test would be a workaround for legacy code and should not become default
>>>  > (pure overhead for later versions).
>>>
>>> We can reduce the overhead of the two tests by testing 
>>> (unsigned long) (addr - PAGE_SIZE) < (PAGE_OFFSET - PAGE_SIZE)
>>>
>> Yep. I'd rather keep the reference to the actual task's segment limit in 
>> this expression though, instead of hardcoding PAGE_OFFSET.
>>
>> Aside of this, we should not need to pass the current task pointer to 
>> each and every syscall anymore, so we may rely on the original uaccess code.
>>
>> __xn_access_ok was there to allow for passing the task pointer
>> explicitly in order to test the address range against the task's segment
>> limit, at times when Adeos would switch the underlying stack to some
>> private, non-Linux one.
>>
>> With modern I-pipe, domains don't have any private stack, but simply
>> reuse the current one, which means that any task running a syscall
>> over the Xenomai domain may always be referenced by "current", including
>> on archs using the stack pointer trick to determine this value.
>>
>> I'll do this change early in the 2.5 series; it's too late for 2.4.
>>
>> This said, we'll still need __xn_access_ok from now on, to pre-test the 
>> address for obvious spuriousness, like the NULL case which started this 
>> discussion.
> 
> ...which still doesn't answer my original question: Is the current
> (virtually fixed - please fix in real-life soon!) version a legacy
> wrapper for you? Because I still don't see the need for it with, e.g.,
> my latest patch for i386-ipipe.

I usually refrain from rehashing the obvious, but here it comes anyway: 
yes, it's a work around, because we have more than 340 older I-pipe 
patches out there for a number of archs, and leaving the possibility for 
lockups with those just due to some uncaught NULL pointer would be bad 
policy. And yes, it adds a few cycles more to the hot path for that 
reason, so we may want to make this conditional -- albeit the fixes are 
functionally quite different, and getting -EFAULT at application level 
may give a better debugging help than relying on CONFIG_IPIPE_DEBUG and 
looking at the kernel log periodically, at least for most common error 
cases like NULL pointers.

As a matter of fact, there is not a single I-pipe patch officially 
released which implements the fixup I've been talking about yesterday 
yet, therefore, no time is lost for making the uaccess trick conditional.

Re: [Xenomai-core] [Xenomai-commits] r3147 - __xn_access_ok changes

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 4747 bytes --]

Philippe Gerum wrote:
> Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>  > [Let's discuss this without bothering users :)]
>>>>  > 
>>>>  > Philippe Gerum wrote:
>>>>  > > Author: rpm
>>>>  > > Date: Sun Nov  4 18:18:39 2007
>>>>  > > New Revision: 3147
>>>>  > > 
>>>>  > > URL: http://svn.gna.org/viewcvs/xenomai?rev=3147&view=rev
>>>>  > > Log:
>>>>  > > Make __xn_access_ok() return false for addresses lower than the natural page size.
>>>>  > > 
>>>>  > > Modified:
>>>>  > >     trunk/ChangeLog
>>>>  > >     trunk/include/asm-x86/syscall_32.h
>>>>  > >     trunk/include/asm-x86/syscall_64.h
>>>>  > 
>>>>  > Could it be that you meant "PAGE_SIZE" instead of "PAGE_OFFSET"? Because
>>>>  > the current version is "slightly" broken, tagging any address in user
>>>>  > land as invalid.
>>>>  > 
>>> Another example that committing last minute "fixes" before leaving for a 
>>> trip is just as safe as checking for your parachute _after_ you jumped 
>>> out of the plane. Fixed the trivial way for now. Sorry for this.
>>>
>>>>  > And if this test was meant to catch NULL page accesses early, is the
>>>>  > intention to cope with all those current i-pipe patches that do not yet
>>>>  > include the discussed domain switch on non-root faults? If yes, this
>>>>  > test would be a workaround for legacy code and should not become default
>>>>  > (pure overhead for later versions).
>>>>
>>>> We can reduce the overhead of the two tests by testing 
>>>> (unsigned long) (addr - PAGE_SIZE) < (PAGE_OFFSET - PAGE_SIZE)
>>>>
>>> Yep. I'd rather keep the reference to the actual task's segment limit in 
>>> this expression though, instead of hardcoding PAGE_OFFSET.
>>>
>>> Aside of this, we should not need to pass the current task pointer to 
>>> each and every syscall anymore, so we may rely on the original uaccess code.
>>>
>>> __xn_access_ok was there to allow for passing the task pointer
>>> explicitly in order to test the address range against the task's segment
>>> limit, at times when Adeos would switch the underlying stack to some
>>> private, non-Linux one.
>>>
>>> With modern I-pipe, domains don't have any private stack, but simply
>>> reuse the current one, which means that any task running a syscall
>>> over the Xenomai domain may always be referenced by "current", including
>>> on archs using the stack pointer trick to determine this value.
>>>
>>> I'll do this change early in the 2.5 series; it's too late for 2.4.
>>>
>>> This said, we'll still need __xn_access_ok from now on, to pre-test the 
>>> address for obvious spuriousness, like the NULL case which started this 
>>> discussion.
>> ...which still doesn't answer my original question: Is the current
>> (virtually fixed - please fix in real-life soon!) version a legacy
>> wrapper for you? Because I still don't see the need for it with, e.g.,
>> my latest patch for i386-ipipe.
> 
> I usually refrain from rehashing the obvious, but here it comes anyway: 

The reason why I'm picking on this is because this "obvious" detail at
least belongs into the code in form of a comment (instead of the
currently documented even more obvious properties).

> yes, it's a work around, because we have more than 340 older I-pipe 
> patches out there for a number of archs, and leaving the possibility for 
> lockups with those just due to some uncaught NULL pointer would be bad 
> policy. And yes, it adds a few cycles more to the hot path for that 
> reason, so we may want to make this conditional -- albeit the fixes are 
> functionally quite different, and getting -EFAULT at application level 
> may give a better debugging help than relying on CONFIG_IPIPE_DEBUG and 
> looking at the kernel log periodically, at least for most common error 
> cases like NULL pointers.

Again, this detection is normally not the job of __xn_access_ok, and
people reading it can expect an explanation of this exceptional
modification (/me avoiding a stronger term here). Moreover, the reason
that certain user bugs currently only show up in kernel log is due to at
least the native skin consequently ignoring the result of
xn_copy_*_user. That's a different story, likely something that needs to
be fixed on a series of rainy days (along with RTnet which is even
worse, BTW)...

> 
> As a matter of fact, there is not a single I-pipe patch officially 
> released which implements the fixup I've been talking about yesterday 
> yet, therefore, no time is lost for making the uaccess trick conditional.

I would expect 2.4 final including fixed-up I-pipe patches for at least
x86, so this should quickly become relevant.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 249 bytes --]