power: validate wakeup source before activating it.

PATCH Superseded Jin Qian 2 participants 5 messages May 6, 2015

Subsystems (4)

Driver core Suspend to RAM Hibernation Power management core

lore

Share & tools

lore

Jin Qian

author

2015-05-06 18:52 UTC #0 lore raw

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

drivers/base/power/wakeup.c +19 -0

index 7726200..aead738 100644
@@ -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

Signature · 1 line

2.2.0.rc0.207.ga3a616c

Rafael J. Wysocki

2015-05-06 21:35 UTC #1 lore raw

Quoted 35 lines · 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 <[email protected]>

---
 drivers/base/power/wakeup.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

drivers/base/power/wakeup.c +14 -0

index 7726200..aead738 100644
@@ -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

drivers/base/power/wakeup.c static void wakeup_source_activate(struct wakeup_source *ws) · line 411

Quoted 7 lines

drivers/base/power/wakeup.c +4 -0

@@ -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.

Quoted 5 lines

drivers/base/power/wakeup.c +1 -0

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

Signature · 2 lines

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

Jin Qian

author

2015-05-06 22:11 UTC #2 lore raw

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

Quoted 56 lines · Rafael J. Wysocki

Quoted 35 lines · 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 <[email protected]>
---
 drivers/base/power/wakeup.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
drivers/base/power/wakeup.c +14 -0
index 7726200..aead738 100644
@@ -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
drivers/base/power/wakeup.c static void wakeup_source_activate(struct wakeup_source *ws) · line 411
Quoted 7 lines
drivers/base/power/wakeup.c +4 -0
@@ -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.
drivers/base/power/wakeup.c static void wakeup_source_activate(struct wakeup_source *ws) · line 415
Quoted 5 lines
drivers/base/power/wakeup.c +1 -0
+
      /*
       * 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.

Rafael J. Wysocki

2015-05-06 22:14 UTC #3 lore raw

Quoted 5 lines · Jin Qian

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.

Quoted 4 lines

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.

Quoted 62 lines · Rafael J. Wysocki

Quoted 53 lines · Jin Qian
Quoted 35 lines
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(+)
drivers/base/power/wakeup.c +14 -0
index 7726200..aead738 100644
@@ -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
drivers/base/power/wakeup.c static void wakeup_source_activate(struct wakeup_source *ws) · line 411
Quoted 7 lines
drivers/base/power/wakeup.c +4 -0
@@ -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.
drivers/base/power/wakeup.c static void wakeup_source_activate(struct wakeup_source *ws) · line 415
Quoted 5 lines
drivers/base/power/wakeup.c +1 -0
+
      /*
       * active wakeup source should bring the system
       * out of PM_SUSPEND_FREEZE state
Quoted 3 lines
--
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/

Signature · 2 lines

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

Jin Qian

author

2015-05-06 22:24 UTC #4 lore raw

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

Thanks,
jin

Quoted 86 lines · Rafael J. Wysocki

Quoted 5 lines · Jin Qian
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.
Quoted 4 lines
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
Quoted 54 lines · Rafael J. Wysocki
Quoted 53 lines · Jin Qian
Quoted 35 lines
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(+)
drivers/base/power/wakeup.c +14 -0
index 7726200..aead738 100644
@@ -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
drivers/base/power/wakeup.c static void wakeup_source_activate(struct wakeup_source *ws) · line 411
Quoted 7 lines
drivers/base/power/wakeup.c +4 -0
@@ -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.
Quoted 5 lines
drivers/base/power/wakeup.c +1 -0
+
      /*
       * active wakeup source should bring the system
       * out of PM_SUSPEND_FREEZE state
Quoted 8 lines
Quoted 3 lines
--
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.