[PATCH] PM / devfreq: stopping the governor before device_unregister()

[PATCH] PM / devfreq: stopping the governor before device_unregister()

[vincent.donnefort]

From: Vincent Donnefort <vincent.donnefort@arm.com>

device_release() is freeing the resources before calling the device
specific release callback which is, in the case of devfreq, stopping
the governor.

It is a problem as some governors are using the device resources. e.g.
simpleondemand which is using the devfreq deferrable monitoring work. If it
is not stopped before the resources are freed, it might lead to a use after
free.

Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>
Reviewed-by: John Einar Reitan <john.reitan@arm.com>

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 4c49bb1..4e43830 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -534,10 +534,6 @@ static void devfreq_dev_release(struct device *dev)
 	list_del(&devfreq->node);
 	mutex_unlock(&devfreq_list_lock);
 
-	if (devfreq->governor)
-		devfreq->governor->event_handler(devfreq,
-						 DEVFREQ_GOV_STOP, NULL);
-
 	if (devfreq->profile->exit)
 		devfreq->profile->exit(devfreq->dev.parent);
 
@@ -672,7 +668,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	list_del(&devfreq->node);
 	mutex_unlock(&devfreq_list_lock);
 
-	device_unregister(&devfreq->dev);
+	devfreq_remove_device(devfreq);
 	devfreq = NULL;
 err_dev:
 	if (devfreq)
@@ -693,6 +689,9 @@ int devfreq_remove_device(struct devfreq *devfreq)
 	if (!devfreq)
 		return -EINVAL;
 
+	if (devfreq->governor)
+		devfreq->governor->event_handler(devfreq,
+						 DEVFREQ_GOV_STOP, NULL);
 	device_unregister(&devfreq->dev);
 
 	return 0;
-- 
1.9.1

Re: [PATCH] PM / devfreq: stopping the governor before device_unregister()

[Chanwoo Choi]

Hi,

On 2018년 08월 30일 19:02, vincent.donnefort@arm.com wrote:
> From: Vincent Donnefort <vincent.donnefort@arm.com>
> 
> device_release() is freeing the resources before calling the device
> specific release callback which is, in the case of devfreq, stopping
> the governor.
> 
> It is a problem as some governors are using the device resources. e.g.
> simpleondemand which is using the devfreq deferrable monitoring work. If it
> is not stopped before the resources are freed, it might lead to a use after
> free.
> 
> Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>
> Reviewed-by: John Einar Reitan <john.reitan@arm.com>
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 4c49bb1..4e43830 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -534,10 +534,6 @@ static void devfreq_dev_release(struct device *dev)
>  	list_del(&devfreq->node);
>  	mutex_unlock(&devfreq_list_lock);
>  
> -	if (devfreq->governor)
> -		devfreq->governor->event_handler(devfreq,
> -						 DEVFREQ_GOV_STOP, NULL);
> -
>  	if (devfreq->profile->exit)
>  		devfreq->profile->exit(devfreq->dev.parent);
>  
> @@ -672,7 +668,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	list_del(&devfreq->node);
>  	mutex_unlock(&devfreq_list_lock);
>  
> -	device_unregister(&devfreq->dev);
> +	devfreq_remove_device(devfreq);
>  	devfreq = NULL;
>  err_dev:
>  	if (devfreq)
> @@ -693,6 +689,9 @@ int devfreq_remove_device(struct devfreq *devfreq)
>  	if (!devfreq)
>  		return -EINVAL;
>  
> +	if (devfreq->governor)
> +		devfreq->governor->event_handler(devfreq,
> +						 DEVFREQ_GOV_STOP, NULL);
>  	device_unregister(&devfreq->dev);
>  
>  	return 0;
> 

As description of this patch, if devfreq_wq is executed and then execute
the 'devfreq->governor->get_target_freq' between step1 and step2
after already freed the 'dev' related resource, it might happen the problem
because the registered callback of get_target_freq requires the 'dev' resource.

device_unregister(dev)
	step 1. device_del(dev)
		<- if devfreq_wq is executed
	step 2. put_device(dev)
		device_release()
			devfreq_dev_release()
				stop the governor for specific devfreq instance

It looks good to me. Stop the governor before calling device_unregister().
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>				

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

[PATCH v2] PM / devfreq: Stop the governor before device_unregister()

[vincent.donnefort]

From: Vincent Donnefort <vincent.donnefort@arm.com>

device_release() is freeing the resources before calling the device
specific release callback which is, in the case of devfreq, stopping
the governor.

It is a problem as some governors are using the device resources. e.g.
simpleondemand which is using the devfreq deferrable monitoring work. If it
is not stopped before the resources are freed, it might lead to a use after
free.

Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>
Reviewed-by: John Einar Reitan <john.reitan@arm.com>
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 4c49bb1..4e43830 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -534,10 +534,6 @@ static void devfreq_dev_release(struct device *dev)
 	list_del(&devfreq->node);
 	mutex_unlock(&devfreq_list_lock);
 
-	if (devfreq->governor)
-		devfreq->governor->event_handler(devfreq,
-						 DEVFREQ_GOV_STOP, NULL);
-
 	if (devfreq->profile->exit)
 		devfreq->profile->exit(devfreq->dev.parent);
 
@@ -672,7 +668,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	list_del(&devfreq->node);
 	mutex_unlock(&devfreq_list_lock);
 
-	device_unregister(&devfreq->dev);
+	devfreq_remove_device(devfreq);
 	devfreq = NULL;
 err_dev:
 	if (devfreq)
@@ -693,6 +689,9 @@ int devfreq_remove_device(struct devfreq *devfreq)
 	if (!devfreq)
 		return -EINVAL;
 
+	if (devfreq->governor)
+		devfreq->governor->event_handler(devfreq,
+						 DEVFREQ_GOV_STOP, NULL);
 	device_unregister(&devfreq->dev);
 
 	return 0;
-- 
1.9.1

RE: [PATCH v2] PM / devfreq: Stop the governor before device_unregister()

[MyungJoo Ham]

> From: Vincent Donnefort <vincent.donnefort@arm.com>
> 
> device_release() is freeing the resources before calling the device
> specific release callback which is, in the case of devfreq, stopping
> the governor.
> 
> It is a problem as some governors are using the device resources. e.g.
> simpleondemand which is using the devfreq deferrable monitoring work. If it
> is not stopped before the resources are freed, it might lead to a use after
> free.
> 
> Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>
> Reviewed-by: John Einar Reitan <john.reitan@arm.com>
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>


Cheers,
MyungJoo