linux-kernel - Re: [PATCH v5 1/6] kprobes: Remove dependency to the module

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200725102110.GA757173@gmail.com>
Date:   Sat, 25 Jul 2020 12:21:10 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Andi Kleen <ak@...ux.intel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Jessica Yu <jeyu@...nel.org>,
        "Naveen N. Rao" <naveen.n.rao@...ux.ibm.com>,
        Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>,
        "David S. Miller" <davem@...emloft.net>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...hat.com>
Subject: Re: [PATCH v5 1/6] kprobes: Remove dependency to the module_mutex


* Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com> wrote:

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