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
> ...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.
Xenomai-core mailing list