Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1185040 > unrolled thread
| Started by | Andy Lutomirski <luto@amacapital.net> |
|---|---|
| First post | 2015-07-15 22:50 +0200 |
| Last post | 2015-07-15 23:30 +0200 |
| Articles | 2 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [tip:x86/asm] x86/entry: Add new, comprehensible entry and exit handlers written in C Andy Lutomirski <luto@amacapital.net> - 2015-07-15 22:50 +0200
[PATCH] x86/entry: Fix _TIF_USER_RETURN_NOTIFY check in prepare_exit_to_usermode Andy Lutomirski <luto@kernel.org> - 2015-07-15 23:30 +0200
| From | Andy Lutomirski <luto@amacapital.net> |
|---|---|
| Date | 2015-07-15 22:50 +0200 |
| Subject | Re: [tip:x86/asm] x86/entry: Add new, comprehensible entry and exit handlers written in C |
| Message-ID | <pMBMS-26M-15@gated-at.bofh.it> |
On Wed, Jul 15, 2015 at 12:56 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> 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.
Crap, that's a bug. I'll send a patch.
>
> 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.
The old do_notify_resume work loop worked more or less the same way,
so we should be okay here. See below.
>
> 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?
>
Yes, but it would be a slowdown unless we converted all the various
handlers stopped clearing the bits separately (two atomics instead of
one). And to do that, we'd probably want to change all the arches,
Signal handling has all the recalc_sigpending stuff. schedule() had
better clear TIF_NEED_RESCHED. fire_user_return_notifiers is totally
absurd but it does clear the bit. uprobes clears the bit directly.
I'd be all for changing this, but coordinating with the generic code
could be annoying.
--Andy
--
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/
[toc] | [next] | [standalone]
| From | Andy Lutomirski <luto@kernel.org> |
|---|---|
| Date | 2015-07-15 23:30 +0200 |
| Subject | [PATCH] x86/entry: Fix _TIF_USER_RETURN_NOTIFY check in prepare_exit_to_usermode |
| Message-ID | <pMCpA-35F-15@gated-at.bofh.it> |
| In reply to | #1185040 |
Linus noticed that the early return check was missing
_TIF_USER_RETURN_NOTIFY. If the only work flag was
_TIF_USER_RETURN_NOTIFY, we'd skip user return notifiers. Fix it.
(This is the only missing bit.)
This fixes double faults on a KVM host. It's the same issue as last
time, except that this time it's very easy to trigger. Apparently no
one uses -next as a KVM host.
(I'm still not quite sure what it is that KVM does that blows up so
badly if we miss a user return notifier. My best guess is that KVM
lets KERNEL_GS_BASE (i.e. the user's gs base) be negative and fixes
it up in a user return notifier. If we actually end up in user mode
with a negative gs base, we blow up pretty badly.)
Fixes: c5c46f59e4e7 ("x86/entry: Add new, comprehensible entry and exit handlers written in C")
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
arch/x86/entry/common.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index febc53086a69..a3e9c7fa15d9 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -264,7 +264,8 @@ __visible void prepare_exit_to_usermode(struct pt_regs *regs)
READ_ONCE(pt_regs_to_thread_info(regs)->flags);
if (!(cached_flags & (_TIF_SIGPENDING | _TIF_NOTIFY_RESUME |
- _TIF_UPROBE | _TIF_NEED_RESCHED)))
+ _TIF_UPROBE | _TIF_NEED_RESCHED |
+ _TIF_USER_RETURN_NOTIFY)))
break;
/* We have work to do. */
--
2.4.3
--
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/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web