From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 260F4C433FE for ; Mon, 14 Nov 2022 15:31:32 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236838AbiKNPb3 (ORCPT ); Mon, 14 Nov 2022 10:31:29 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53954 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236774AbiKNPbT (ORCPT ); Mon, 14 Nov 2022 10:31:19 -0500 Received: from esa.microchip.iphmx.com (esa.microchip.iphmx.com [68.232.154.123]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 96DCF2A429 for ; Mon, 14 Nov 2022 07:31:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=microchip.com; i=@microchip.com; q=dns/txt; s=mchp; t=1668439876; x=1699975876; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=+z7vhMLu3yh8zpZfl0R8/tbQ1mKg/DE0MiKRhJ26irU=; b=ypCOojXZNdgW9BEE3+RGlymS8Myl4IfgS4M0qtpqrAZ6c7v8iDoBBilK Cyxx8Zv8ZGl3heg0Fmm0wXuxN0VhY6la4chv3vS1kOy/7Hu2cPiEQ4QzT ktbfK+3BeyvEV8yTwyAgzJIFS2Qm1EbG+u9dwBrPt+D6ZBbZYAbdaOsgK RuYJBUA4uxaJZYn/fsW7P8s0uK98yifzIZA2bGfjfU5Hd/J68j1+XkTVK R02KM1Ujy9YRBhycK7GZhm8xx9bPm8iPQqIspVGE0WoVaI0szMjO6jjgc 0Z7GerLZ6iIJ1Kpod0+zJAzpnr+/tjHhDXq5Ua/IEGwC3gbGfVvfrnrY2 Q==; X-IronPort-AV: E=Sophos;i="5.96,164,1665471600"; d="scan'208";a="123323299" Received: from unknown (HELO email.microchip.com) ([170.129.1.10]) by esa6.microchip.iphmx.com with ESMTP/TLS/AES256-SHA256; 14 Nov 2022 08:31:15 -0700 Received: from chn-vm-ex04.mchp-main.com (10.10.85.152) by chn-vm-ex02.mchp-main.com (10.10.85.144) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.12; Mon, 14 Nov 2022 08:31:12 -0700 Received: from wendy (10.10.115.15) by chn-vm-ex04.mchp-main.com (10.10.85.152) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.12 via Frontend Transport; Mon, 14 Nov 2022 08:31:11 -0700 Date: Mon, 14 Nov 2022 15:30:54 +0000 From: Conor Dooley To: Andrew Bresticker CC: Guo Ren , Palmer Dabbelt , Paul Walmsley , Albert Ou , Atish Patra , , Subject: Re: [PATCH] RISC-V: Fix unannoted hardirqs-on in return to userspace slow-path Message-ID: References: <20221111223108.1976562-1-abrestic@rivosinc.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Nov 14, 2022 at 10:20:30AM -0500, Andrew Bresticker wrote: > On Mon, Nov 14, 2022 at 9:43 AM Guo Ren wrote: > > > > On Sat, Nov 12, 2022 at 6:31 AM Andrew Bresticker wrote: > > > > > > The return to userspace path in entry.S may enable interrupts without the > > > corresponding lockdep annotation, producing a splat[0] when DEBUG_LOCKDEP > > > is enabled. Simply calling __trace_hardirqs_on() here gets a bit messy > > > due to the use of RA to point back to ret_from_exception, so just move > > > the whole slow-path loop into C. It's more readable and it lets us use > > > local_irq_{enable,disable}(), avoiding the need for manual annotations > > > altogether. > > Could generic_entry solve your problem? please try: > > https://lore.kernel.org/linux-riscv/20221103075047.1634923-1-guoren@kernel.org/ > > Indeed it does, as I mentioned below, however it wasn't clear to me > how close your series was to landing so I'v emailed this small fix for > the existing bug in case your series does not make it into 6.2 The backportablilty of this 26+, 26- change has to be considered too. The generic entry patchset has 20x the changes and would not ordinarily be stable material. How much of that series would be required to solve the problem? > > > > > > > > [0]: > > > ------------[ cut here ]------------ > > > DEBUG_LOCKS_WARN_ON(!lockdep_hardirqs_enabled()) > > > WARNING: CPU: 2 PID: 1 at kernel/locking/lockdep.c:5512 check_flags+0x10a/0x1e0 > > > Modules linked in: > > > CPU: 2 PID: 1 Comm: init Not tainted 6.1.0-rc4-00160-gb56b6e2b4f31 #53 > > > Hardware name: riscv-virtio,qemu (DT) > > > epc : check_flags+0x10a/0x1e0 > > > ra : check_flags+0x10a/0x1e0 > > > > > > status: 0000000200000100 badaddr: 0000000000000000 cause: 0000000000000003 > > > [] lock_is_held_type+0x78/0x14e > > > [] __might_resched+0x26/0x22c > > > [] __might_sleep+0x3c/0x66 > > > [] get_signal+0x9e/0xa70 > > > [] do_notify_resume+0x6e/0x422 > > > [] ret_from_exception+0x0/0x10 > > > irq event stamp: 44512 > > > hardirqs last enabled at (44511): [] _raw_spin_unlock_irqrestore+0x54/0x62 > > > hardirqs last disabled at (44512): [] __trace_hardirqs_off+0xc/0x14 > > > softirqs last enabled at (44472): [] __do_softirq+0x3de/0x51e > > > softirqs last disabled at (44467): [] irq_exit+0xd6/0x104 > > > ---[ end trace 0000000000000000 ]--- > > > possible reason: unannotated irqs-on. > > > > > > Signed-off-by: Andrew Bresticker > > > --- > > > This should also theoretically be fixed by the conversion to generic entry, > > > but it's not clear how far away that series is from landing. > > > --- > > > arch/riscv/kernel/entry.S | 18 +++++------------- > > > arch/riscv/kernel/signal.c | 34 +++++++++++++++++++++------------- > > > 2 files changed, 26 insertions(+), 26 deletions(-) > > > > > > diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S > > > index b9eda3fcbd6d..58dfa8595e19 100644 > > > --- a/arch/riscv/kernel/entry.S > > > +++ b/arch/riscv/kernel/entry.S > > > @@ -263,12 +263,11 @@ ret_from_exception: > > > #endif > > > bnez s0, resume_kernel > > > > > > -resume_userspace: > > > /* Interrupts must be disabled here so flags are checked atomically */ > > > REG_L s0, TASK_TI_FLAGS(tp) /* current_thread_info->flags */ > > > andi s1, s0, _TIF_WORK_MASK > > > - bnez s1, work_pending > > > - > > > + bnez s1, resume_userspace_slow > > > +resume_userspace: > > > #ifdef CONFIG_CONTEXT_TRACKING_USER > > > call user_enter_callable > > > #endif > > > @@ -368,19 +367,12 @@ resume_kernel: > > > j restore_all > > > #endif > > > > > > -work_pending: > > > +resume_userspace_slow: > > > /* Enter slow path for supplementary processing */ > > > - la ra, ret_from_exception > > > - andi s1, s0, _TIF_NEED_RESCHED > > > - bnez s1, work_resched > > > -work_notifysig: > > > - /* Handle pending signals and notify-resume requests */ > > > - csrs CSR_STATUS, SR_IE /* Enable interrupts for do_notify_resume() */ > > > move a0, sp /* pt_regs */ > > > move a1, s0 /* current_thread_info->flags */ > > > - tail do_notify_resume > > > -work_resched: > > > - tail schedule > > > + call do_work_pending > > > + j resume_userspace > > > > > > /* Slow paths for ptrace. */ > > > handle_syscall_trace_enter: > > > diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c > > > index 5c591123c440..bfb2afa4135f 100644 > > > --- a/arch/riscv/kernel/signal.c > > > +++ b/arch/riscv/kernel/signal.c > > > @@ -313,19 +313,27 @@ static void do_signal(struct pt_regs *regs) > > > } > > > > > > /* > > > - * notification of userspace execution resumption > > > - * - triggered by the _TIF_WORK_MASK flags > > > + * Handle any pending work on the resume-to-userspace path, as indicated by > > > + * _TIF_WORK_MASK. Entered from assembly with IRQs off. > > > */ > > > -asmlinkage __visible void do_notify_resume(struct pt_regs *regs, > > > - unsigned long thread_info_flags) > > > +asmlinkage __visible void do_work_pending(struct pt_regs *regs, > > > + unsigned long thread_info_flags) > > > { > > > - if (thread_info_flags & _TIF_UPROBE) > > > - uprobe_notify_resume(regs); > > > - > > > - /* Handle pending signal delivery */ > > > - if (thread_info_flags & (_TIF_SIGPENDING | _TIF_NOTIFY_SIGNAL)) > > > - do_signal(regs); > > > - > > > - if (thread_info_flags & _TIF_NOTIFY_RESUME) > > > - resume_user_mode_work(regs); > > > + do { > > > + if (thread_info_flags & _TIF_NEED_RESCHED) { > > > + schedule(); > > > + } else { > > > + local_irq_enable(); > > > + if (thread_info_flags & _TIF_UPROBE) > > > + uprobe_notify_resume(regs); > > > + /* Handle pending signal delivery */ > > > + if (thread_info_flags & (_TIF_SIGPENDING | > > > + _TIF_NOTIFY_SIGNAL)) > > > + do_signal(regs); > > > + if (thread_info_flags & _TIF_NOTIFY_RESUME) > > > + resume_user_mode_work(regs); > > > + } > > > + local_irq_disable(); > > > + thread_info_flags = read_thread_flags(); > > > + } while (thread_info_flags & _TIF_WORK_MASK); > > > } > > The more graceful code has been written in kernel/entry/common.c. > > Let's base it on that: > > > > static unsigned long exit_to_user_mode_loop(struct pt_regs *regs, > > unsigned long ti_work) > > { > > /* > > * Before returning to user space ensure that all pending work > > * items have been completed. > > */ > > while (ti_work & EXIT_TO_USER_MODE_WORK) { > > > > local_irq_enable_exit_to_user(ti_work); > > > > if (ti_work & _TIF_NEED_RESCHED) > > schedule(); > > > > if (ti_work & _TIF_UPROBE) > > uprobe_notify_resume(regs); > > > > if (ti_work & _TIF_PATCH_PENDING) > > klp_update_patch_state(current); > > > > if (ti_work & (_TIF_SIGPENDING | _TIF_NOTIFY_SIGNAL)) > > arch_do_signal_or_restart(regs); > > > > if (ti_work & _TIF_NOTIFY_RESUME) > > resume_user_mode_work(regs); > > > > /* Architecture specific TIF work */ > > arch_exit_to_user_mode_work(regs, ti_work); > > > > /* > > * Disable interrupts and reevaluate the work flags as they > > * might have changed while interrupts and preemption was > > * enabled above. > > */ > > local_irq_disable_exit_to_user(); > > > > /* Check if any of the above work has queued a > > deferred wakeup */ > > tick_nohz_user_enter_prepare(); > > > > ti_work = read_thread_flags(); > > } > > > > /* Return the latest work state for arch_exit_to_user_mode() */ > > return ti_work; > > } > > > > > -- > > > 2.25.1 > > > > > > > > > -- > > Best Regards > > Guo Ren > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv