From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753802AbbGOUrb (ORCPT ); Wed, 15 Jul 2015 16:47:31 -0400 Received: from mail-la0-f53.google.com ([209.85.215.53]:34174 "EHLO mail-la0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753442AbbGOUrT (ORCPT ); Wed, 15 Jul 2015 16:47:19 -0400 MIME-Version: 1.0 In-Reply-To: References: <20150714230727.GD29441@lerouge> From: Andy Lutomirski Date: Wed, 15 Jul 2015 13:46:58 -0700 Message-ID: Subject: Re: [tip:x86/asm] x86/entry: Add new, comprehensible entry and exit handlers written in C To: Linus Torvalds Cc: Frederic Weisbecker , Kees Cook , Peter Zijlstra , Denys Vlasenko , Ingo Molnar , Brian Gerst , Borislav Petkov , Andrew Lutomirski , Oleg Nesterov , Peter Anvin , Linux Kernel Mailing List , Denys Vlasenko , Thomas Gleixner , Rik van Riel , "linux-tip-commits@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jul 15, 2015 at 12:56 PM, Linus Torvalds wrote: > On Tue, Jul 14, 2015 at 4:07 PM, Frederic Weisbecker 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