* [PATCH] power: vexpress: fix corruption in notifier registration @ 2018-06-18 11:40 ` Sudeep Holla 0 siblings, 0 replies; 14+ messages in thread From: Sudeep Holla @ 2018-06-18 11:40 UTC (permalink / raw) To: Sebastian Reichel, linux-pm, linux-arm-kernel Cc: Lorenzo Pieralisi, Liviu Dudau, Sebastian Reichel, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH] power: vexpress: fix corruption in notifier registration @ 2018-06-18 11:40 ` Sudeep Holla 0 siblings, 0 replies; 14+ messages in thread From: Sudeep Holla @ 2018-06-18 11:40 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] power: vexpress: fix corruption in notifier registration 2018-06-18 11:40 ` Sudeep Holla @ 2018-06-18 14:56 ` Lorenzo Pieralisi -1 siblings, 0 replies; 14+ messages in thread From: Lorenzo Pieralisi @ 2018-06-18 14:56 UTC (permalink / raw) To: Sudeep Holla Cc: Sebastian Reichel, Liviu Dudau, Sebastian Reichel, linux-arm-kernel, linux-pm 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] power: vexpress: fix corruption in notifier registration @ 2018-06-18 14:56 ` Lorenzo Pieralisi 0 siblings, 0 replies; 14+ messages in thread From: Lorenzo Pieralisi @ 2018-06-18 14:56 UTC (permalink / raw) To: linux-arm-kernel 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] power: vexpress: fix corruption in notifier registration 2018-06-18 14:56 ` Lorenzo Pieralisi @ 2018-06-18 15:51 ` Sudeep Holla -1 siblings, 0 replies; 14+ messages in thread From: Sudeep Holla @ 2018-06-18 15:51 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: linux-pm, Liviu Dudau, Sebastian Reichel, Sudeep Holla, Sebastian Reichel, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] power: vexpress: fix corruption in notifier registration @ 2018-06-18 15:51 ` Sudeep Holla 0 siblings, 0 replies; 14+ messages in thread From: Sudeep Holla @ 2018-06-18 15:51 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] power: vexpress: fix corruption in notifier registration 2018-06-18 11:40 ` Sudeep Holla @ 2018-06-18 15:54 ` Sudeep Holla -1 siblings, 0 replies; 14+ messages in thread From: Sudeep Holla @ 2018-06-18 15:54 UTC (permalink / raw) To: Sebastian Reichel, linux-pm, linux-arm-kernel Cc: Lorenzo Pieralisi, Liviu Dudau, Sebastian Reichel, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2] power: vexpress: fix corruption in notifier registration @ 2018-06-18 15:54 ` Sudeep Holla 0 siblings, 0 replies; 14+ messages in thread From: Sudeep Holla @ 2018-06-18 15:54 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2] power: vexpress: fix corruption in notifier registration 2018-06-18 15:54 ` Sudeep Holla @ 2018-06-22 12:47 ` Sudeep Holla -1 siblings, 0 replies; 14+ messages in thread From: Sudeep Holla @ 2018-06-22 12:47 UTC (permalink / raw) To: Sebastian Reichel, linux-pm, linux-arm-kernel Cc: Lorenzo Pieralisi, Liviu Dudau, Sebastian Reichel, 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] power: vexpress: fix corruption in notifier registration @ 2018-06-22 12:47 ` Sudeep Holla 0 siblings, 0 replies; 14+ messages in thread From: Sudeep Holla @ 2018-06-22 12:47 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] power: vexpress: fix corruption in notifier registration 2018-06-18 15:54 ` Sudeep Holla @ 2018-07-06 11:34 ` Sudeep Holla -1 siblings, 0 replies; 14+ messages in thread From: Sudeep Holla @ 2018-07-06 11:34 UTC (permalink / raw) To: Sebastian Reichel, linux-pm, linux-arm-kernel, Sebastian Reichel Cc: Lorenzo Pieralisi, Liviu Dudau, 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] power: vexpress: fix corruption in notifier registration @ 2018-07-06 11:34 ` Sudeep Holla 0 siblings, 0 replies; 14+ messages in thread From: Sudeep Holla @ 2018-07-06 11:34 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] power: vexpress: fix corruption in notifier registration 2018-06-18 15:54 ` Sudeep Holla @ 2018-07-06 14:33 ` Sebastian Reichel -1 siblings, 0 replies; 14+ messages in thread From: Sebastian Reichel @ 2018-07-06 14:33 UTC (permalink / raw) To: Sudeep Holla; +Cc: Lorenzo Pieralisi, Liviu Dudau, linux-arm-kernel, linux-pm [-- 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] power: vexpress: fix corruption in notifier registration @ 2018-07-06 14:33 ` Sebastian Reichel 0 siblings, 0 replies; 14+ messages in thread From: Sebastian Reichel @ 2018-07-06 14:33 UTC (permalink / raw) To: linux-arm-kernel 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> ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-07-06 14:33 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-18 11:40 [PATCH] power: vexpress: fix corruption in notifier registration Sudeep Holla 2018-06-18 11:40 ` Sudeep Holla 2018-06-18 14:56 ` Lorenzo Pieralisi 2018-06-18 14:56 ` Lorenzo Pieralisi 2018-06-18 15:51 ` Sudeep Holla 2018-06-18 15:51 ` Sudeep Holla 2018-06-18 15:54 ` [PATCH v2] " Sudeep Holla 2018-06-18 15:54 ` Sudeep Holla 2018-06-22 12:47 ` Sudeep Holla 2018-06-22 12:47 ` Sudeep Holla 2018-07-06 11:34 ` Sudeep Holla 2018-07-06 11:34 ` Sudeep Holla 2018-07-06 14:33 ` Sebastian Reichel 2018-07-06 14:33 ` Sebastian Reichel
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.