Re: [PATCH v5 1/6] kprobes: Remove dependency to the module_mutex

[Masami Hiramatsu <mhiramat@kernel.org>]

On Sat, 25 Jul 2020 12:21:10 +0200
Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> 
> > On Fri, Jul 24, 2020 at 11:17:11AM +0200, Ingo Molnar wrote:
> > > 
> > > * Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > > 
> > > > --- a/kernel/kprobes.c
> > > > +++ b/kernel/kprobes.c
> > > > @@ -564,7 +564,7 @@ static void kprobe_optimizer(struct work_struct *work)
> > > >  	cpus_read_lock();
> > > >  	mutex_lock(&text_mutex);
> > > >  	/* Lock modules while optimizing kprobes */
> > > > -	mutex_lock(&module_mutex);
> > > > +	lock_modules();
> > > >  
> > > >  	/*
> > > >  	 * Step 1: Unoptimize kprobes and collect cleaned (unused and disarmed)
> > > > @@ -589,7 +589,7 @@ static void kprobe_optimizer(struct work_struct *work)
> > > >  	/* Step 4: Free cleaned kprobes after quiesence period */
> > > >  	do_free_cleaned_kprobes();
> > > >  
> > > > -	mutex_unlock(&module_mutex);
> > > > +	unlock_modules();
> > > >  	mutex_unlock(&text_mutex);
> > > >  	cpus_read_unlock();
> > > 
> > > BTW., it would be nice to expand on the comments above - exactly which 
> > > parts of the modules code is being serialized against and why?
> > > 
> > > We already hold the text_mutex here, which should protect against most 
> > > kprobes related activities interfering - and it's unclear (to me) 
> > > which part of the modules code is being serialized with here, and the 
> > > 'lock modules while optimizing kprobes' comments is unhelpful. :-)
> > > 
> > > Thanks,
> > > 
> > > 	Ingo
> > 
> > AFAIK, only if you need to call find_module(), you ever need to acquire
> > this mutex. 99% of time it is internally taken care by kernel/module.c.
> > 
> > I cannot make up any obvious reason to acquire it here.
> 
> If it's unnecessary, then it needs to be removed.
> 
> If it's necessary, then it needs to be documented better.

Good catch! This is not needed anymore.
It has been introduced to avoid conflict with text modification, at that
point we didn't get text_mutex. But after commit f1c6ece23729 ("kprobes: Fix 
potential deadlock in kprobe_optimizer()") moved the text_mutex in the current
position, we don't need it. (and anyway, keeping kprobe_mutex locked means
any module unloading will be stopped inside kprobes_module_callback())

This may help?

From 2355ecd941c3234b12a6de7443592848ed4e2087 Mon Sep 17 00:00:00 2001
From: Masami Hiramatsu <mhiramat@kernel.org>
Date: Tue, 28 Jul 2020 16:32:34 +0900
Subject: [PATCH] kprobes: Remove unneeded module_mutex lock from the optimizer

Remove unneeded module_mutex locking from the optimizer. Since
we already locks both kprobe_mutex and text_mutex in the optimizer,
text will not be changed and the module unloading will be stopped
inside kprobes_module_callback().

Suggested-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/kprobes.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 4a904cc56d68..d1b02e890793 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -563,8 +563,6 @@ static void kprobe_optimizer(struct work_struct *work)
 	mutex_lock(&kprobe_mutex);
 	cpus_read_lock();
 	mutex_lock(&text_mutex);
-	/* Lock modules while optimizing kprobes */
-	mutex_lock(&module_mutex);
 
 	/*
 	 * Step 1: Unoptimize kprobes and collect cleaned (unused and disarmed)
@@ -589,7 +587,6 @@ static void kprobe_optimizer(struct work_struct *work)
 	/* Step 4: Free cleaned kprobes after quiesence period */
 	do_free_cleaned_kprobes();
 
-	mutex_unlock(&module_mutex);
 	mutex_unlock(&text_mutex);
 	cpus_read_unlock();
 
-- 
2.25.1
-- 
Masami Hiramatsu <mhiramat@kernel.org>