[PATCH 0/2] x86/mm/kmmio: Have mmiotracer play nice with lockdep

[PATCH 0/2] x86/mm/kmmio: Have mmiotracer play nice with lockdep

[Steven Rostedt]

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
needs to grab a lock while in the breakpoint handler. This is considered
an NMI state, and then lockdep warns that the lock is being held in both
an NMI state (really a breakpoint handler) and also in normal context.

As the breakpoint/NMI state only happens when the driver is accessing
memory, there's no concern of a race condition against the setup and
tear-down of mmiotracer.

To make lockdep and mmiotrace work together, convert the locks used in the
breakpoint handler into arch_spin_lock().

It also takes the rcu_read_lock() at the fault, and releases it in
the single step breakpoint. This makes lockdep sad. Luckily, it also
disables preemption when taking the rcu_read_lock() and enables it when
releasing the rcu_read_lock(). As now the sched RCU variant is basically
the same as the "normal" RCU variant, there's no reason to take
the rcu_read_lock(), so just remove it.

Steven Rostedt (2):
      x86/mm/kmmio: Switch to arch_spin_lock()
      x86/mm/kmmio: Remove rcu_read_lock()

----
 arch/x86/mm/kmmio.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

[PATCH 1/2] x86/mm/kmmio: Switch to arch_spin_lock()

[Steven Rostedt]

From: Steven Rostedt <rostedt@goodmis.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
needs to grab a lock while in the breakpoint handler. This is considered
an NMI state, and then lockdep warns that the lock is being held in both
an NMI state (really a breakpoint handler) and also in normal context.

As the breakpoint/NMI state only happens when the driver is accessing
memory, there's no concern of a race condition against the setup and
tear-down of mmiotracer.

To make lockdep and mmiotrace work together, convert the locks used in the
breakpoint handler into arch_spin_lock().

Link: https://lore.kernel.org/lkml/20221201213126.620b7dd3@gandalf.local.home/

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/mm/kmmio.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/arch/x86/mm/kmmio.c b/arch/x86/mm/kmmio.c
index d3efbc5b3449..edb486450158 100644
--- a/arch/x86/mm/kmmio.c
+++ b/arch/x86/mm/kmmio.c
@@ -62,7 +62,13 @@ struct kmmio_context {
 	int active;
 };
 
-static DEFINE_SPINLOCK(kmmio_lock);
+/*
+ * The kmmio_lock is taken in int3 context, which is treated as NMI context.
+ * This causes lockdep to complain about it bein in both NMI and normal
+ * context. Hide it from lockdep, as it should not have any other locks
+ * taken under it, and this is only enabled for debugging mmio anyway.
+ */
+static arch_spinlock_t kmmio_lock = __ARCH_SPIN_LOCK_UNLOCKED;
 
 /* Protected by kmmio_lock */
 unsigned int kmmio_count;
@@ -346,10 +352,10 @@ static int post_kmmio_handler(unsigned long condition, struct pt_regs *regs)
 		ctx->probe->post_handler(ctx->probe, condition, regs);
 
 	/* Prevent racing against release_kmmio_fault_page(). */
-	spin_lock(&kmmio_lock);
+	arch_spin_lock(&kmmio_lock);
 	if (ctx->fpage->count)
 		arm_kmmio_fault_page(ctx->fpage);
-	spin_unlock(&kmmio_lock);
+	arch_spin_unlock(&kmmio_lock);
 
 	regs->flags &= ~X86_EFLAGS_TF;
 	regs->flags |= ctx->saved_flags;
@@ -440,7 +446,8 @@ int register_kmmio_probe(struct kmmio_probe *p)
 	unsigned int l;
 	pte_t *pte;
 
-	spin_lock_irqsave(&kmmio_lock, flags);
+	local_irq_save(flags);
+	arch_spin_lock(&kmmio_lock);
 	if (get_kmmio_probe(addr)) {
 		ret = -EEXIST;
 		goto out;
@@ -460,7 +467,9 @@ int register_kmmio_probe(struct kmmio_probe *p)
 		size += page_level_size(l);
 	}
 out:
-	spin_unlock_irqrestore(&kmmio_lock, flags);
+	arch_spin_unlock(&kmmio_lock);
+	local_irq_restore(flags);
+
 	/*
 	 * XXX: What should I do here?
 	 * Here was a call to global_flush_tlb(), but it does not exist
@@ -494,7 +503,8 @@ static void remove_kmmio_fault_pages(struct rcu_head *head)
 	struct kmmio_fault_page **prevp = &dr->release_list;
 	unsigned long flags;
 
-	spin_lock_irqsave(&kmmio_lock, flags);
+	local_irq_save(flags);
+	arch_spin_lock(&kmmio_lock);
 	while (f) {
 		if (!f->count) {
 			list_del_rcu(&f->list);
@@ -506,7 +516,8 @@ static void remove_kmmio_fault_pages(struct rcu_head *head)
 		}
 		f = *prevp;
 	}
-	spin_unlock_irqrestore(&kmmio_lock, flags);
+	arch_spin_unlock(&kmmio_lock);
+	local_irq_restore(flags);
 
 	/* This is the real RCU destroy call. */
 	call_rcu(&dr->rcu, rcu_free_kmmio_fault_pages);
@@ -540,14 +551,16 @@ void unregister_kmmio_probe(struct kmmio_probe *p)
 	if (!pte)
 		return;
 
-	spin_lock_irqsave(&kmmio_lock, flags);
+	local_irq_save(flags);
+	arch_spin_lock(&kmmio_lock);
 	while (size < size_lim) {
 		release_kmmio_fault_page(addr + size, &release_list);
 		size += page_level_size(l);
 	}
 	list_del_rcu(&p->list);
 	kmmio_count--;
-	spin_unlock_irqrestore(&kmmio_lock, flags);
+	arch_spin_unlock(&kmmio_lock);
+	local_irq_restore(flags);
 
 	if (!release_list)
 		return;
-- 
2.35.1

[PATCH 2/2] x86/mm/kmmio: Remove rcu_read_lock()

[Steven Rostedt]

From: Steven Rostedt <rostedt@goodmis.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@kernel.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.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

Re: [PATCH 2/2] x86/mm/kmmio: Remove rcu_read_lock()

[Paul E. McKenney]

On Tue, Dec 06, 2022 at 02:12:03PM -0500, Steven Rostedt wrote:
> From: Steven Rostedt <rostedt@goodmis.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@kernel.org>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.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@kernel.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
> 
> 

Re: [PATCH 2/2] x86/mm/kmmio: Remove rcu_read_lock()

[Steven Rostedt]

On Wed, 7 Dec 2022 09:36:21 -0800
"Paul E. McKenney" <paulmck@kernel.org> wrote:

> > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.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.

Good point. I'll add a comment in v2.

> 
> Acked-by: Paul E. McKenney <paulmck@kernel.org>

Thanks!

-- Steve

Re: [PATCH 2/2] x86/mm/kmmio: Remove rcu_read_lock()

[Steven Rostedt]

On Fri, 9 Dec 2022 13:03:34 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 7 Dec 2022 09:36:21 -0800
> "Paul E. McKenney" <paulmck@kernel.org> wrote:
> 
> > > Cc: "Paul E. McKenney" <paulmck@kernel.org>
> > > Signed-off-by: Steven Rostedt <rostedt@goodmis.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.  
> 
> Good point. I'll add a comment in v2.

Actually, rcu_read_lock_sched_notrace() may work instead. Let me test it.

-- Steve