[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20221207173621.GF4001@paulmck-ThinkPad-P17-Gen-1>
Date: Wed, 7 Dec 2022 09:36:21 -0800
From: "Paul E. McKenney" <paulmck@...nel.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
Masami Hiramatsu <mhiramat@...nel.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Karol Herbst <karolherbst@...il.com>,
Pekka Paalanen <ppaalanen@...il.com>,
Dave Hansen <dave.hansen@...ux.intel.com>,
Andy Lutomirski <luto@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
x86@...nel.org
Subject: Re: [PATCH 2/2] x86/mm/kmmio: Remove rcu_read_lock()
On Tue, Dec 06, 2022 at 02:12:03PM -0500, Steven Rostedt wrote:
> From: Steven Rostedt <rostedt@...dmis.org>
>
> The mmiotrace tracer is "special". The purpose is to help reverse engineer
> binary drivers by removing the memory allocated by the driver and when the
> driver goes to access it, a fault occurs, the mmiotracer will record what
> the driver was doing and then do the work on its behalf by single stepping
> through the process.
>
> But to achieve this ability, it must do some special things. One is it
> take the rcu_read_lock() when the fault occurs, and then release it in the
> breakpoint that in the single stepping. This makes lockdep unhappy, as it
> changes the state of RCU from within an exception that is not contained in
> that exception, and we get a nasty splat from lockdep.
>
> As it also disables preemption everywhere rcu_read_lock() is taken, and
> enables preemption everywhere rcu_read_unlock(), and does not enable
> preemption in between, it is the same as synchronize_rcu_sched(). But as
> the RCU sched variant has the same grace period as normal RCU, there's no
> reason to take the rcu_read_lock(). Simply remove it.
>
> Cc: "Paul E. McKenney" <paulmck@...nel.org>
> Signed-off-by: Steven Rostedt <rostedt@...dmis.org>
Might be worth adding a comment saying that others are using this
preempt_disable() to block an RCU grace period, but that is up to
you guys. I will let you and your future selves be the judges.
Acked-by: Paul E. McKenney <paulmck@...nel.org>
> ---
> arch/x86/mm/kmmio.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/arch/x86/mm/kmmio.c b/arch/x86/mm/kmmio.c
> index edb486450158..e15e3aaaf94c 100644
> --- a/arch/x86/mm/kmmio.c
> +++ b/arch/x86/mm/kmmio.c
> @@ -254,7 +254,6 @@ int kmmio_handler(struct pt_regs *regs, unsigned long addr)
> * again.
> */
> preempt_disable();
> - rcu_read_lock();
>
> faultpage = get_kmmio_fault_page(page_base);
> if (!faultpage) {
> @@ -323,7 +322,6 @@ int kmmio_handler(struct pt_regs *regs, unsigned long addr)
> return 1; /* fault handled */
>
> no_kmmio:
> - rcu_read_unlock();
> preempt_enable_no_resched();
> return ret;
> }
> @@ -363,7 +361,6 @@ static int post_kmmio_handler(unsigned long condition, struct pt_regs *regs)
> /* These were acquired in kmmio_handler(). */
> ctx->active--;
> BUG_ON(ctx->active);
> - rcu_read_unlock();
> preempt_enable_no_resched();
>
> /*
> --
> 2.35.1
>
>
Powered by blists - more mailing lists