linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* opp_get_notifier() needs to be under rcu_lock?
@ 2012-11-20 20:08 Kees Cook
  2012-11-21 10:10 ` [PATCH] PM / devfreq: missing rcu_read_lock() added for find_device_opp() MyungJoo Ham
  0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2012-11-20 20:08 UTC (permalink / raw)
  To: MyungJoo Ham; +Cc: LKML, Serge Hallyn

Hi,

It looks like find_device_opp() (called from opp_get_notifier()) needs
to be under RCU read lock, but this doesn't seem to be happening in
drivers/devfreq/devfreq.c. Doesn't this run the risk of referencing a
freed variable?

Thanks,

-Kees

-- 
Kees Cook
Chrome OS Security

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] PM / devfreq: missing rcu_read_lock() added for find_device_opp()
  2012-11-20 20:08 opp_get_notifier() needs to be under rcu_lock? Kees Cook
@ 2012-11-21 10:10 ` MyungJoo Ham
  2012-11-21 13:46   ` Serge E. Hallyn
  0 siblings, 1 reply; 6+ messages in thread
From: MyungJoo Ham @ 2012-11-21 10:10 UTC (permalink / raw)
  To: linux-pm; +Cc: rjw, keescook, serge.hallyn, linux-kernel, myungjoo.ham

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().

Reported-by: Kees Cook <keescook@chromium.org>
Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.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 45e053e..e91cb22 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);
+	if (!ret)
+		ret = srcu_notifier_chain_register(nh, &devfreq->nb);
+	rcu_read_unlock();
+
+	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);
+	if (!ret)
+		ret = srcu_notifier_chain_unregister(nh, &devfreq->nb);
+	rcu_read_unlock();
+
+	return ret;
 }
 
 MODULE_AUTHOR("MyungJoo Ham <myungjoo.ham@samsung.com>");
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] PM / devfreq: missing rcu_read_lock() added for find_device_opp()
  2012-11-21 10:10 ` [PATCH] PM / devfreq: missing rcu_read_lock() added for find_device_opp() MyungJoo Ham
@ 2012-11-21 13:46   ` Serge E. Hallyn
  2012-11-22  6:36     ` MyungJoo Ham
  0 siblings, 1 reply; 6+ messages in thread
From: Serge E. Hallyn @ 2012-11-21 13:46 UTC (permalink / raw)
  To: MyungJoo Ham
  Cc: linux-pm, rjw, keescook, serge.hallyn, linux-kernel, myungjoo.ham

Quoting MyungJoo Ham (myungjoo.ham@samsung.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().
> 
> Reported-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.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 45e053e..e91cb22 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);
> +	if (!ret)
> +		ret = srcu_notifier_chain_register(nh, &devfreq->nb);

Hm, but if I'm seeing right, srcu_notifier_chain_register calls
mutex_lock(), which sleeps, so you can't do that under rcu_read_lock().

-serge

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] PM / devfreq: missing rcu_read_lock() added for find_device_opp()
  2012-11-21 13:46   ` Serge E. Hallyn
@ 2012-11-22  6:36     ` MyungJoo Ham
  2012-11-22 14:09       ` Serge E. Hallyn
  0 siblings, 1 reply; 6+ messages in thread
From: MyungJoo Ham @ 2012-11-22  6:36 UTC (permalink / raw)
  To: serge; +Cc: myungjoo.ham, linux-pm, rjw, keescook, serge.hallyn, linux-kernel

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().

Reported-by: Kees Cook <keescook@chromium.org>
Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.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@samsung.com>");
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] PM / devfreq: missing rcu_read_lock() added for find_device_opp()
  2012-11-22  6:36     ` MyungJoo Ham
@ 2012-11-22 14:09       ` Serge E. Hallyn
  2012-11-22 17:45         ` Serge E. Hallyn
  0 siblings, 1 reply; 6+ messages in thread
From: Serge E. Hallyn @ 2012-11-22 14:09 UTC (permalink / raw)
  To: MyungJoo Ham
  Cc: serge, myungjoo.ham, linux-pm, rjw, keescook, serge.hallyn, linux-kernel

Quoting MyungJoo Ham (myungjoo.ham@samsung.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().
> 

Reviewed-by: Serge Hallyn <serge.hallyn@canonical.com>

> Reported-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.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@samsung.com>");
> -- 
> 1.7.5.4

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] PM / devfreq: missing rcu_read_lock() added for find_device_opp()
  2012-11-22 14:09       ` Serge E. Hallyn
@ 2012-11-22 17:45         ` Serge E. Hallyn
  0 siblings, 0 replies; 6+ messages in thread
From: Serge E. Hallyn @ 2012-11-22 17:45 UTC (permalink / raw)
  To: Serge E. Hallyn
  Cc: MyungJoo Ham, myungjoo.ham, linux-pm, rjw, keescook,
	serge.hallyn, linux-kernel

Quoting Serge E. Hallyn (serge@hallyn.com):
> Quoting MyungJoo Ham (myungjoo.ham@samsung.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@canonical.com>
> 
> > Reported-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.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@samsung.com>");
> > -- 
> > 1.7.5.4

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-11-22 18:51 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-11-20 20:08 opp_get_notifier() needs to be under rcu_lock? Kees Cook
2012-11-21 10:10 ` [PATCH] PM / devfreq: missing rcu_read_lock() added for find_device_opp() MyungJoo Ham
2012-11-21 13:46   ` Serge E. Hallyn
2012-11-22  6:36     ` MyungJoo Ham
2012-11-22 14:09       ` Serge E. Hallyn
2012-11-22 17:45         ` Serge E. Hallyn

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).