Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 25 Jul 2020 12:21:10 +0200
From:   Ingo Molnar <mingo@kernel.org>
To:     Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Andi Kleen <ak@linux.intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Jessica Yu <jeyu@kernel.org>,
        "Naveen N. Rao" <naveen.n.rao@linux.ibm.com>,
        Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>,
        "David S. Miller" <davem@davemloft.net>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ingo Molnar <mingo@redhat.com>
Subject: Re: [PATCH v5 1/6] kprobes: Remove dependency to the module_mutex
Message-ID: <20200725102110.GA757173@gmail.com>
References: <20200724050553.1724168-1-jarkko.sakkinen@linux.intel.com>
 <20200724050553.1724168-2-jarkko.sakkinen@linux.intel.com>
 <20200724091711.GB517988@gmail.com>
 <20200725030107.GF17052@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200725030107.GF17052@linux.intel.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


* 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.

Thanks,

	Ingo