MIME-Version: 1.0
In-Reply-To: <20150714230727.GD29441@lerouge>
References: <c57c8b87661a4152801d7d3786eac2d1a2f209dd.1435952415.git.luto@kernel.org>
	<tip-c5c46f59e4e7c1ab244b8d38f2b61d317df90bba@git.kernel.org>
	<20150714230727.GD29441@lerouge>
Date: Wed, 15 Jul 2015 12:56:35 -0700
Message-ID: <CA+55aFy7e=h0bUDL=TV3QRcxicLfgg5vJX9jf+wwSmPg+jYN4w@mail.gmail.com>
Subject: Re: [tip:x86/asm] x86/entry: Add new, comprehensible entry and exit
 handlers written in C
From: Linus Torvalds <torvalds@linux-foundation.org>
To: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Kees Cook <keescook@chromium.org>, Peter Zijlstra <peterz@infradead.org>,
        Denys Vlasenko <vda.linux@googlemail.com>,
        Ingo Molnar <mingo@kernel.org>, Brian Gerst <brgerst@gmail.com>,
        Andy Lutomirski <luto@amacapital.net>, Borislav Petkov <bp@alien8.de>,
        Andrew Lutomirski <luto@kernel.org>, Oleg Nesterov <oleg@redhat.com>,
        Peter Anvin <hpa@zytor.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Denys Vlasenko <dvlasenk@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>, Rik van Riel <riel@redhat.com>,
        "linux-tip-commits@vger.kernel.org" 
	<linux-tip-commits@vger.kernel.org>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2629
Lines: 72

On Tue, Jul 14, 2015 at 4:07 PM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> On Tue, Jul 07, 2015 at 03:51:48AM -0700, tip-bot for Andy Lutomirski wrote:
>> +     while (true) {
>> +             u32 cached_flags =
>> +                     READ_ONCE(pt_regs_to_thread_info(regs)->flags);
>> +
>> +             if (!(cached_flags & (_TIF_SIGPENDING | _TIF_NOTIFY_RESUME |
>> +                                   _TIF_UPROBE | _TIF_NEED_RESCHED)))
>> +                     break;
>> +
>> +             /* We have work to do. */
>> +             local_irq_enable();
>> +
>> +             if (cached_flags & _TIF_NEED_RESCHED)
>> +                     schedule();
>> +
>> +             if (cached_flags & _TIF_UPROBE)
>> +                     uprobe_notify_resume(regs);
>> +
>> +             /* deal with pending signal delivery */
>> +             if (cached_flags & _TIF_SIGPENDING)
>> +                     do_signal(regs);
>> +
>> +             if (cached_flags & _TIF_NOTIFY_RESUME) {
>> +                     clear_thread_flag(TIF_NOTIFY_RESUME);
>> +                     tracehook_notify_resume(regs);
>> +             }
>> +
>> +             if (cached_flags & _TIF_USER_RETURN_NOTIFY)
>> +                     fire_user_return_notifiers();
>> +
>> +             /* Disable IRQs and retry */
>> +             local_irq_disable();
>> +     }
>
> I dreamed so many times about this loop in C!

So this made me look at it again, and now I'm worried.

There's that "early break", but it doesn't check
_TIF_USER_RETURN_NOTIFY. So if *only* USER_RETURN_NOTIFY is set, we're
screwed.

It migth be that that doesn't happen for some reason, but I'm not
seeing what that reason would be.

The other thing that worries me is that this depends on all the
handler routines to clear the flags (except for
tracehook_notify_resume()). Which they hopefully do. But that means
that just looking at this locally, it's not at all obvious that it
works right.

So wouldn't it be much nicer to do:

        u32 cached_flags = READ_ONCE(pt_regs_to_thread_info(regs)->flags);

        cached_flags &= _TIF_SIGPENDING | _TIF_NOTIFY_RESUME |
_TIF_USER_RETURN_NOTIFY | _TIF_UPROBE | _TIF_NEED_RESCHED;

        if (!cached_flags)
                break;

        atomic_clear_mask(cached_flags, &pt_regs_to_thread_info(regs)->flags);

and then have those bit tests after that?

                   Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/