2015-05-06 18:52:05

by Jin Qian

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

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 <[email protected]>
---
drivers/base/power/wakeup.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index 7726200..aead738 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,11 @@ 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

2015-05-06 21:35:43

by Rafael J. Wysocki

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

On Wednesday, May 06, 2015 11:51:07 AM 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 <[email protected]>
> ---
> drivers/base/power/wakeup.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index 7726200..aead738 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,11 @@ 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;
> + }

The braces are not necessary and you can use format strings with WARN_ONCE()
too IIRC.

> +
> /*
> * active wakeup source should bring the system
> * out of PM_SUSPEND_FREEZE state
>

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

2015-05-06 22:11:52

by Jin Qian

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

Hi Rafael,

I kept braces since checkpatch.pl didn't report error or warning. The
if condition has two lines, a brace make the statement more clear I
think.

I removed the format string since ws->name can be random strings if
the wakeup_source is not registered (i.e. using prepare functions).
It's not very useful to print name if we have stack dump from
WARN_ONCE.

Thanks,
jin

On Wed, May 6, 2015 at 3:00 PM, Rafael J. Wysocki <[email protected]> wrote:
> On Wednesday, May 06, 2015 11:51:07 AM 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 <[email protected]>
>> ---
>> drivers/base/power/wakeup.c | 19 +++++++++++++++++++
>> 1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
>> index 7726200..aead738 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,11 @@ 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;
>> + }
>
> The braces are not necessary and you can use format strings with WARN_ONCE()
> too IIRC.
>
>> +
>> /*
>> * active wakeup source should bring the system
>> * out of PM_SUSPEND_FREEZE state
>>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

2015-05-06 22:24:55

by Jin Qian

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

I have to break the line since it's over 80 characters : ) I'll follow
CodingStyle and send a new patch.

Thanks,
jin

On Wed, May 6, 2015 at 3:39 PM, Rafael J. Wysocki <[email protected]> wrote:
> On Wednesday, May 06, 2015 03:11:48 PM Jin Qian wrote:
>> Hi Rafael,
>>
>> I kept braces since checkpatch.pl didn't report error or warning. The
>> if condition has two lines, a brace make the statement more clear I
>> think.
>
> Well, you don't have to break the "if" line. :-)
>
> As per CodingStyle, the braces should not be there.
>
>
>> I removed the format string since ws->name can be random strings if
>> the wakeup_source is not registered (i.e. using prepare functions).
>> It's not very useful to print name if we have stack dump from
>> WARN_ONCE.
>
> OK
>
>
>> On Wed, May 6, 2015 at 3:00 PM, Rafael J. Wysocki <[email protected]> wrote:
>> > On Wednesday, May 06, 2015 11:51:07 AM 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 <[email protected]>
>> >> ---
>> >> drivers/base/power/wakeup.c | 19 +++++++++++++++++++
>> >> 1 file changed, 19 insertions(+)
>> >>
>> >> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
>> >> index 7726200..aead738 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,11 @@ 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;
>> >> + }
>> >
>> > The braces are not necessary and you can use format strings with WARN_ONCE()
>> > too IIRC.
>> >
>> >> +
>> >> /*
>> >> * active wakeup source should bring the system
>> >> * out of PM_SUSPEND_FREEZE state
>> >>
>> >
>> > --
>> > I speak only for myself.
>> > Rafael J. Wysocki, Intel Open Source Technology Center.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

2015-05-06 22:14:09

by Rafael J. Wysocki

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

On Wednesday, May 06, 2015 03:11:48 PM Jin Qian wrote:
> Hi Rafael,
>
> I kept braces since checkpatch.pl didn't report error or warning. The
> if condition has two lines, a brace make the statement more clear I
> think.

Well, you don't have to break the "if" line. :-)

As per CodingStyle, the braces should not be there.


> I removed the format string since ws->name can be random strings if
> the wakeup_source is not registered (i.e. using prepare functions).
> It's not very useful to print name if we have stack dump from
> WARN_ONCE.

OK


> On Wed, May 6, 2015 at 3:00 PM, Rafael J. Wysocki <[email protected]> wrote:
> > On Wednesday, May 06, 2015 11:51:07 AM 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 <[email protected]>
> >> ---
> >> drivers/base/power/wakeup.c | 19 +++++++++++++++++++
> >> 1 file changed, 19 insertions(+)
> >>
> >> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> >> index 7726200..aead738 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,11 @@ 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;
> >> + }
> >
> > The braces are not necessary and you can use format strings with WARN_ONCE()
> > too IIRC.
> >
> >> +
> >> /*
> >> * active wakeup source should bring the system
> >> * out of PM_SUSPEND_FREEZE state
> >>
> >
> > --
> > I speak only for myself.
> > Rafael J. Wysocki, Intel Open Source Technology Center.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

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