[PATCH 0/3] Propagate module notifier errors

[PATCH 0/3] Propagate module notifier errors

[Peter Zijlstra]

Hi all,

These patches came from the desire to propagate MODULE_STATE_COMING errors.
While looking at that I spotted fail with a number of module notifiers
themselves and with the whole notification business.

Please consider.

[PATCH 1/3] notifier: Fix broken error handling pattern

[Peter Zijlstra]

The current notifiers have the following error handling pattern all
over the place:

	int nr;

	ret = __foo_notifier_call_chain(&chain, val_up, v, -1, &nr);
	if (err & NOTIFIER_STOP_MASK)
		__foo_notifier_call_chain(&chain, val_down, v, nr-1, NULL)

And aside from the endless repetition thereof, it is broken. Consider
blocking notifiers; both calls take and drop the rwsem, this means
that the notifier list can change in between the two calls, making @nr
meaningless.

Fix this by replacing all the __foo_notifier_call_chain() functions
with foo_notifier_call_chain_error() that embeds the above patter, but
ensures it is inside a single lock region.

XXX: It is probably still broken for the RCU (atomic, src) users
(cpu_pm_notifier).

Note: software_resume() error handling was broken afaict.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Len Brown <len.brown@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Sam Protsenko <semen.protsenko@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Alexios Zavras <alexios.zavras@intel.com>
Cc: Allison Randal <allison@lohutok.net>
Cc: Vasily Averin <vvs@virtuozzo.com>
Cc: Todd Brandt <todd.e.brandt@linux.intel.com>
Cc: linux-kernel@vger.kernel.org
Cc: linux-pm@vger.kernel.org
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/notifier.h           |   17 ++-
 kernel/cpu_pm.c                    |   51 +++++------
 kernel/notifier.c                  |  159 +++++++++++++------------------------
 kernel/power/hibernate.c           |   26 ++----
 kernel/power/main.c                |    8 -
 kernel/power/power.h               |    3 
 kernel/power/suspend.c             |   14 +--
 kernel/power/user.c                |   14 ---
 tools/power/pm-graph/sleepgraph.py |    2 
 9 files changed, 118 insertions(+), 176 deletions(-)

--- a/include/linux/notifier.h
+++ b/include/linux/notifier.h
@@ -165,20 +165,21 @@ extern int srcu_notifier_chain_unregiste
 
 extern int atomic_notifier_call_chain(struct atomic_notifier_head *nh,
 		unsigned long val, void *v);
-extern int __atomic_notifier_call_chain(struct atomic_notifier_head *nh,
-	unsigned long val, void *v, int nr_to_call, int *nr_calls);
 extern int blocking_notifier_call_chain(struct blocking_notifier_head *nh,
 		unsigned long val, void *v);
-extern int __blocking_notifier_call_chain(struct blocking_notifier_head *nh,
-	unsigned long val, void *v, int nr_to_call, int *nr_calls);
 extern int raw_notifier_call_chain(struct raw_notifier_head *nh,
 		unsigned long val, void *v);
-extern int __raw_notifier_call_chain(struct raw_notifier_head *nh,
-	unsigned long val, void *v, int nr_to_call, int *nr_calls);
 extern int srcu_notifier_call_chain(struct srcu_notifier_head *nh,
 		unsigned long val, void *v);
-extern int __srcu_notifier_call_chain(struct srcu_notifier_head *nh,
-	unsigned long val, void *v, int nr_to_call, int *nr_calls);
+
+extern int atomic_notifier_call_chain_error(struct atomic_notifier_head *nh,
+		unsigned long val_up, unsigned long val_down, void *v);
+extern int blocking_notifier_call_chain_error(struct blocking_notifier_head *nh,
+		unsigned long val_up, unsigned long val_down, void *v);
+extern int raw_notifier_call_chain_error(struct raw_notifier_head *nh,
+		unsigned long val_up, unsigned long val_down, void *v);
+extern int srcu_notifier_call_chain_error(struct srcu_notifier_head *nh,
+		unsigned long val_up, unsigned long val_down, void *v);
 
 #define NOTIFY_DONE		0x0000		/* Don't care */
 #define NOTIFY_OK		0x0001		/* Suits me */
--- a/kernel/cpu_pm.c
+++ b/kernel/cpu_pm.c
@@ -15,7 +15,7 @@
 
 static ATOMIC_NOTIFIER_HEAD(cpu_pm_notifier_chain);
 
-static int cpu_pm_notify(enum cpu_pm_event event, int nr_to_call, int *nr_calls)
+static int cpu_pm_notify(enum cpu_pm_event event)
 {
 	int ret;
 
@@ -25,8 +25,23 @@ static int cpu_pm_notify(enum cpu_pm_eve
 	 * RCU know this.
 	 */
 	rcu_irq_enter_irqson();
-	ret = __atomic_notifier_call_chain(&cpu_pm_notifier_chain, event, NULL,
-		nr_to_call, nr_calls);
+	ret = atomic_notifier_call_chain(&cpu_pm_notifier_chain, event, NULL);
+	rcu_irq_exit_irqson();
+
+	return notifier_to_errno(ret);
+}
+
+static int cpu_pm_notify_error(enum cpu_pm_event event_up, enum cpu_pm_event event_down)
+{
+	int ret;
+
+	/*
+	 * __atomic_notifier_call_chain has a RCU read critical section, which
+	 * could be disfunctional in cpu idle. Copy RCU_NONIDLE code to let
+	 * RCU know this.
+	 */
+	rcu_irq_enter_irqson();
+	ret = atomic_notifier_call_chain_error(&cpu_pm_notifier_chain, event_up, event_down, NULL);
 	rcu_irq_exit_irqson();
 
 	return notifier_to_errno(ret);
@@ -80,18 +95,7 @@ EXPORT_SYMBOL_GPL(cpu_pm_unregister_noti
  */
 int cpu_pm_enter(void)
 {
-	int nr_calls;
-	int ret = 0;
-
-	ret = cpu_pm_notify(CPU_PM_ENTER, -1, &nr_calls);
-	if (ret)
-		/*
-		 * Inform listeners (nr_calls - 1) about failure of CPU PM
-		 * PM entry who are notified earlier to prepare for it.
-		 */
-		cpu_pm_notify(CPU_PM_ENTER_FAILED, nr_calls - 1, NULL);
-
-	return ret;
+	return cpu_pm_notify_error(CPU_PM_ENTER, CPU_PM_ENTER_FAILED);
 }
 EXPORT_SYMBOL_GPL(cpu_pm_enter);
 
@@ -109,7 +113,7 @@ EXPORT_SYMBOL_GPL(cpu_pm_enter);
  */
 int cpu_pm_exit(void)
 {
-	return cpu_pm_notify(CPU_PM_EXIT, -1, NULL);
+	return cpu_pm_notify(CPU_PM_EXIT);
 }
 EXPORT_SYMBOL_GPL(cpu_pm_exit);
 
@@ -131,18 +135,7 @@ EXPORT_SYMBOL_GPL(cpu_pm_exit);
  */
 int cpu_cluster_pm_enter(void)
 {
-	int nr_calls;
-	int ret = 0;
-
-	ret = cpu_pm_notify(CPU_CLUSTER_PM_ENTER, -1, &nr_calls);
-	if (ret)
-		/*
-		 * Inform listeners (nr_calls - 1) about failure of CPU cluster
-		 * PM entry who are notified earlier to prepare for it.
-		 */
-		cpu_pm_notify(CPU_CLUSTER_PM_ENTER_FAILED, nr_calls - 1, NULL);
-
-	return ret;
+	return cpu_pm_notify_enter(CPU_CLUSTER_PM_ENTER, CPU_CLUSTER_ENTER_FAILED);
 }
 EXPORT_SYMBOL_GPL(cpu_cluster_pm_enter);
 
@@ -163,7 +156,7 @@ EXPORT_SYMBOL_GPL(cpu_cluster_pm_enter);
  */
 int cpu_cluster_pm_exit(void)
 {
-	return cpu_pm_notify(CPU_CLUSTER_PM_EXIT, -1, NULL);
+	return cpu_pm_notify(CPU_CLUSTER_PM_EXIT);
 }
 EXPORT_SYMBOL_GPL(cpu_cluster_pm_exit);
 
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -106,6 +106,19 @@ static int notifier_call_chain(struct no
 }
 NOKPROBE_SYMBOL(notifier_call_chain);
 
+static int notifier_call_chain_error(struct notifier_block **nl,
+				     unsigned long val_up, unsigned long val_down,
+				     void *v)
+{
+	int ret, nr = 0;
+
+	ret = notifier_call_chain(nl, val_up, v, -1, &nr);
+	if (ret & NOTIFY_STOP_MASK)
+		notifier_call_chain(nl, val_down, v, nr-1, NULL);
+
+	return ret;
+}
+
 /*
  *	Atomic notifier chain routines.  Registration and unregistration
  *	use a spinlock, and call_chain is synchronized by RCU (no locks).
@@ -156,43 +169,30 @@ int atomic_notifier_chain_unregister(str
 }
 EXPORT_SYMBOL_GPL(atomic_notifier_chain_unregister);
 
-/**
- *	__atomic_notifier_call_chain - Call functions in an atomic notifier chain
- *	@nh: Pointer to head of the atomic notifier chain
- *	@val: Value passed unmodified to notifier function
- *	@v: Pointer passed unmodified to notifier function
- *	@nr_to_call: See the comment for notifier_call_chain.
- *	@nr_calls: See the comment for notifier_call_chain.
- *
- *	Calls each function in a notifier chain in turn.  The functions
- *	run in an atomic context, so they must not block.
- *	This routine uses RCU to synchronize with changes to the chain.
- *
- *	If the return value of the notifier can be and'ed
- *	with %NOTIFY_STOP_MASK then atomic_notifier_call_chain()
- *	will return immediately, with the return value of
- *	the notifier function which halted execution.
- *	Otherwise the return value is the return value
- *	of the last notifier function called.
- */
-int __atomic_notifier_call_chain(struct atomic_notifier_head *nh,
-				 unsigned long val, void *v,
-				 int nr_to_call, int *nr_calls)
+int atomic_notifier_call_chain_error(struct atomic_notifier_head *nh,
+		unsigned long val_up, unsigned long val_down, void *v)
 {
 	int ret;
 
 	rcu_read_lock();
-	ret = notifier_call_chain(&nh->head, val, v, nr_to_call, nr_calls);
+	ret = notifier_call_chain_error(&nh->head, val_up, val_down, v);
 	rcu_read_unlock();
+
 	return ret;
 }
-EXPORT_SYMBOL_GPL(__atomic_notifier_call_chain);
-NOKPROBE_SYMBOL(__atomic_notifier_call_chain);
+EXPORT_SYMBOL_GPL(atomic_notifier_call_chain_error);
+NOKPROBE_SYMBOL(atomic_notifier_call_chain_error);
 
 int atomic_notifier_call_chain(struct atomic_notifier_head *nh,
 			       unsigned long val, void *v)
 {
-	return __atomic_notifier_call_chain(nh, val, v, -1, NULL);
+	int ret;
+
+	rcu_read_lock();
+	ret = notifier_call_chain(&nh->head, val, v, -1, NULL);
+	rcu_read_unlock();
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(atomic_notifier_call_chain);
 NOKPROBE_SYMBOL(atomic_notifier_call_chain);
@@ -285,27 +285,8 @@ int blocking_notifier_chain_unregister(s
 }
 EXPORT_SYMBOL_GPL(blocking_notifier_chain_unregister);
 
-/**
- *	__blocking_notifier_call_chain - Call functions in a blocking notifier chain
- *	@nh: Pointer to head of the blocking notifier chain
- *	@val: Value passed unmodified to notifier function
- *	@v: Pointer passed unmodified to notifier function
- *	@nr_to_call: See comment for notifier_call_chain.
- *	@nr_calls: See comment for notifier_call_chain.
- *
- *	Calls each function in a notifier chain in turn.  The functions
- *	run in a process context, so they are allowed to block.
- *
- *	If the return value of the notifier can be and'ed
- *	with %NOTIFY_STOP_MASK then blocking_notifier_call_chain()
- *	will return immediately, with the return value of
- *	the notifier function which halted execution.
- *	Otherwise the return value is the return value
- *	of the last notifier function called.
- */
-int __blocking_notifier_call_chain(struct blocking_notifier_head *nh,
-				   unsigned long val, void *v,
-				   int nr_to_call, int *nr_calls)
+int blocking_notifier_call_chain_error(struct blocking_notifier_head *nh,
+		unsigned long val_up, unsigned long val_down, void *v)
 {
 	int ret = NOTIFY_DONE;
 
@@ -316,18 +297,29 @@ int __blocking_notifier_call_chain(struc
 	 */
 	if (rcu_access_pointer(nh->head)) {
 		down_read(&nh->rwsem);
-		ret = notifier_call_chain(&nh->head, val, v, nr_to_call,
-					nr_calls);
+		ret = notifier_call_chain_error(&nh->head, val_up, val_down, v);
 		up_read(&nh->rwsem);
 	}
 	return ret;
 }
-EXPORT_SYMBOL_GPL(__blocking_notifier_call_chain);
+EXPORT_SYMBOL_GPL(blocking_notifier_call_chain_error);
 
 int blocking_notifier_call_chain(struct blocking_notifier_head *nh,
 		unsigned long val, void *v)
 {
-	return __blocking_notifier_call_chain(nh, val, v, -1, NULL);
+	int ret = NOTIFY_DONE;
+
+	/*
+	 * We check the head outside the lock, but if this access is
+	 * racy then it does not matter what the result of the test
+	 * is, we re-check the list after having taken the lock anyway:
+	 */
+	if (rcu_access_pointer(nh->head)) {
+		down_read(&nh->rwsem);
+		ret = notifier_call_chain(&nh->head, val, v, -1, NULL);
+		up_read(&nh->rwsem);
+	}
+	return ret;
 }
 EXPORT_SYMBOL_GPL(blocking_notifier_call_chain);
 
@@ -370,37 +362,17 @@ int raw_notifier_chain_unregister(struct
 }
 EXPORT_SYMBOL_GPL(raw_notifier_chain_unregister);
 
-/**
- *	__raw_notifier_call_chain - Call functions in a raw notifier chain
- *	@nh: Pointer to head of the raw notifier chain
- *	@val: Value passed unmodified to notifier function
- *	@v: Pointer passed unmodified to notifier function
- *	@nr_to_call: See comment for notifier_call_chain.
- *	@nr_calls: See comment for notifier_call_chain
- *
- *	Calls each function in a notifier chain in turn.  The functions
- *	run in an undefined context.
- *	All locking must be provided by the caller.
- *
- *	If the return value of the notifier can be and'ed
- *	with %NOTIFY_STOP_MASK then raw_notifier_call_chain()
- *	will return immediately, with the return value of
- *	the notifier function which halted execution.
- *	Otherwise the return value is the return value
- *	of the last notifier function called.
- */
-int __raw_notifier_call_chain(struct raw_notifier_head *nh,
-			      unsigned long val, void *v,
-			      int nr_to_call, int *nr_calls)
+int raw_notifier_call_chain_error(struct raw_notifier_head *nh,
+		unsigned long val_up, unsigned long val_down, void *v)
 {
-	return notifier_call_chain(&nh->head, val, v, nr_to_call, nr_calls);
+	return notifier_call_chain_error(&nh->head, val_up, val_down, v);
 }
-EXPORT_SYMBOL_GPL(__raw_notifier_call_chain);
+EXPORT_SYMBOL_GPL(raw_notifier_call_chain_error);
 
 int raw_notifier_call_chain(struct raw_notifier_head *nh,
 		unsigned long val, void *v)
 {
-	return __raw_notifier_call_chain(nh, val, v, -1, NULL);
+	return notifier_call_chain(&nh->head, val, v, -1, NULL);
 }
 EXPORT_SYMBOL_GPL(raw_notifier_call_chain);
 
@@ -471,42 +443,29 @@ int srcu_notifier_chain_unregister(struc
 }
 EXPORT_SYMBOL_GPL(srcu_notifier_chain_unregister);
 
-/**
- *	__srcu_notifier_call_chain - Call functions in an SRCU notifier chain
- *	@nh: Pointer to head of the SRCU notifier chain
- *	@val: Value passed unmodified to notifier function
- *	@v: Pointer passed unmodified to notifier function
- *	@nr_to_call: See comment for notifier_call_chain.
- *	@nr_calls: See comment for notifier_call_chain
- *
- *	Calls each function in a notifier chain in turn.  The functions
- *	run in a process context, so they are allowed to block.
- *
- *	If the return value of the notifier can be and'ed
- *	with %NOTIFY_STOP_MASK then srcu_notifier_call_chain()
- *	will return immediately, with the return value of
- *	the notifier function which halted execution.
- *	Otherwise the return value is the return value
- *	of the last notifier function called.
- */
-int __srcu_notifier_call_chain(struct srcu_notifier_head *nh,
-			       unsigned long val, void *v,
-			       int nr_to_call, int *nr_calls)
+int srcu_notifier_call_chain_error(struct srcu_notifier_head *nh,
+		unsigned long val_up, unsigned long val_down, void *v)
 {
 	int ret;
 	int idx;
 
 	idx = srcu_read_lock(&nh->srcu);
-	ret = notifier_call_chain(&nh->head, val, v, nr_to_call, nr_calls);
+	ret = notifier_call_chain_error(&nh->head, val_up, val_down, v);
 	srcu_read_unlock(&nh->srcu, idx);
 	return ret;
 }
-EXPORT_SYMBOL_GPL(__srcu_notifier_call_chain);
+EXPORT_SYMBOL_GPL(srcu_notifier_call_chain_error);
 
 int srcu_notifier_call_chain(struct srcu_notifier_head *nh,
 		unsigned long val, void *v)
 {
-	return __srcu_notifier_call_chain(nh, val, v, -1, NULL);
+	int ret;
+	int idx;
+
+	idx = srcu_read_lock(&nh->srcu);
+	ret = notifier_call_chain(&nh->head, val, v, -1, NULL);
+	srcu_read_unlock(&nh->srcu, idx);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(srcu_notifier_call_chain);
 
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -693,8 +693,8 @@ static int load_image_and_restore(void)
  */
 int hibernate(void)
 {
-	int error, nr_calls = 0;
 	bool snapshot_test = false;
+	int error;
 
 	if (!hibernation_available()) {
 		pm_pr_dbg("Hibernation not available.\n");
@@ -710,11 +710,9 @@ int hibernate(void)
 
 	pr_info("hibernation entry\n");
 	pm_prepare_console();
-	error = __pm_notifier_call_chain(PM_HIBERNATION_PREPARE, -1, &nr_calls);
-	if (error) {
-		nr_calls--;
-		goto Exit;
-	}
+	error = pm_notifier_call_chain_error(PM_HIBERNATION_PREPARE, PM_POST_HIBERNATION);
+	if (error)
+		goto Restore;
 
 	ksys_sync_helper();
 
@@ -772,7 +770,8 @@ int hibernate(void)
 	/* Don't bother checking whether freezer_test_done is true */
 	freezer_test_done = false;
  Exit:
-	__pm_notifier_call_chain(PM_POST_HIBERNATION, nr_calls, NULL);
+	pm_notifier_call_chain(PM_POST_HIBERNATION);
+ Restore:
 	pm_restore_console();
 	atomic_inc(&snapshot_device_available);
  Unlock:
@@ -800,7 +799,7 @@ int hibernate(void)
  */
 static int software_resume(void)
 {
-	int error, nr_calls = 0;
+	int error;
 
 	/*
 	 * If the user said "noresume".. bail out early.
@@ -887,11 +886,9 @@ static int software_resume(void)
 
 	pr_info("resume from hibernation\n");
 	pm_prepare_console();
-	error = __pm_notifier_call_chain(PM_RESTORE_PREPARE, -1, &nr_calls);
-	if (error) {
-		nr_calls--;
-		goto Close_Finish;
-	}
+	error = pm_notifier_call_chain_error(PM_RESTORE_PREPARE, PM_POST_RESTORE);
+	if (error)
+		goto Restore;
 
 	pm_pr_dbg("Preparing processes for restore.\n");
 	error = freeze_processes();
@@ -900,7 +897,8 @@ static int software_resume(void)
 	error = load_image_and_restore();
 	thaw_processes();
  Finish:
-	__pm_notifier_call_chain(PM_POST_RESTORE, nr_calls, NULL);
+	pm_notifier_call_chain(PM_POST_RESTORE);
+ Restore:
 	pm_restore_console();
 	pr_info("resume from hibernation failed (%d)\n", error);
 	atomic_inc(&snapshot_device_available);
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -79,18 +79,18 @@ int unregister_pm_notifier(struct notifi
 }
 EXPORT_SYMBOL_GPL(unregister_pm_notifier);
 
-int __pm_notifier_call_chain(unsigned long val, int nr_to_call, int *nr_calls)
+int pm_notifier_call_chain_error(unsigned long val_up, unsigned long val_down)
 {
 	int ret;
 
-	ret = __blocking_notifier_call_chain(&pm_chain_head, val, NULL,
-						nr_to_call, nr_calls);
+	ret = blocking_notifier_call_chain_error(&pm_chain_head, val_up, val_down, NULL);
 
 	return notifier_to_errno(ret);
 }
+
 int pm_notifier_call_chain(unsigned long val)
 {
-	return __pm_notifier_call_chain(val, -1, NULL);
+	return blocking_notifier_call_chain(&pm_chain_head, val, NULL);
 }
 
 /* If set, devices may be suspended and resumed asynchronously. */
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -212,8 +212,7 @@ static inline void suspend_test_finish(c
 
 #ifdef CONFIG_PM_SLEEP
 /* kernel/power/main.c */
-extern int __pm_notifier_call_chain(unsigned long val, int nr_to_call,
-				    int *nr_calls);
+extern int pm_notifier_call_chain_error(unsigned long val_up, unsigned long val_down);
 extern int pm_notifier_call_chain(unsigned long val);
 #endif
 
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -352,18 +352,16 @@ static int suspend_test(int level)
  */
 static int suspend_prepare(suspend_state_t state)
 {
-	int error, nr_calls = 0;
+	int error;
 
 	if (!sleep_state_supported(state))
 		return -EPERM;
 
 	pm_prepare_console();
 
-	error = __pm_notifier_call_chain(PM_SUSPEND_PREPARE, -1, &nr_calls);
-	if (error) {
-		nr_calls--;
-		goto Finish;
-	}
+	error = pm_notifier_call_chain_error(PM_SUSPEND_PREPARE, PM_POST_SUSPEND);
+	if (error)
+		goto Restore;
 
 	trace_suspend_resume(TPS("freeze_processes"), 0, true);
 	error = suspend_freeze_processes();
@@ -373,8 +371,8 @@ static int suspend_prepare(suspend_state
 
 	suspend_stats.failed_freeze++;
 	dpm_save_failed_step(SUSPEND_FREEZE);
- Finish:
-	__pm_notifier_call_chain(PM_POST_SUSPEND, nr_calls, NULL);
+	pm_notifier_call_chain(PM_POST_SUSPEND);
+ Restore:
 	pm_restore_console();
 	return error;
 }
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -44,7 +44,7 @@ atomic_t snapshot_device_available = ATO
 static int snapshot_open(struct inode *inode, struct file *filp)
 {
 	struct snapshot_data *data;
-	int error, nr_calls = 0;
+	int error;
 
 	if (!hibernation_available())
 		return -EPERM;
@@ -71,9 +71,7 @@ static int snapshot_open(struct inode *i
 			swap_type_of(swsusp_resume_device, 0, NULL) : -1;
 		data->mode = O_RDONLY;
 		data->free_bitmaps = false;
-		error = __pm_notifier_call_chain(PM_HIBERNATION_PREPARE, -1, &nr_calls);
-		if (error)
-			__pm_notifier_call_chain(PM_POST_HIBERNATION, --nr_calls, NULL);
+		error = pm_notifier_call_chain_error(PM_HIBERNATION_PREPARE, PM_POST_HIBERNATION);
 	} else {
 		/*
 		 * Resuming.  We may need to wait for the image device to
@@ -83,15 +81,11 @@ static int snapshot_open(struct inode *i
 
 		data->swap = -1;
 		data->mode = O_WRONLY;
-		error = __pm_notifier_call_chain(PM_RESTORE_PREPARE, -1, &nr_calls);
+		error = pm_notifier_call_chain_error(PM_RESTORE_PREPARE, PM_POST_RESTORE);
 		if (!error) {
 			error = create_basic_memory_bitmaps();
 			data->free_bitmaps = !error;
-		} else
-			nr_calls--;
-
-		if (error)
-			__pm_notifier_call_chain(PM_POST_RESTORE, nr_calls, NULL);
+		}
 	}
 	if (error)
 		atomic_inc(&snapshot_device_available);
--- a/tools/power/pm-graph/sleepgraph.py
+++ b/tools/power/pm-graph/sleepgraph.py
@@ -146,7 +146,7 @@ from subprocess import call, Popen, PIPE
 	tracefuncs = {
 		'sys_sync': {},
 		'ksys_sync': {},
-		'__pm_notifier_call_chain': {},
+		'pm_notifier_call_chain_error': {},
 		'pm_prepare_console': {},
 		'pm_notifier_call_chain': {},
 		'freeze_processes': {},

Re: [PATCH 1/3] notifier: Fix broken error handling pattern

[Josh Poimboeuf]

On Mon, Jun 24, 2019 at 11:18:44AM +0200, Peter Zijlstra wrote:
> The current notifiers have the following error handling pattern all
> over the place:
> 
> 	int nr;
> 
> 	ret = __foo_notifier_call_chain(&chain, val_up, v, -1, &nr);
> 	if (err & NOTIFIER_STOP_MASK)

s/err/ret/

> 		__foo_notifier_call_chain(&chain, val_down, v, nr-1, NULL)
> 
> And aside from the endless repetition thereof, it is broken. Consider
> blocking notifiers; both calls take and drop the rwsem, this means
> that the notifier list can change in between the two calls, making @nr
> meaningless.
> 
> Fix this by replacing all the __foo_notifier_call_chain() functions
> with foo_notifier_call_chain_error() that embeds the above patter, but
> ensures it is inside a single lock region.

The name "notifier_call_chain_error()" seems confusing, it almost sounds
like it's notifying an error code.  Then again, I can't really think of
a more reasonably succinct name.

> @@ -25,8 +25,23 @@ static int cpu_pm_notify(enum cpu_pm_eve
>  	 * RCU know this.
>  	 */
>  	rcu_irq_enter_irqson();
> -	ret = __atomic_notifier_call_chain(&cpu_pm_notifier_chain, event, NULL,
> -		nr_to_call, nr_calls);
> +	ret = atomic_notifier_call_chain(&cpu_pm_notifier_chain, event, NULL);
> +	rcu_irq_exit_irqson();
> +
> +	return notifier_to_errno(ret);
> +}
> +
> +static int cpu_pm_notify_error(enum cpu_pm_event event_up, enum cpu_pm_event event_down)
> +{
> +	int ret;
> +
> +	/*
> +	 * __atomic_notifier_call_chain has a RCU read critical section, which

__atomic_notifier_call_chain() no longer exists.

> +	 * could be disfunctional in cpu idle. Copy RCU_NONIDLE code to let

"dysfunctional"

> @@ -156,43 +169,30 @@ int atomic_notifier_chain_unregister(str
>  }
>  EXPORT_SYMBOL_GPL(atomic_notifier_chain_unregister);
>  
> -/**
> - *	__atomic_notifier_call_chain - Call functions in an atomic notifier chain
> - *	@nh: Pointer to head of the atomic notifier chain
> - *	@val: Value passed unmodified to notifier function
> - *	@v: Pointer passed unmodified to notifier function
> - *	@nr_to_call: See the comment for notifier_call_chain.
> - *	@nr_calls: See the comment for notifier_call_chain.
> - *
> - *	Calls each function in a notifier chain in turn.  The functions
> - *	run in an atomic context, so they must not block.
> - *	This routine uses RCU to synchronize with changes to the chain.
> - *
> - *	If the return value of the notifier can be and'ed
> - *	with %NOTIFY_STOP_MASK then atomic_notifier_call_chain()
> - *	will return immediately, with the return value of
> - *	the notifier function which halted execution.
> - *	Otherwise the return value is the return value
> - *	of the last notifier function called.
> - */

Why remove the useful comment?

Ditto for the blocking, raw, srcu, comments.

-- 
Josh

Re: [PATCH 1/3] notifier: Fix broken error handling pattern

[Peter Zijlstra]

On Mon, Jun 24, 2019 at 05:21:07PM -0500, Josh Poimboeuf wrote:
> On Mon, Jun 24, 2019 at 11:18:44AM +0200, Peter Zijlstra wrote:
> > The current notifiers have the following error handling pattern all
> > over the place:
> > 
> > 	int nr;
> > 
> > 	ret = __foo_notifier_call_chain(&chain, val_up, v, -1, &nr);
> > 	if (err & NOTIFIER_STOP_MASK)
> 
> s/err/ret/

-ETOOWARM :-)

> > 		__foo_notifier_call_chain(&chain, val_down, v, nr-1, NULL)
> > 
> > And aside from the endless repetition thereof, it is broken. Consider
> > blocking notifiers; both calls take and drop the rwsem, this means
> > that the notifier list can change in between the two calls, making @nr
> > meaningless.
> > 
> > Fix this by replacing all the __foo_notifier_call_chain() functions
> > with foo_notifier_call_chain_error() that embeds the above patter, but
> > ensures it is inside a single lock region.
> 
> The name "notifier_call_chain_error()" seems confusing, it almost sounds
> like it's notifying an error code.  Then again, I can't really think of
> a more reasonably succinct name.

I;m not attached to the name; I very much ran out of ideas and just
typed something.

> > @@ -25,8 +25,23 @@ static int cpu_pm_notify(enum cpu_pm_eve
> >  	 * RCU know this.
> >  	 */
> >  	rcu_irq_enter_irqson();
> > -	ret = __atomic_notifier_call_chain(&cpu_pm_notifier_chain, event, NULL,
> > -		nr_to_call, nr_calls);
> > +	ret = atomic_notifier_call_chain(&cpu_pm_notifier_chain, event, NULL);
> > +	rcu_irq_exit_irqson();
> > +
> > +	return notifier_to_errno(ret);
> > +}
> > +
> > +static int cpu_pm_notify_error(enum cpu_pm_event event_up, enum cpu_pm_event event_down)
> > +{
> > +	int ret;
> > +
> > +	/*
> > +	 * __atomic_notifier_call_chain has a RCU read critical section, which
> 
> __atomic_notifier_call_chain() no longer exists.
> 
> > +	 * could be disfunctional in cpu idle. Copy RCU_NONIDLE code to let
> 
> "dysfunctional"

That's copy paste, I don't think I've read the comment, my bad.

> > @@ -156,43 +169,30 @@ int atomic_notifier_chain_unregister(str
> >  }
> >  EXPORT_SYMBOL_GPL(atomic_notifier_chain_unregister);
> >  
> > -/**
> > - *	__atomic_notifier_call_chain - Call functions in an atomic notifier chain
> > - *	@nh: Pointer to head of the atomic notifier chain
> > - *	@val: Value passed unmodified to notifier function
> > - *	@v: Pointer passed unmodified to notifier function
> > - *	@nr_to_call: See the comment for notifier_call_chain.
> > - *	@nr_calls: See the comment for notifier_call_chain.
> > - *
> > - *	Calls each function in a notifier chain in turn.  The functions
> > - *	run in an atomic context, so they must not block.
> > - *	This routine uses RCU to synchronize with changes to the chain.
> > - *
> > - *	If the return value of the notifier can be and'ed
> > - *	with %NOTIFY_STOP_MASK then atomic_notifier_call_chain()
> > - *	will return immediately, with the return value of
> > - *	the notifier function which halted execution.
> > - *	Otherwise the return value is the return value
> > - *	of the last notifier function called.
> > - */
> 
> Why remove the useful comment?

Because I delete the whole function ?

Re: [PATCH 1/3] notifier: Fix broken error handling pattern

[Josh Poimboeuf]

On Tue, Jun 25, 2019 at 09:38:21AM +0200, Peter Zijlstra wrote:
> > > @@ -156,43 +169,30 @@ int atomic_notifier_chain_unregister(str
> > >  }
> > >  EXPORT_SYMBOL_GPL(atomic_notifier_chain_unregister);
> > >  
> > > -/**
> > > - *	__atomic_notifier_call_chain - Call functions in an atomic notifier chain
> > > - *	@nh: Pointer to head of the atomic notifier chain
> > > - *	@val: Value passed unmodified to notifier function
> > > - *	@v: Pointer passed unmodified to notifier function
> > > - *	@nr_to_call: See the comment for notifier_call_chain.
> > > - *	@nr_calls: See the comment for notifier_call_chain.
> > > - *
> > > - *	Calls each function in a notifier chain in turn.  The functions
> > > - *	run in an atomic context, so they must not block.
> > > - *	This routine uses RCU to synchronize with changes to the chain.
> > > - *
> > > - *	If the return value of the notifier can be and'ed
> > > - *	with %NOTIFY_STOP_MASK then atomic_notifier_call_chain()
> > > - *	will return immediately, with the return value of
> > > - *	the notifier function which halted execution.
> > > - *	Otherwise the return value is the return value
> > > - *	of the last notifier function called.
> > > - */
> > 
> > Why remove the useful comment?
> 
> Because I delete the whole function ?

I viewed it as more of a rename... Regardless would the comment not
still be useful for the non-double-underscore version of the function?

-- 
Josh

Re: [PATCH 1/3] notifier: Fix broken error handling pattern

[Peter Zijlstra]

On Tue, Jun 25, 2019 at 07:13:34AM -0500, Josh Poimboeuf wrote:
> On Tue, Jun 25, 2019 at 09:38:21AM +0200, Peter Zijlstra wrote:
> > > > @@ -156,43 +169,30 @@ int atomic_notifier_chain_unregister(str
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(atomic_notifier_chain_unregister);
> > > >  
> > > > -/**
> > > > - *	__atomic_notifier_call_chain - Call functions in an atomic notifier chain
> > > > - *	@nh: Pointer to head of the atomic notifier chain
> > > > - *	@val: Value passed unmodified to notifier function
> > > > - *	@v: Pointer passed unmodified to notifier function
> > > > - *	@nr_to_call: See the comment for notifier_call_chain.
> > > > - *	@nr_calls: See the comment for notifier_call_chain.
> > > > - *
> > > > - *	Calls each function in a notifier chain in turn.  The functions
> > > > - *	run in an atomic context, so they must not block.
> > > > - *	This routine uses RCU to synchronize with changes to the chain.
> > > > - *
> > > > - *	If the return value of the notifier can be and'ed
> > > > - *	with %NOTIFY_STOP_MASK then atomic_notifier_call_chain()
> > > > - *	will return immediately, with the return value of
> > > > - *	the notifier function which halted execution.
> > > > - *	Otherwise the return value is the return value
> > > > - *	of the last notifier function called.
> > > > - */
> > > 
> > > Why remove the useful comment?
> > 
> > Because I delete the whole function ?
> 
> I viewed it as more of a rename... Regardless would the comment not
> still be useful for the non-double-underscore version of the function?

I never got that far, I just deleted the whole thing without reading it.
But yes, with a few tweaks it should apply to the normal function.

[PATCH 2/3] module: Fix up module_notifier return values.

[Peter Zijlstra]

While auditing all module notifiers I noticed a whole bunch of fail
wrt the return value. Notifiers have a 'special' return semantics.

Cc: Robert Richter <rric@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Martin KaFai Lau <kafai@fb.com>
Cc: Song Liu <songliubraving@fb.com>
Cc: Yonghong Song <yhs@fb.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
Cc: "Joel Fernandes (Google)" <joel@joelfernandes.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: oprofile-list@lists.sf.net
Cc: linux-kernel@vger.kernel.org
Cc: netdev@vger.kernel.org
Cc: bpf@vger.kernel.org
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 drivers/oprofile/buffer_sync.c |    4 ++--
 kernel/module.c                |    9 +++++----
 kernel/trace/bpf_trace.c       |    8 ++++++--
 kernel/trace/trace.c           |    2 +-
 kernel/trace/trace_events.c    |    2 +-
 kernel/trace/trace_printk.c    |    4 ++--
 kernel/tracepoint.c            |    2 +-
 7 files changed, 18 insertions(+), 13 deletions(-)

--- a/drivers/oprofile/buffer_sync.c
+++ b/drivers/oprofile/buffer_sync.c
@@ -116,7 +116,7 @@ module_load_notify(struct notifier_block
 {
 #ifdef CONFIG_MODULES
 	if (val != MODULE_STATE_COMING)
-		return 0;
+		return NOTIFY_DONE;
 
 	/* FIXME: should we process all CPU buffers ? */
 	mutex_lock(&buffer_mutex);
@@ -124,7 +124,7 @@ module_load_notify(struct notifier_block
 	add_event_entry(MODULE_LOADED_CODE);
 	mutex_unlock(&buffer_mutex);
 #endif
-	return 0;
+	return NOTIFY_OK;
 }
 
 
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1302,10 +1302,11 @@ static int bpf_event_notify(struct notif
 {
 	struct bpf_trace_module *btm, *tmp;
 	struct module *mod = module;
+	int ret = 0;
 
 	if (mod->num_bpf_raw_events == 0 ||
 	    (op != MODULE_STATE_COMING && op != MODULE_STATE_GOING))
-		return 0;
+		goto out;
 
 	mutex_lock(&bpf_module_mutex);
 
@@ -1315,6 +1316,8 @@ static int bpf_event_notify(struct notif
 		if (btm) {
 			btm->module = module;
 			list_add(&btm->list, &bpf_trace_modules);
+		} else {
+			ret = -ENOMEM;
 		}
 		break;
 	case MODULE_STATE_GOING:
@@ -1330,7 +1333,8 @@ static int bpf_event_notify(struct notif
 
 	mutex_unlock(&bpf_module_mutex);
 
-	return 0;
+out:
+	return notifier_from_errno(ret);
 }
 
 static struct notifier_block bpf_module_nb = {
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -8685,7 +8685,7 @@ static int trace_module_notify(struct no
 		break;
 	}
 
-	return 0;
+	return NOTIFY_OK;
 }
 
 static struct notifier_block trace_module_nb = {
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -2450,7 +2450,7 @@ static int trace_module_notify(struct no
 	mutex_unlock(&trace_types_lock);
 	mutex_unlock(&event_mutex);
 
-	return 0;
+	return NOTIFY_OK;
 }
 
 static struct notifier_block trace_module_nb = {
--- a/kernel/trace/trace_printk.c
+++ b/kernel/trace/trace_printk.c
@@ -95,7 +95,7 @@ static int module_trace_bprintk_format_n
 		if (val == MODULE_STATE_COMING)
 			hold_module_trace_bprintk_format(start, end);
 	}
-	return 0;
+	return NOTIFY_OK;
 }
 
 /*
@@ -173,7 +173,7 @@ __init static int
 module_trace_bprintk_format_notify(struct notifier_block *self,
 		unsigned long val, void *data)
 {
-	return 0;
+	return NOTIFY_OK;
 }
 static inline const char **
 find_next_mod_format(int start_index, void *v, const char **fmt, loff_t *pos)
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -538,7 +538,7 @@ static int tracepoint_module_notify(stru
 	case MODULE_STATE_UNFORMED:
 		break;
 	}
-	return ret;
+	return notifier_from_errno(ret);
 }
 
 static struct notifier_block tracepoint_module_nb = {

Re: [PATCH 2/3] module: Fix up module_notifier return values.

[Mathieu Desnoyers]

----- On Jun 24, 2019, at 5:18 AM, Peter Zijlstra peterz@infradead.org wrote:

> While auditing all module notifiers I noticed a whole bunch of fail
> wrt the return value. Notifiers have a 'special' return semantics.
> 
> Cc: Robert Richter <rric@kernel.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Martin KaFai Lau <kafai@fb.com>
> Cc: Song Liu <songliubraving@fb.com>
> Cc: Yonghong Song <yhs@fb.com>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
> Cc: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: oprofile-list@lists.sf.net
> Cc: linux-kernel@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Cc: bpf@vger.kernel.org
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Thanks Peter for looking into this, especially considering your
endless love for kernel modules! ;)

It's not directly related to your changes, but I notice that
kernel/trace/trace_printk.c:hold_module_trace_bprintk_format()
appears to leak memory. Am I missing something ?

With respect to your changes:
Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

I have a similar erroneous module notifier return value pattern
in lttng-modules as well. I'll go fix it right away. CCing
Frank Eigler from SystemTAP which AFAIK use a copy of
lttng-tracepoint.c in their project, which should be fixed
as well. I'm pasting the lttng-modules fix below.

Thanks!

Mathieu

--

commit 5eac9d146a7d947f0f314c4f7103c92cbccaeaf3
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date:   Mon Jun 24 09:43:45 2019 -0400

    Fix: lttng-tracepoint module notifier should return NOTIFY_OK
    
    Module notifiers should return NOTIFY_OK on success rather than the
    value 0. The return value 0 does not seem to have any ill side-effects
    in the notifier chain caller, but it is preferable to respect the API
    requirements in case this changes in the future.
    
    Notifiers can encapsulate a negative errno value with
    notifier_from_errno(), but this is not needed by the LTTng tracepoint
    notifier.
    
    The approach taken in this notifier is to just print a console warning
    on error, because tracing failure should not prevent loading a module.
    So we definitely do not want to stop notifier iteration. Returning
    an error without stopping iteration is not really that useful, because
    only the return value of the last callback is returned to notifier chain
    caller.
    
    Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

diff --git a/lttng-tracepoint.c b/lttng-tracepoint.c
index bbb2c7a4..8298b397 100644
--- a/lttng-tracepoint.c
+++ b/lttng-tracepoint.c
@@ -256,7 +256,7 @@ int lttng_tracepoint_coming(struct tp_module *tp_mod)
                }
        }
        mutex_unlock(&lttng_tracepoint_mutex);
-       return 0;
+       return NOTIFY_OK;
 }
 
 static


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH 2/3] module: Fix up module_notifier return values.

[Joel Fernandes]

On Mon, Jun 24, 2019 at 10:01:04AM -0400, Mathieu Desnoyers wrote:
> ----- On Jun 24, 2019, at 5:18 AM, Peter Zijlstra peterz@infradead.org wrote:
> 
> > While auditing all module notifiers I noticed a whole bunch of fail
> > wrt the return value. Notifiers have a 'special' return semantics.
> > 
> > Cc: Robert Richter <rric@kernel.org>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Cc: Martin KaFai Lau <kafai@fb.com>
> > Cc: Song Liu <songliubraving@fb.com>
> > Cc: Yonghong Song <yhs@fb.com>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
> > Cc: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: oprofile-list@lists.sf.net
> > Cc: linux-kernel@vger.kernel.org
> > Cc: netdev@vger.kernel.org
> > Cc: bpf@vger.kernel.org
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> 
> Thanks Peter for looking into this, especially considering your
> endless love for kernel modules! ;)
> 
> It's not directly related to your changes, but I notice that
> kernel/trace/trace_printk.c:hold_module_trace_bprintk_format()
> appears to leak memory. Am I missing something ?

Could you elaborate? Do you mean there is no MODULE_STATE_GOING notifier
check? If that's what you mean then I agree, there should be some place
where the format structures are freed when the module is unloaded no?

> 
> With respect to your changes:
> Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Looks good to me too.

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Could we CC stable so that the fix is propagated to older kernels?

thanks,

 - Joel


> I have a similar erroneous module notifier return value pattern
> in lttng-modules as well. I'll go fix it right away. CCing
> Frank Eigler from SystemTAP which AFAIK use a copy of
> lttng-tracepoint.c in their project, which should be fixed
> as well. I'm pasting the lttng-modules fix below.
> 
> Thanks!
> 
> Mathieu
> 
> --
> 
> commit 5eac9d146a7d947f0f314c4f7103c92cbccaeaf3
> Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Date:   Mon Jun 24 09:43:45 2019 -0400
> 
>     Fix: lttng-tracepoint module notifier should return NOTIFY_OK
>     
>     Module notifiers should return NOTIFY_OK on success rather than the
>     value 0. The return value 0 does not seem to have any ill side-effects
>     in the notifier chain caller, but it is preferable to respect the API
>     requirements in case this changes in the future.
>     
>     Notifiers can encapsulate a negative errno value with
>     notifier_from_errno(), but this is not needed by the LTTng tracepoint
>     notifier.
>     
>     The approach taken in this notifier is to just print a console warning
>     on error, because tracing failure should not prevent loading a module.
>     So we definitely do not want to stop notifier iteration. Returning
>     an error without stopping iteration is not really that useful, because
>     only the return value of the last callback is returned to notifier chain
>     caller.
>     
>     Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> 
> diff --git a/lttng-tracepoint.c b/lttng-tracepoint.c
> index bbb2c7a4..8298b397 100644
> --- a/lttng-tracepoint.c
> +++ b/lttng-tracepoint.c
> @@ -256,7 +256,7 @@ int lttng_tracepoint_coming(struct tp_module *tp_mod)
>                 }
>         }
>         mutex_unlock(&lttng_tracepoint_mutex);
> -       return 0;
> +       return NOTIFY_OK;
>  }
>  
>  static
> 
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com

Re: [PATCH 2/3] module: Fix up module_notifier return values.

[Mathieu Desnoyers]

----- On Jun 24, 2019, at 11:52 AM, Joel Fernandes, Google joel@joelfernandes.org wrote:

> On Mon, Jun 24, 2019 at 10:01:04AM -0400, Mathieu Desnoyers wrote:
>> ----- On Jun 24, 2019, at 5:18 AM, Peter Zijlstra peterz@infradead.org wrote:
>> 
>> > While auditing all module notifiers I noticed a whole bunch of fail
>> > wrt the return value. Notifiers have a 'special' return semantics.
>> > 
>> > Cc: Robert Richter <rric@kernel.org>
>> > Cc: Steven Rostedt <rostedt@goodmis.org>
>> > Cc: Ingo Molnar <mingo@redhat.com>
>> > Cc: Alexei Starovoitov <ast@kernel.org>
>> > Cc: Daniel Borkmann <daniel@iogearbox.net>
>> > Cc: Martin KaFai Lau <kafai@fb.com>
>> > Cc: Song Liu <songliubraving@fb.com>
>> > Cc: Yonghong Song <yhs@fb.com>
>> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>> > Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
>> > Cc: "Joel Fernandes (Google)" <joel@joelfernandes.org>
>> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> > Cc: Thomas Gleixner <tglx@linutronix.de>
>> > Cc: oprofile-list@lists.sf.net
>> > Cc: linux-kernel@vger.kernel.org
>> > Cc: netdev@vger.kernel.org
>> > Cc: bpf@vger.kernel.org
>> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>> 
>> Thanks Peter for looking into this, especially considering your
>> endless love for kernel modules! ;)
>> 
>> It's not directly related to your changes, but I notice that
>> kernel/trace/trace_printk.c:hold_module_trace_bprintk_format()
>> appears to leak memory. Am I missing something ?
> 
> Could you elaborate? Do you mean there is no MODULE_STATE_GOING notifier
> check? If that's what you mean then I agree, there should be some place
> where the format structures are freed when the module is unloaded no?

Yes, the lack of GOING notifier is worrying considering that GOING
performs memory allocation.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH 2/3] module: Fix up module_notifier return values.

[Frank Ch. Eigler]

Hi -

> > While auditing all module notifiers I noticed a whole bunch of fail
> > wrt the return value. Notifiers have a 'special' return semantics.

From peterz's comments, the patches, it's not obvious to me how one is
to choose between 0 (NOTIFY_DONE) and 1 (NOTIFY_OK) in the case of a
routine success.

> [...]
> I have a similar erroneous module notifier return value pattern
> in lttng-modules as well. I'll go fix it right away. CCing
> Frank Eigler from SystemTAP which AFAIK use a copy of
> lttng-tracepoint.c in their project, which should be fixed
> as well. I'm pasting the lttng-modules fix below.

Sure, following suit.  Thanks.

- FChE

Re: [PATCH 2/3] module: Fix up module_notifier return values.

[Peter Zijlstra]

On Mon, Jun 24, 2019 at 04:58:10PM -0400, Frank Ch. Eigler wrote:
> Hi -
> 
> > > While auditing all module notifiers I noticed a whole bunch of fail
> > > wrt the return value. Notifiers have a 'special' return semantics.
> 
> From peterz's comments, the patches, it's not obvious to me how one is
> to choose between 0 (NOTIFY_DONE) and 1 (NOTIFY_OK) in the case of a
> routine success.

I'm not sure either; what I think I choice was:

 - if I want to completely ignore the callback, use DONE (per the
   "Don't care" comment).

 - if we finished the notifier without error, use OK or
   notifier_from_errno(0).

But yes, its a bit of a shit interface.

Re: [PATCH 2/3] module: Fix up module_notifier return values.

[Robert Richter]

On 25.06.19 09:42:14, Peter Zijlstra wrote:
> On Mon, Jun 24, 2019 at 04:58:10PM -0400, Frank Ch. Eigler wrote:

> > From peterz's comments, the patches, it's not obvious to me how one is
> > to choose between 0 (NOTIFY_DONE) and 1 (NOTIFY_OK) in the case of a
> > routine success.
> 
> I'm not sure either; what I think I choice was:
> 
>  - if I want to completely ignore the callback, use DONE (per the
>    "Don't care" comment).
> 
>  - if we finished the notifier without error, use OK or
>    notifier_from_errno(0).
> 
> But yes, its a bit of a shit interface.

It looks like it was rarely used in earlier kernels as some sort of
error detection for the notifier calls, e.g.:

1da177e4c3f41524e886b7f1b8a0c1fc7321cac2:kernel/profile.c-int profile_handoff_task(struct task_struct * task)
1da177e4c3f41524e886b7f1b8a0c1fc7321cac2:kernel/profile.c-{
1da177e4c3f41524e886b7f1b8a0c1fc7321cac2:kernel/profile.c-      int ret;
1da177e4c3f41524e886b7f1b8a0c1fc7321cac2:kernel/profile.c-      read_lock(&handoff_lock);
1da177e4c3f41524e886b7f1b8a0c1fc7321cac2:kernel/profile.c-      ret = notifier_call_chain(&task_free_notifier, 0, task);
1da177e4c3f41524e886b7f1b8a0c1fc7321cac2:kernel/profile.c-      read_unlock(&handoff_lock);
1da177e4c3f41524e886b7f1b8a0c1fc7321cac2:kernel/profile.c:      return (ret == NOTIFY_OK) ? 1 : 0;
1da177e4c3f41524e886b7f1b8a0c1fc7321cac2:kernel/profile.c-}

So NOTIFY_OK was used to state there is no error, while NOTIFY_DONE
says the notifier was executed and there might have been errors. The
caller may distinguish the results then.

-Robert

Re: [PATCH 2/3] module: Fix up module_notifier return values.

[Robert Richter]

On 24.06.19 11:18:45, Peter Zijlstra wrote:
> While auditing all module notifiers I noticed a whole bunch of fail
> wrt the return value. Notifiers have a 'special' return semantics.
> 
> Cc: Robert Richter <rric@kernel.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Martin KaFai Lau <kafai@fb.com>
> Cc: Song Liu <songliubraving@fb.com>
> Cc: Yonghong Song <yhs@fb.com>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
> Cc: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: oprofile-list@lists.sf.net
> Cc: linux-kernel@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Cc: bpf@vger.kernel.org
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  drivers/oprofile/buffer_sync.c |    4 ++--
>  kernel/module.c                |    9 +++++----
>  kernel/trace/bpf_trace.c       |    8 ++++++--
>  kernel/trace/trace.c           |    2 +-
>  kernel/trace/trace_events.c    |    2 +-
>  kernel/trace/trace_printk.c    |    4 ++--
>  kernel/tracepoint.c            |    2 +-
>  7 files changed, 18 insertions(+), 13 deletions(-)

Reviewed-by: Robert Richter <rric@kernel.org>

[PATCH 3/3] module: Properly propagate MODULE_STATE_COMING failure

[Peter Zijlstra]

Now that notifiers got unbroken; use the proper interface to handle
notifier errors and propagate them.

There were already MODULE_STATE_COMING notifiers that failed; notably:

 - jump_label_module_notifier()
 - tracepoint_module_notify()
 - bpf_event_notify()

By propagating this error, we fix those users.

Cc: Jessica Yu <jeyu@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Martin KaFai Lau <kafai@fb.com>
Cc: Song Liu <songliubraving@fb.com>
Cc: Yonghong Song <yhs@fb.com>
Cc: linux-kernel@vger.kernel.org
Cc: netdev@vger.kernel.org
Cc: bpf@vger.kernel.org
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/module.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3643,9 +3643,10 @@ static int prepare_coming_module(struct
 	if (err)
 		return err;
 
-	blocking_notifier_call_chain(&module_notify_list,
-				     MODULE_STATE_COMING, mod);
-	return 0;
+	err = blocking_notifier_call_chain_error(&module_notify_list,
+			MODULE_STATE_COMING, MODULE_STATE_GOING, mod);
+
+	return notifier_to_errno(err);
 }
 
 static int unknown_module_param_cb(char *param, char *val, const char *modname,

Re: [PATCH 3/3] module: Properly propagate MODULE_STATE_COMING failure

[Peter Zijlstra]

On Mon, Jun 24, 2019 at 11:18:46AM +0200, Peter Zijlstra wrote:
> Now that notifiers got unbroken; use the proper interface to handle
> notifier errors and propagate them.
> 
> There were already MODULE_STATE_COMING notifiers that failed; notably:
> 
>  - jump_label_module_notifier()
>  - tracepoint_module_notify()
>  - bpf_event_notify()
> 
> By propagating this error, we fix those users.
> 
> Cc: Jessica Yu <jeyu@kernel.org>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Martin KaFai Lau <kafai@fb.com>
> Cc: Song Liu <songliubraving@fb.com>
> Cc: Yonghong Song <yhs@fb.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Cc: bpf@vger.kernel.org
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/module.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3643,9 +3643,10 @@ static int prepare_coming_module(struct
>  	if (err)
>  		return err;
>  
> -	blocking_notifier_call_chain(&module_notify_list,
> -				     MODULE_STATE_COMING, mod);
> -	return 0;
> +	err = blocking_notifier_call_chain_error(&module_notify_list,
> +			MODULE_STATE_COMING, MODULE_STATE_GOING, mod);
> +
> +	return notifier_to_errno(err);
>  }

Argh, I messed up that klp thing again :/