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

[Masami Hiramatsu]

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

> 
> * Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx> wrote:
> 
> > On Fri, Jul 24, 2020 at 11:17:11AM +0200, Ingo Molnar wrote:
> > > 
> > > * Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx> 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?