linux-kernel - Re: [PATCH 1/3] jump_label: Pull get_online

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] [day] [month] [year] [list]

Date:   Fri, 21 Apr 2017 18:20:31 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Jason Baron <jbaron@...mai.com>
Cc:     rostedt@...dmis.org, tglx@...utronix.de, mingo@...nel.org,
        bigeasy@...utronix.de, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] jump_label: Pull get_online_cpus() into generic code

On Fri, Apr 21, 2017 at 12:08:26PM -0400, Jason Baron wrote:
> On 04/18/2017 06:32 AM, Peter Zijlstra wrote:
> > @@ -146,6 +154,7 @@ static void __static_key_slow_dec(struct
> >  	 * returns is unbalanced, because all other static_key_slow_inc()
> >  	 * instances block while the update is in progress.
> >  	 */
> > +	get_online_cpus();
> >  	if (!atomic_dec_and_mutex_lock(&key->enabled, &jump_label_mutex)) {
> >  		WARN(atomic_read(&key->enabled) < 0,
> >  		     "jump label: negative count!\n");
> 
> So the get and put can be unbalanced here since the above:
> 
> 'if (!atomic_dec_and_mutex_lock(&key->enabled, &jump_label_mutex))'
> 
> is followed by 'return;'. However, I see that the next patch removes this
> and so things are balanced again...

Duh.. right you are.

> > @@ -159,6 +168,7 @@ static void __static_key_slow_dec(struct
> >  		jump_label_update(key);
> >  	}
> >  	jump_label_unlock();
> > +	put_online_cpus();
> >  }
> > 
> >  static void jump_label_update_timeout(struct work_struct *work)
> > @@ -592,6 +602,10 @@ jump_label_module_notify(struct notifier
> > 
> >  	switch (val) {
> >  	case MODULE_STATE_COMING:
> > +		/*
> > +		 * XXX do we need get_online_cpus() ?  the module isn't
> > +		 * executable yet, so nothing should be looking at our code.
> > +		 */
> 
> Since we're just updating the table of places we potentially need to patch,
> but not actually doing any patching, we should not need get_online_cpus()
> here...so in attempt to reduce confusion I would remove this.

Thanks for confirming it is indeed not required. Will make it go away.