Re: [Xen-devel] [PATCH v5 4/9] x86/traps: Enable all exception handler callbacks early

[Andy Lutomirski <luto@xxxxxxxxxxxxxx>]

On Sun, Apr 3, 2016 at 1:07 AM, Borislav Petkov <bp@xxxxxxxxx> wrote:
> On Sat, Apr 02, 2016 at 10:52:48PM +0200, Borislav Petkov wrote:
>> On Sat, Apr 02, 2016 at 01:16:07PM -0700, Andy Lutomirski wrote:
>> > I have no idea why it was explicitly unsupported, but I'm guessing it
>> > was just to avoid duplicating the code.  Early "ext" uaccess failures
>> > are certainly not going to work, but I don't think this is a problem
>> > -- there's no userspace before trap_init runs, so how exactly is an
>> > "ext" uaccess going to happen in the first place?
>> >
>> > In any event, if it did happen in older kernels, it would have
>> > immediately panicked due to that code.  At least with my code it just
>> > might manage to EFAULT correctly.
>>
>> Yeah, I was wondering what that early thing meant.
>>
>> Linus or tip guys probably remember what this whole deal with early
>> uaccess was about. I'll try to do some git archeology tomorrow.
>
> Yep, just as I suspected:
>
> 6a1ea279c210 ("x86, extable: Add early_fixup_exception()")
>
> Apparently, thread_info might not have been setup yet. I'm guessing the
> intention behind this no-uaccess-fixup-early is to not even attempt any
> fixup due to stuff *probably* not initialized yet and so the safer thing
> would be to panic instead.
>
> I'm wondering whether making it try to EFAULT correctly is the right
> thing to do... We're certainly more conservative if we panic and not
> allow some silently failed attempt at recovery which looks successful,
> to continue.
>
> hpa, thoughts?

I don't think this matters much.  There aren't many users of this
mechanism in the tree:

./arch/x86/kernel/signal.c:    get_user_try {
./arch/x86/kernel/signal.c:    put_user_try {
./arch/x86/kernel/signal.c:    put_user_try {
./arch/x86/kernel/signal.c:    put_user_try {
./arch/x86/kernel/signal.c:    put_user_try {
./arch/x86/kernel/signal_compat.c:    put_user_try {
./arch/x86/kernel/signal_compat.c:    get_user_try {
./arch/x86/kernel/vm86_32.c:    put_user_try {
./arch/x86/kernel/vm86_32.c:    get_user_try {
./arch/x86/ia32/ia32_signal.c:    get_user_try {
./arch/x86/ia32/ia32_signal.c:    put_user_try {
./arch/x86/ia32/ia32_signal.c:    put_user_try {
./arch/x86/ia32/ia32_signal.c:    put_user_try {
./arch/x86/include/asm/uaccess.h: * {get|put}_user_try and catch
./arch/x86/include/asm/uaccess.h: * get_user_try {
./arch/x86/include/asm/uaccess.h:#define get_user_try        uaccess_try
./arch/x86/include/asm/uaccess.h:#define put_user_try        uaccess_try

I don't see how we could get to that code in the first place without
current_thread_info() working.

If we can ever convince gcc to do jump labels properly for uaccess, it
would probably be better to just delete all that code.

--Andy

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel