[PATCH 1/3] firmware: arm_sdei: fix possible deadlock

[PATCH 1/3] firmware: arm_sdei: fix possible deadlock

[luanshi]

From: Liguang Zhang <zhangliguang@linux.alibaba.com>

We call sdei_reregister_event() with sdei_list_lock held but
_sdei_event_register() and sdei_event_destroy() also acquires
sdei_list_lock thus creating A-A deadlock.

Fixes: da351827240e ("firmware: arm_sdei: Add support for CPU and system power states")
Signed-off-by: Liguang Zhang <zhangliguang@linux.alibaba.com>
---
 drivers/firmware/arm_sdei.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index a479023..b122927 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -651,20 +651,19 @@ static int sdei_reregister_event(struct sdei_event *event)
 
 	lockdep_assert_held(&sdei_events_lock);
 
-	err = _sdei_event_register(event);
+	err = sdei_api_event_register(event->event_num,
+				       sdei_entry_point,
+				       event->registered,
+				       SDEI_EVENT_REGISTER_RM_ANY, 0);
 	if (err) {
 		pr_err("Failed to re-register event %u\n", event->event_num);
-		sdei_event_destroy(event);
+		list_del(&event->list);
+		kfree(event->registered);
 		return err;
 	}
 
-	if (event->reenable) {
-		if (event->type == SDEI_EVENT_TYPE_SHARED)
-			err = sdei_api_event_enable(event->event_num);
-		else
-			err = sdei_do_cross_call(_local_event_enable, event);
-	}
-
+	if (event->reenable)
+		err = sdei_api_event_enable(event->event_num);
 	if (err)
 		pr_err("Failed to re-enable event %u\n", event->event_num);
 
-- 
1.8.3.1

[PATCH 1/3] firmware: arm_sdei: fix possible deadlock

[luanshi]

From: Liguang Zhang <zhangliguang@linux.alibaba.com>

We call sdei_reregister_event() with sdei_list_lock held but
_sdei_event_register() and sdei_event_destroy() also acquires
sdei_list_lock thus creating A-A deadlock.

Fixes: da351827240e ("firmware: arm_sdei: Add support for CPU and system power states")
Signed-off-by: Liguang Zhang <zhangliguang@linux.alibaba.com>
---
 drivers/firmware/arm_sdei.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index a479023..b122927 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -651,20 +651,19 @@ static int sdei_reregister_event(struct sdei_event *event)
 
 	lockdep_assert_held(&sdei_events_lock);
 
-	err = _sdei_event_register(event);
+	err = sdei_api_event_register(event->event_num,
+				       sdei_entry_point,
+				       event->registered,
+				       SDEI_EVENT_REGISTER_RM_ANY, 0);
 	if (err) {
 		pr_err("Failed to re-register event %u\n", event->event_num);
-		sdei_event_destroy(event);
+		list_del(&event->list);
+		kfree(event->registered);
 		return err;
 	}
 
-	if (event->reenable) {
-		if (event->type == SDEI_EVENT_TYPE_SHARED)
-			err = sdei_api_event_enable(event->event_num);
-		else
-			err = sdei_do_cross_call(_local_event_enable, event);
-	}
-
+	if (event->reenable)
+		err = sdei_api_event_enable(event->event_num);
 	if (err)
 		pr_err("Failed to re-enable event %u\n", event->event_num);
 
-- 
1.8.3.1


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

[PATCH 2/3] firmware: arm_sdei: Removed multiple white lines.

[luanshi]

From: Liguang Zhang <zhangliguang@linux.alibaba.com>

Remove one unnecessary white line.

Signed-off-by: Liguang Zhang <zhangliguang@linux.alibaba.com>
---
 drivers/firmware/arm_sdei.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index b122927..0a366cf 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -595,7 +595,6 @@ static int _sdei_event_register(struct sdei_event *event)
 					       event->registered,
 					       SDEI_EVENT_REGISTER_RM_ANY, 0);
 
-
 	err = sdei_do_cross_call(_local_event_register, event);
 	if (err) {
 		spin_lock(&sdei_list_lock);
-- 
1.8.3.1

[PATCH 2/3] firmware: arm_sdei: Removed multiple white lines.

[luanshi]

From: Liguang Zhang <zhangliguang@linux.alibaba.com>

Remove one unnecessary white line.

Signed-off-by: Liguang Zhang <zhangliguang@linux.alibaba.com>
---
 drivers/firmware/arm_sdei.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index b122927..0a366cf 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -595,7 +595,6 @@ static int _sdei_event_register(struct sdei_event *event)
 					       event->registered,
 					       SDEI_EVENT_REGISTER_RM_ANY, 0);
 
-
 	err = sdei_do_cross_call(_local_event_register, event);
 	if (err) {
 		spin_lock(&sdei_list_lock);
-- 
1.8.3.1


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

[PATCH 3/3] firmware: arm_sdei: clean up sdei_event_create()

[luanshi]

From: Liguang Zhang <zhangliguang@linux.alibaba.com>

Function sdei_event_find() is always called in sdei_event_create(), but
it is already called in sdei_event_register(). So we should remove some
needless sdei_event_find() calls.

Signed-off-by: Liguang Zhang <zhangliguang@linux.alibaba.com>
---
 drivers/firmware/arm_sdei.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index 0a366cf..2c49907 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -267,15 +267,9 @@ static struct sdei_event *sdei_event_create(u32 event_num,
 		event->private_registered = regs;
 	}
 
-	if (sdei_event_find(event_num)) {
-		kfree(event->registered);
-		kfree(event);
-		event = ERR_PTR(-EBUSY);
-	} else {
-		spin_lock(&sdei_list_lock);
-		list_add(&event->list, &sdei_list);
-		spin_unlock(&sdei_list_lock);
-	}
+	spin_lock(&sdei_list_lock);
+	list_add(&event->list, &sdei_list);
+	spin_unlock(&sdei_list_lock);
 
 	return event;
 }
-- 
1.8.3.1

[PATCH 3/3] firmware: arm_sdei: clean up sdei_event_create()

[luanshi]

From: Liguang Zhang <zhangliguang@linux.alibaba.com>

Function sdei_event_find() is always called in sdei_event_create(), but
it is already called in sdei_event_register(). So we should remove some
needless sdei_event_find() calls.

Signed-off-by: Liguang Zhang <zhangliguang@linux.alibaba.com>
---
 drivers/firmware/arm_sdei.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index 0a366cf..2c49907 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -267,15 +267,9 @@ static struct sdei_event *sdei_event_create(u32 event_num,
 		event->private_registered = regs;
 	}
 
-	if (sdei_event_find(event_num)) {
-		kfree(event->registered);
-		kfree(event);
-		event = ERR_PTR(-EBUSY);
-	} else {
-		spin_lock(&sdei_list_lock);
-		list_add(&event->list, &sdei_list);
-		spin_unlock(&sdei_list_lock);
-	}
+	spin_lock(&sdei_list_lock);
+	list_add(&event->list, &sdei_list);
+	spin_unlock(&sdei_list_lock);
 
 	return event;
 }
-- 
1.8.3.1


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

Re: [PATCH 1/3] firmware: arm_sdei: fix possible deadlock

[James Morse]

Hi Luanshi!

On 23/12/2019 14:22, luanshi wrote:
> From: Liguang Zhang <zhangliguang@linux.alibaba.com>
> 
> We call sdei_reregister_event() with sdei_list_lock held but
> _sdei_event_register() and sdei_event_destroy() also acquires
> sdei_list_lock thus creating A-A deadlock.

Ooer. This was clearly never tested properly!
The hibernate support got plenty of testing, but it must have been with only private
events. Hibernate+SDEI with a side-order of cpuhp is a niche sport.


> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index a479023..b122927 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -651,20 +651,19 @@ static int sdei_reregister_event(struct sdei_event *event)
>  
>  	lockdep_assert_held(&sdei_events_lock);
>  
> -	err = _sdei_event_register(event);
> +	err = sdei_api_event_register(event->event_num,
> +				       sdei_entry_point,
> +				       event->registered,
> +				       SDEI_EVENT_REGISTER_RM_ANY, 0);

I don't like pushing these 'api' calls further out creating more of them...

The root of the problem is the reregister/reenable values are protected by the same lock
as the list, _sdei_event_register() needs to manipulate these, which it can't do from
something that is walking the list.

The list lock is a spin_lock() because the cpuhp callbacks happen too early for taking
mutexes, (fairly sure). Those callbacks don't hit this because they skip shared events.


As the simplest fix for stable, could we add another spin_lock inside struct sdei_event to
independently protect the reregister/renable values? This would always be taken last, and
removes the double-lock.


Was this from inspection, or is there some tool I should be running?!
(my testing obviously missed it)


Thanks,

James

Re: [PATCH 1/3] firmware: arm_sdei: fix possible deadlock

[James Morse]

Hi Luanshi!

On 23/12/2019 14:22, luanshi wrote:
> From: Liguang Zhang <zhangliguang@linux.alibaba.com>
> 
> We call sdei_reregister_event() with sdei_list_lock held but
> _sdei_event_register() and sdei_event_destroy() also acquires
> sdei_list_lock thus creating A-A deadlock.

Ooer. This was clearly never tested properly!
The hibernate support got plenty of testing, but it must have been with only private
events. Hibernate+SDEI with a side-order of cpuhp is a niche sport.


> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index a479023..b122927 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -651,20 +651,19 @@ static int sdei_reregister_event(struct sdei_event *event)
>  
>  	lockdep_assert_held(&sdei_events_lock);
>  
> -	err = _sdei_event_register(event);
> +	err = sdei_api_event_register(event->event_num,
> +				       sdei_entry_point,
> +				       event->registered,
> +				       SDEI_EVENT_REGISTER_RM_ANY, 0);

I don't like pushing these 'api' calls further out creating more of them...

The root of the problem is the reregister/reenable values are protected by the same lock
as the list, _sdei_event_register() needs to manipulate these, which it can't do from
something that is walking the list.

The list lock is a spin_lock() because the cpuhp callbacks happen too early for taking
mutexes, (fairly sure). Those callbacks don't hit this because they skip shared events.


As the simplest fix for stable, could we add another spin_lock inside struct sdei_event to
independently protect the reregister/renable values? This would always be taken last, and
removes the double-lock.


Was this from inspection, or is there some tool I should be running?!
(my testing obviously missed it)


Thanks,

James

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

Re: [PATCH 1/3] firmware: arm_sdei: fix possible deadlock

[乱石]

Hi James,

Sorry for the late reply, this problem was found by code review.


Thanks,

luanshi

在 2019/12/23 22:22, luanshi 写道:
> From: Liguang Zhang <zhangliguang@linux.alibaba.com>
>
> We call sdei_reregister_event() with sdei_list_lock held but
> _sdei_event_register() and sdei_event_destroy() also acquires
> sdei_list_lock thus creating A-A deadlock.
>
> Fixes: da351827240e ("firmware: arm_sdei: Add support for CPU and system power states")
> Signed-off-by: Liguang Zhang <zhangliguang@linux.alibaba.com>
> ---
>   drivers/firmware/arm_sdei.c | 17 ++++++++---------
>   1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index a479023..b122927 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -651,20 +651,19 @@ static int sdei_reregister_event(struct sdei_event *event)
>   
>   	lockdep_assert_held(&sdei_events_lock);
>   
> -	err = _sdei_event_register(event);
> +	err = sdei_api_event_register(event->event_num,
> +				       sdei_entry_point,
> +				       event->registered,
> +				       SDEI_EVENT_REGISTER_RM_ANY, 0);
>   	if (err) {
>   		pr_err("Failed to re-register event %u\n", event->event_num);
> -		sdei_event_destroy(event);
> +		list_del(&event->list);
> +		kfree(event->registered);
>   		return err;
>   	}
>   
> -	if (event->reenable) {
> -		if (event->type == SDEI_EVENT_TYPE_SHARED)
> -			err = sdei_api_event_enable(event->event_num);
> -		else
> -			err = sdei_do_cross_call(_local_event_enable, event);
> -	}
> -
> +	if (event->reenable)
> +		err = sdei_api_event_enable(event->event_num);
>   	if (err)
>   		pr_err("Failed to re-enable event %u\n", event->event_num);
>   

Re: [PATCH 1/3] firmware: arm_sdei: fix possible deadlock

[乱石]

Hi James,

Sorry for the late reply, this problem was found by code review.


Thanks,

luanshi

在 2019/12/23 22:22, luanshi 写道:
> From: Liguang Zhang <zhangliguang@linux.alibaba.com>
>
> We call sdei_reregister_event() with sdei_list_lock held but
> _sdei_event_register() and sdei_event_destroy() also acquires
> sdei_list_lock thus creating A-A deadlock.
>
> Fixes: da351827240e ("firmware: arm_sdei: Add support for CPU and system power states")
> Signed-off-by: Liguang Zhang <zhangliguang@linux.alibaba.com>
> ---
>   drivers/firmware/arm_sdei.c | 17 ++++++++---------
>   1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index a479023..b122927 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -651,20 +651,19 @@ static int sdei_reregister_event(struct sdei_event *event)
>   
>   	lockdep_assert_held(&sdei_events_lock);
>   
> -	err = _sdei_event_register(event);
> +	err = sdei_api_event_register(event->event_num,
> +				       sdei_entry_point,
> +				       event->registered,
> +				       SDEI_EVENT_REGISTER_RM_ANY, 0);
>   	if (err) {
>   		pr_err("Failed to re-register event %u\n", event->event_num);
> -		sdei_event_destroy(event);
> +		list_del(&event->list);
> +		kfree(event->registered);
>   		return err;
>   	}
>   
> -	if (event->reenable) {
> -		if (event->type == SDEI_EVENT_TYPE_SHARED)
> -			err = sdei_api_event_enable(event->event_num);
> -		else
> -			err = sdei_do_cross_call(_local_event_enable, event);
> -	}
> -
> +	if (event->reenable)
> +		err = sdei_api_event_enable(event->event_num);
>   	if (err)
>   		pr_err("Failed to re-enable event %u\n", event->event_num);
>   

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