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

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

[Sudeep Holla]

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
allows notifier 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 | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/power/reset/vexpress-poweroff.c b/drivers/power/reset/vexpress-poweroff.c
index 102f95a09460..cdc68eb06a91 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)
 {
@@ -96,13 +97,16 @@ static const struct of_device_id vexpress_reset_of_match[] = {
 
 static int _vexpress_register_restart_handler(struct device *dev)
 {
-	int err;
+	int err = 0;
 
 	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] power: vexpress: fix corruption in notifier registration

[Sudeep Holla]

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
allows notifier 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 | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/power/reset/vexpress-poweroff.c b/drivers/power/reset/vexpress-poweroff.c
index 102f95a09460..cdc68eb06a91 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)
 {
@@ -96,13 +97,16 @@ static const struct of_device_id vexpress_reset_of_match[] = {
 
 static int _vexpress_register_restart_handler(struct device *dev)
 {
-	int err;
+	int err = 0;
 
 	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

Re: [PATCH] power: vexpress: fix corruption in notifier registration

[Lorenzo Pieralisi]

On Mon, Jun 18, 2018 at 12:40:07PM +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
> allows notifier to be registered once only, however vexpress restart
> notifier can get registered twice.

Nit: I would say "notifier_chain_register() relies on notifiers to be
registered only once to work properly"; put it differently, it allows
notifiers to be registered twice (ie it does nothing to prevent it),
that's why we have this issue.

> 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 | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/power/reset/vexpress-poweroff.c b/drivers/power/reset/vexpress-poweroff.c
> index 102f95a09460..cdc68eb06a91 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)
>  {
> @@ -96,13 +97,16 @@ static const struct of_device_id vexpress_reset_of_match[] = {
>  
>  static int _vexpress_register_restart_handler(struct device *dev)
>  {
> -	int err;
> +	int err = 0;

Nit: I do not not see why you need to initialize err.

>  	vexpress_restart_device = dev;

It is unclear to me how the !vexpress_restart_device sentinel is
used while registering FUNC_RESET. It is unrelated to this patch
but if the registration below fails for FUNC_REBOOT can we end
up in a situation where vexpress_restart_device is initialized
with no restart handler registered ?

By looking at it I am not a big fan of the vexpress_restart_device
global variable it has been there since we merged this code but
its usage is a bit obscure.

Anyway, thanks for having a look and fixing the issue.

Lorenzo

> -	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] power: vexpress: fix corruption in notifier registration

[Lorenzo Pieralisi]

On Mon, Jun 18, 2018 at 12:40:07PM +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
> allows notifier to be registered once only, however vexpress restart
> notifier can get registered twice.

Nit: I would say "notifier_chain_register() relies on notifiers to be
registered only once to work properly"; put it differently, it allows
notifiers to be registered twice (ie it does nothing to prevent it),
that's why we have this issue.

> 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 | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/power/reset/vexpress-poweroff.c b/drivers/power/reset/vexpress-poweroff.c
> index 102f95a09460..cdc68eb06a91 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)
>  {
> @@ -96,13 +97,16 @@ static const struct of_device_id vexpress_reset_of_match[] = {
>  
>  static int _vexpress_register_restart_handler(struct device *dev)
>  {
> -	int err;
> +	int err = 0;

Nit: I do not not see why you need to initialize err.

>  	vexpress_restart_device = dev;

It is unclear to me how the !vexpress_restart_device sentinel is
used while registering FUNC_RESET. It is unrelated to this patch
but if the registration below fails for FUNC_REBOOT can we end
up in a situation where vexpress_restart_device is initialized
with no restart handler registered ?

By looking at it I am not a big fan of the vexpress_restart_device
global variable it has been there since we merged this code but
its usage is a bit obscure.

Anyway, thanks for having a look and fixing the issue.

Lorenzo

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

Re: [PATCH] power: vexpress: fix corruption in notifier registration

[Sudeep Holla]

On 18/06/18 15:56, Lorenzo Pieralisi wrote:
> On Mon, Jun 18, 2018 at 12:40:07PM +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
>> allows notifier to be registered once only, however vexpress restart
>> notifier can get registered twice.
> 
> Nit: I would say "notifier_chain_register() relies on notifiers to be
> registered only once to work properly"; put it differently, it allows
> notifiers to be registered twice (ie it does nothing to prevent it),
> that's why we have this issue.
> 

Indeed. I saw there's notifier_chain_cond_register too, not sure why
that's not used everywhere. I will change from allows to relies in the
wording.

>> 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 | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/power/reset/vexpress-poweroff.c b/drivers/power/reset/vexpress-poweroff.c
>> index 102f95a09460..cdc68eb06a91 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)
>>  {
>> @@ -96,13 +97,16 @@ static const struct of_device_id vexpress_reset_of_match[] = {
>>  
>>  static int _vexpress_register_restart_handler(struct device *dev)
>>  {
>> -	int err;
>> +	int err = 0;
> 
> Nit: I do not not see why you need to initialize err.
> 

Yes, I did notice this just after I sent it out. Left over from my
debugging, will remove it.

>>  	vexpress_restart_device = dev;
> 
> It is unclear to me how the !vexpress_restart_device sentinel is
> used while registering FUNC_RESET. It is unrelated to this patch
> but if the registration below fails for FUNC_REBOOT can we end
> up in a situation where vexpress_restart_device is initialized
> with no restart handler registered ?
> 

Yes, it took sometime for me to understand that. IIUC, FUNC_RESET is
optional, so it's probed first then !vexpress_restart_device will be
used. FUNC_REBOOT will override FUNC_RESET but not other way around.

> By looking at it I am not a big fan of the vexpress_restart_device
> global variable it has been there since we merged this code but
> its usage is a bit obscure.
> 

Yes, I was thinking of making access to it locked via mutex or some lock
but that won't fix the issue seen as it won't prevent FUNC_RESET being
probed first and then FUNC_REBOOT which will attempt to register
notifier again anyways.

-- 
Regards,
Sudeep

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

[Sudeep Holla]

On 18/06/18 15:56, Lorenzo Pieralisi wrote:
> On Mon, Jun 18, 2018 at 12:40:07PM +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
>> allows notifier to be registered once only, however vexpress restart
>> notifier can get registered twice.
> 
> Nit: I would say "notifier_chain_register() relies on notifiers to be
> registered only once to work properly"; put it differently, it allows
> notifiers to be registered twice (ie it does nothing to prevent it),
> that's why we have this issue.
> 

Indeed. I saw there's notifier_chain_cond_register too, not sure why
that's not used everywhere. I will change from allows to relies in the
wording.

>> 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 | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/power/reset/vexpress-poweroff.c b/drivers/power/reset/vexpress-poweroff.c
>> index 102f95a09460..cdc68eb06a91 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)
>>  {
>> @@ -96,13 +97,16 @@ static const struct of_device_id vexpress_reset_of_match[] = {
>>  
>>  static int _vexpress_register_restart_handler(struct device *dev)
>>  {
>> -	int err;
>> +	int err = 0;
> 
> Nit: I do not not see why you need to initialize err.
> 

Yes, I did notice this just after I sent it out. Left over from my
debugging, will remove it.

>>  	vexpress_restart_device = dev;
> 
> It is unclear to me how the !vexpress_restart_device sentinel is
> used while registering FUNC_RESET. It is unrelated to this patch
> but if the registration below fails for FUNC_REBOOT can we end
> up in a situation where vexpress_restart_device is initialized
> with no restart handler registered ?
> 

Yes, it took sometime for me to understand that. IIUC, FUNC_RESET is
optional, so it's probed first then !vexpress_restart_device will be
used. FUNC_REBOOT will override FUNC_RESET but not other way around.

> By looking at it I am not a big fan of the vexpress_restart_device
> global variable it has been there since we merged this code but
> its usage is a bit obscure.
> 

Yes, I was thinking of making access to it locked via mutex or some lock
but that won't fix the issue seen as it won't prevent FUNC_RESET being
probed first and then FUNC_REBOOT which will attempt to register
notifier again anyways.

-- 
Regards,
Sudeep

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

[Sudeep Holla]

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

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 v2] power: vexpress: fix corruption in notifier registration

[Sudeep Holla]

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

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

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

[Sudeep Holla]

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

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

[Sudeep Holla]

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

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

[Sudeep Holla]

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

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

[Sudeep Holla]

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

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

[Sebastian Reichel]

[-- Attachment #1.1: Type: text/plain, Size: 2926 bytes --]

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
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sebastian Reichel]

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
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180706/b5e8423f/attachment-0001.sig>