linux-kernel - Re: [PATCH] PM / devfreq: missing rcu_read_lock() added for find_device

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]

Message-ID: <20121122174542.GA15651@mail.hallyn.com>
Date:	Thu, 22 Nov 2012 17:45:42 +0000
From:	"Serge E. Hallyn" <serge@...lyn.com>
To:	"Serge E. Hallyn" <serge@...lyn.com>
Cc:	MyungJoo Ham <myungjoo.ham@...sung.com>, myungjoo.ham@...il.com,
	linux-pm@...r.kernel.org, rjw@...k.pl, keescook@...omium.org,
	serge.hallyn@...onical.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] PM / devfreq: missing rcu_read_lock() added for
 find_device_opp()

Quoting Serge E. Hallyn (serge@...lyn.com):
> Quoting MyungJoo Ham (myungjoo.ham@...sung.com):
> > opp_get_notifier() uses find_device_opp(), which requires to
> > held rcu_read_lock. In order to keep the notifier-header
> > valid, we have added rcu_read_lock().
> > 

Well, to be honest, the opp locking isn't 100% clear to me, but IIUC
(1) things can be added but never removed, (2) opp_get_notifier doesn't
pin a refcount on what it returns, but it returns something which won't
be deleted.

So IIUC what's below is fine, bc it doesn't need to pin the nh for the
nh to remain valid outside of the rcu_read_lock.  If I'm wrong about
that, then the below is not sufficient.

> Reviewed-by: Serge Hallyn <serge.hallyn@...onical.com>
> 
> > Reported-by: Kees Cook <keescook@...omium.org>
> > Signed-off-by: MyungJoo Ham <myungjoo.ham@...sung.com>
> > ---
> >  drivers/devfreq/devfreq.c |   26 ++++++++++++++++++++------
> >  1 files changed, 20 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > index 1388d46..5275883 100644
> > --- a/drivers/devfreq/devfreq.c
> > +++ b/drivers/devfreq/devfreq.c
> > @@ -1023,11 +1023,18 @@ struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq,
> >   */
> >  int devfreq_register_opp_notifier(struct device *dev, struct devfreq *devfreq)
> >  {
> > -	struct srcu_notifier_head *nh = opp_get_notifier(dev);
> > +	struct srcu_notifier_head *nh;
> > +	int ret = 0;
> >  
> > +	rcu_read_lock();
> > +	nh = opp_get_notifier(dev);
> >  	if (IS_ERR(nh))
> > -		return PTR_ERR(nh);
> > -	return srcu_notifier_chain_register(nh, &devfreq->nb);
> > +		ret = PTR_ERR(nh);
> > +	rcu_read_unlock();
> > +	if (!ret)
> > +		ret = srcu_notifier_chain_register(nh, &devfreq->nb);
> > +
> > +	return ret;
> >  }
> >  
> >  /**
> > @@ -1042,11 +1049,18 @@ int devfreq_register_opp_notifier(struct device *dev, struct devfreq *devfreq)
> >   */
> >  int devfreq_unregister_opp_notifier(struct device *dev, struct devfreq *devfreq)
> >  {
> > -	struct srcu_notifier_head *nh = opp_get_notifier(dev);
> > +	struct srcu_notifier_head *nh;
> > +	int ret = 0;
> >  
> > +	rcu_read_lock();
> > +	nh = opp_get_notifier(dev);
> >  	if (IS_ERR(nh))
> > -		return PTR_ERR(nh);
> > -	return srcu_notifier_chain_unregister(nh, &devfreq->nb);
> > +		ret = PTR_ERR(nh);
> > +	rcu_read_unlock();
> > +	if (!ret)
> > +		ret = srcu_notifier_chain_unregister(nh, &devfreq->nb);
> > +
> > +	return ret;
> >  }
> >  
> >  MODULE_AUTHOR("MyungJoo Ham <myungjoo.ham@...sung.com>");
> > -- 
> > 1.7.5.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/