[PATCH 1/1] power: validate wakeup source before activating it.

[PATCH 1/1] power: validate wakeup source before activating it.

[Jin Qian]

A rogue wakeup source not registered in wakeup_sources list is not visible
from wakeup_sources_stats_show. Check if the wakeup source is registered
properly by looking at the timer struct.

Signed-off-by: Jin Qian <jinqian@android.com>
---
 drivers/base/power/wakeup.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index 7726200..7b5ad9a 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -351,6 +351,20 @@ int device_set_wakeup_enable(struct device *dev, bool enable)
 }
 EXPORT_SYMBOL_GPL(device_set_wakeup_enable);
 
+/**
+ * wakeup_source_not_registered - validate the given wakeup source.
+ * @ws: Wakeup source to be validated.
+ */
+static bool wakeup_source_not_registered(struct wakeup_source *ws)
+{
+	/*
+	 * Use timer struct to check if the given source is initialized
+	 * by wakeup_source_add.
+	 */
+	return ws->timer.function != pm_wakeup_timer_fn ||
+		   ws->timer.data != (unsigned long)ws;
+}
+
 /*
  * The functions below use the observation that each wakeup event starts a
  * period in which the system should not be suspended.  The moment this period
@@ -391,6 +405,10 @@ static void wakeup_source_activate(struct wakeup_source *ws)
 {
 	unsigned int cec;
 
+	if (WARN_ONCE(wakeup_source_not_registered(ws),
+			"unregistered wakeup source\n"))
+		return;
+
 	/*
 	 * active wakeup source should bring the system
 	 * out of PM_SUSPEND_FREEZE state
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH 1/1] power: validate wakeup source before activating it.

[Rafael J. Wysocki]

On Wednesday, May 06, 2015 03:26:56 PM Jin Qian wrote:
> A rogue wakeup source not registered in wakeup_sources list is not visible
> from wakeup_sources_stats_show. Check if the wakeup source is registered
> properly by looking at the timer struct.
> 
> Signed-off-by: Jin Qian <jinqian@android.com>
> ---
>  drivers/base/power/wakeup.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index 7726200..7b5ad9a 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -351,6 +351,20 @@ int device_set_wakeup_enable(struct device *dev, bool enable)
>  }
>  EXPORT_SYMBOL_GPL(device_set_wakeup_enable);
>  
> +/**
> + * wakeup_source_not_registered - validate the given wakeup source.
> + * @ws: Wakeup source to be validated.
> + */
> +static bool wakeup_source_not_registered(struct wakeup_source *ws)
> +{
> +	/*
> +	 * Use timer struct to check if the given source is initialized
> +	 * by wakeup_source_add.
> +	 */
> +	return ws->timer.function != pm_wakeup_timer_fn ||
> +		   ws->timer.data != (unsigned long)ws;
> +}
> +
>  /*
>   * The functions below use the observation that each wakeup event starts a
>   * period in which the system should not be suspended.  The moment this period
> @@ -391,6 +405,10 @@ static void wakeup_source_activate(struct wakeup_source *ws)
>  {
>  	unsigned int cec;
>  
> +	if (WARN_ONCE(wakeup_source_not_registered(ws),
> +			"unregistered wakeup source\n"))
> +		return;
> +
>  	/*
>  	 * active wakeup source should bring the system
>  	 * out of PM_SUSPEND_FREEZE state

Queued up for 4.2, thanks!


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/1] power: validate wakeup source before activating it.

[Jin Qian]

Hi Rafael,

The latest version is in [PATCHv3 1/3] power: validate wakeup source
before activating it. I changed WARN_ONCE back to WARN since if
multiple drivers activating uninitialized wakeup_sources, only the
first driver will by flagged. We lost alert for other drivers. The
warning should really happen during driver development. Hope this is
ok.

Thanks,
jin

On Thu, May 14, 2015 at 5:22 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, May 06, 2015 03:26:56 PM Jin Qian wrote:
>> A rogue wakeup source not registered in wakeup_sources list is not visible
>> from wakeup_sources_stats_show. Check if the wakeup source is registered
>> properly by looking at the timer struct.
>>
>> Signed-off-by: Jin Qian <jinqian@android.com>
>> ---
>>  drivers/base/power/wakeup.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
>> index 7726200..7b5ad9a 100644
>> --- a/drivers/base/power/wakeup.c
>> +++ b/drivers/base/power/wakeup.c
>> @@ -351,6 +351,20 @@ int device_set_wakeup_enable(struct device *dev, bool enable)
>>  }
>>  EXPORT_SYMBOL_GPL(device_set_wakeup_enable);
>>
>> +/**
>> + * wakeup_source_not_registered - validate the given wakeup source.
>> + * @ws: Wakeup source to be validated.
>> + */
>> +static bool wakeup_source_not_registered(struct wakeup_source *ws)
>> +{
>> +     /*
>> +      * Use timer struct to check if the given source is initialized
>> +      * by wakeup_source_add.
>> +      */
>> +     return ws->timer.function != pm_wakeup_timer_fn ||
>> +                ws->timer.data != (unsigned long)ws;
>> +}
>> +
>>  /*
>>   * The functions below use the observation that each wakeup event starts a
>>   * period in which the system should not be suspended.  The moment this period
>> @@ -391,6 +405,10 @@ static void wakeup_source_activate(struct wakeup_source *ws)
>>  {
>>       unsigned int cec;
>>
>> +     if (WARN_ONCE(wakeup_source_not_registered(ws),
>> +                     "unregistered wakeup source\n"))
>> +             return;
>> +
>>       /*
>>        * active wakeup source should bring the system
>>        * out of PM_SUSPEND_FREEZE state
>
> Queued up for 4.2, thanks!
>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/1] power: validate wakeup source before activating it.

[Rafael J. Wysocki]

On Friday, May 15, 2015 06:28:17 PM Jin Qian wrote:
> Hi Rafael,
> 
> The latest version is in [PATCHv3 1/3] power: validate wakeup source
> before activating it. I changed WARN_ONCE back to WARN since if
> multiple drivers activating uninitialized wakeup_sources, only the
> first driver will by flagged.

Which is OK.

If you're a user, you can't fix the problem by yourself and spamming the
logs with continued warnings doesn't help here.

If you're a developer, you'll fix the first driver and then see the
warning from the second one.

> We lost alert for other drivers. The warning should really happen during
> driver development. Hope this is ok.

First, I've applied your patch already.  Second, I really think that
WARN_ON_ONCE() is better here.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.