[v2] power: vexpress: fix corruption in notifier registration

Message ID 1529337272-11799-1-git-send-email-sudeep.holla@arm.com
State Accepted
Commit 09bebb1adb21ecd04adf7ccb3b06f73e3a851e93
Headers show
Series
  • [v2] power: vexpress: fix corruption in notifier registration
Related show

Commit Message

Sudeep Holla June 18, 2018, 3:54 p.m.
Vexpress platforms provide two different restart handlers: SYS_REBOOT
that restart the entire system, while DB_RESET only restarts the
daughter board containing the CPU. DB_RESET is overridden by SYS_REBOOT
if it exists.

notifier_chain_register used in register_restart_handler by design
relies on notifiers to be registered once only, however vexpress restart
notifier can get registered twice. When this happen it corrupts list
of notifiers, as result some notifiers can be not called on proper
event, traverse on list can be cycled forever, and second unregister
can access already freed memory.

So far, since this was the only restart handler in the system, no issue
was observed even if the same notifier was registered twice. However
commit 6c5c0d48b686 ("watchdog: sp805: add restart handler") added
support for SP805 restart handlers and since the system under test
contains two vexpress restart and two SP805 watchdog instances, it was
observed that during the boot traversing the restart handler list looped
forever as there's a cycle in that list resulting in boot hang.

This patch fixes the issues by ensuring that the notifier is installed
only once.

Cc: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

---
 drivers/power/reset/vexpress-poweroff.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

v1->v2:
	- Updated changelog wordings as suggested by Lorenzo
	- Dropped unnecessary error variable initialisation on stack

--
2.7.4

Comments

Sudeep Holla June 22, 2018, 12:47 p.m. | #1
Hi Sebastian,

On 18/06/18 16:54, Sudeep Holla wrote:
> Vexpress platforms provide two different restart handlers: SYS_REBOOT

> that restart the entire system, while DB_RESET only restarts the

> daughter board containing the CPU. DB_RESET is overridden by SYS_REBOOT

> if it exists.

> 

> notifier_chain_register used in register_restart_handler by design

> relies on notifiers to be registered once only, however vexpress restart

> notifier can get registered twice. When this happen it corrupts list

> of notifiers, as result some notifiers can be not called on proper

> event, traverse on list can be cycled forever, and second unregister

> can access already freed memory.

> 

> So far, since this was the only restart handler in the system, no issue

> was observed even if the same notifier was registered twice. However

> commit 6c5c0d48b686 ("watchdog: sp805: add restart handler") added

> support for SP805 restart handlers and since the system under test

> contains two vexpress restart and two SP805 watchdog instances, it was

> observed that during the boot traversing the restart handler list looped

> forever as there's a cycle in that list resulting in boot hang.

> 

> This patch fixes the issues by ensuring that the notifier is installed

> only once.

> 



Can you pick this up ? Without this patch, my Vexpress TC2 hangs in the
boot.

-- 
Regards,
Sudeep
Sudeep Holla July 6, 2018, 11:34 a.m. | #2
On 18/06/18 16:54, Sudeep Holla wrote:
> Vexpress platforms provide two different restart handlers: SYS_REBOOT

> that restart the entire system, while DB_RESET only restarts the

> daughter board containing the CPU. DB_RESET is overridden by SYS_REBOOT

> if it exists.

> 

> notifier_chain_register used in register_restart_handler by design

> relies on notifiers to be registered once only, however vexpress restart

> notifier can get registered twice. When this happen it corrupts list

> of notifiers, as result some notifiers can be not called on proper

> event, traverse on list can be cycled forever, and second unregister

> can access already freed memory.

> 

> So far, since this was the only restart handler in the system, no issue

> was observed even if the same notifier was registered twice. However

> commit 6c5c0d48b686 ("watchdog: sp805: add restart handler") added

> support for SP805 restart handlers and since the system under test

> contains two vexpress restart and two SP805 watchdog instances, it was

> observed that during the boot traversing the restart handler list looped

> forever as there's a cycle in that list resulting in boot hang.

> 

> This patch fixes the issues by ensuring that the notifier is installed

> only once.

> 

> Cc: Sebastian Reichel <sre@kernel.org>



Gentle Ping ! This needs to go as fix as my platform(TC2) fails to boot
without this.

-- 
Regards,
Sudeep
Sebastian Reichel July 6, 2018, 2:33 p.m. | #3
Hi,

On Mon, Jun 18, 2018 at 04:54:32PM +0100, Sudeep Holla wrote:
> Vexpress platforms provide two different restart handlers: SYS_REBOOT

> that restart the entire system, while DB_RESET only restarts the

> daughter board containing the CPU. DB_RESET is overridden by SYS_REBOOT

> if it exists.

> 

> notifier_chain_register used in register_restart_handler by design

> relies on notifiers to be registered once only, however vexpress restart

> notifier can get registered twice. When this happen it corrupts list

> of notifiers, as result some notifiers can be not called on proper

> event, traverse on list can be cycled forever, and second unregister

> can access already freed memory.

> 

> So far, since this was the only restart handler in the system, no issue

> was observed even if the same notifier was registered twice. However

> commit 6c5c0d48b686 ("watchdog: sp805: add restart handler") added

> support for SP805 restart handlers and since the system under test

> contains two vexpress restart and two SP805 watchdog instances, it was

> observed that during the boot traversing the restart handler list looped

> forever as there's a cycle in that list resulting in boot hang.

> 

> This patch fixes the issues by ensuring that the notifier is installed

> only once.

> 

> Cc: Sebastian Reichel <sre@kernel.org>

> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

> ---


Thanks, added Fixes: tag and queued to power-supply-fixes.

-- Sebastian

>  drivers/power/reset/vexpress-poweroff.c | 12 ++++++++----

>  1 file changed, 8 insertions(+), 4 deletions(-)

> 

> v1->v2:

> 	- Updated changelog wordings as suggested by Lorenzo

> 	- Dropped unnecessary error variable initialisation on stack

> 

> diff --git a/drivers/power/reset/vexpress-poweroff.c b/drivers/power/reset/vexpress-poweroff.c

> index 102f95a09460..e9e749f87517 100644

> --- a/drivers/power/reset/vexpress-poweroff.c

> +++ b/drivers/power/reset/vexpress-poweroff.c

> @@ -35,6 +35,7 @@ static void vexpress_reset_do(struct device *dev, const char *what)

>  }

> 

>  static struct device *vexpress_power_off_device;

> +static atomic_t vexpress_restart_nb_refcnt = ATOMIC_INIT(0);

> 

>  static void vexpress_power_off(void)

>  {

> @@ -99,10 +100,13 @@ static int _vexpress_register_restart_handler(struct device *dev)

>  	int err;

> 

>  	vexpress_restart_device = dev;

> -	err = register_restart_handler(&vexpress_restart_nb);

> -	if (err) {

> -		dev_err(dev, "cannot register restart handler (err=%d)\n", err);

> -		return err;

> +	if (atomic_inc_return(&vexpress_restart_nb_refcnt) == 1) {

> +		err = register_restart_handler(&vexpress_restart_nb);

> +		if (err) {

> +			dev_err(dev, "cannot register restart handler (err=%d)\n", err);

> +			atomic_dec(&vexpress_restart_nb_refcnt);

> +			return err;

> +		}

>  	}

>  	device_create_file(dev, &dev_attr_active);

> 

> --

> 2.7.4

>

Patch

diff --git a/drivers/power/reset/vexpress-poweroff.c b/drivers/power/reset/vexpress-poweroff.c
index 102f95a09460..e9e749f87517 100644
--- a/drivers/power/reset/vexpress-poweroff.c
+++ b/drivers/power/reset/vexpress-poweroff.c
@@ -35,6 +35,7 @@  static void vexpress_reset_do(struct device *dev, const char *what)
 }

 static struct device *vexpress_power_off_device;
+static atomic_t vexpress_restart_nb_refcnt = ATOMIC_INIT(0);

 static void vexpress_power_off(void)
 {
@@ -99,10 +100,13 @@  static int _vexpress_register_restart_handler(struct device *dev)
 	int err;

 	vexpress_restart_device = dev;
-	err = register_restart_handler(&vexpress_restart_nb);
-	if (err) {
-		dev_err(dev, "cannot register restart handler (err=%d)\n", err);
-		return err;
+	if (atomic_inc_return(&vexpress_restart_nb_refcnt) == 1) {
+		err = register_restart_handler(&vexpress_restart_nb);
+		if (err) {
+			dev_err(dev, "cannot register restart handler (err=%d)\n", err);
+			atomic_dec(&vexpress_restart_nb_refcnt);
+			return err;
+		}
 	}
 	device_create_file(dev, &dev_attr_active);