2024-02-02 18:39:47

by Ira Weiny

Subject: [PATCH] acpi/ghes: Prevent sleeping with spinlock held

Smatch caught that cxl_cper_post_event() is called with a spinlock held
or preemption disabled.[1] There is no need for the callback to sleep.

Replace the RW semaphore with a RW lock.

A static call was considered but ARM does not select HAVE_STATIC_CALL
and in that case setting the function pointer uses a RW semaphore.

[1] https://lore.kernel.org/all/[email protected]/

Reported-by: Dan Carpenter <[email protected]>
Signed-off-by: Ira Weiny <[email protected]>
---
drivers/acpi/apei/ghes.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 7b7c605166e0..bdc0ec2813a3 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -39,6 +39,7 @@
#include <linux/aer.h>
#include <linux/nmi.h>
#include <linux/sched/clock.h>
+#include <linux/spinlock.h>
#include <linux/uuid.h>
#include <linux/ras.h>
#include <linux/task_work.h>
@@ -677,7 +678,7 @@ static void ghes_defer_non_standard_event(struct acpi_hest_generic_data *gdata,
/*
* Only a single callback can be registered for CXL CPER events.
*/
-static DECLARE_RWSEM(cxl_cper_rw_sem);
+static DEFINE_RWLOCK(cxl_callback_lock);
static cxl_cper_callback cper_callback;

/* CXL Event record UUIDs are formatted as GUIDs and reported in section type */
@@ -721,14 +722,14 @@ static void cxl_cper_post_event(enum cxl_event_type event_type,
return;
}

- guard(rwsem_read)(&cxl_cper_rw_sem);
+ guard(read_lock_irqsave)(&cxl_callback_lock);
if (cper_callback)
cper_callback(event_type, rec);
}

int cxl_cper_register_callback(cxl_cper_callback callback)
{
- guard(rwsem_write)(&cxl_cper_rw_sem);
+ guard(write_lock_irq)(&cxl_callback_lock);
if (cper_callback)
return -EINVAL;
cper_callback = callback;
@@ -738,7 +739,7 @@ EXPORT_SYMBOL_NS_GPL(cxl_cper_register_callback, CXL);

int cxl_cper_unregister_callback(cxl_cper_callback callback)
{
- guard(rwsem_write)(&cxl_cper_rw_sem);
+ guard(write_lock_irq)(&cxl_callback_lock);
if (callback != cper_callback)
return -EINVAL;
cper_callback = NULL;

---
base-commit: 861c0981648f5b64c86fd028ee622096eb7af05a
change-id: 20240201-cxl-cper-smatch-82b129498498

Best regards,
--
Ira Weiny <[email protected]>

2024-02-02 19:14:18

by Dan Williams

Subject: Re: [PATCH] acpi/ghes: Prevent sleeping with spinlock held

Ira Weiny wrote:
> Smatch caught that cxl_cper_post_event() is called with a spinlock held
> or preemption disabled.[1] There is no need for the callback to sleep.
>
> Replace the RW semaphore with a RW lock.
>
> A static call was considered but ARM does not select HAVE_STATIC_CALL
> and in that case setting the function pointer uses a RW semaphore.
>
> [1] https://lore.kernel.org/all/[email protected]/
>
> Reported-by: Dan Carpenter <[email protected]>
> Signed-off-by: Ira Weiny <[email protected]>
> ---
> drivers/acpi/apei/ghes.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 7b7c605166e0..bdc0ec2813a3 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -39,6 +39,7 @@
> #include <linux/aer.h>
> #include <linux/nmi.h>
> #include <linux/sched/clock.h>
> +#include <linux/spinlock.h>
> #include <linux/uuid.h>
> #include <linux/ras.h>
> #include <linux/task_work.h>
> @@ -677,7 +678,7 @@ static void ghes_defer_non_standard_event(struct acpi_hest_generic_data *gdata,
> /*
> * Only a single callback can be registered for CXL CPER events.
> */
> -static DECLARE_RWSEM(cxl_cper_rw_sem);
> +static DEFINE_RWLOCK(cxl_callback_lock);
> static cxl_cper_callback cper_callback;
>
> /* CXL Event record UUIDs are formatted as GUIDs and reported in section type */
> @@ -721,14 +722,14 @@ static void cxl_cper_post_event(enum cxl_event_type event_type,
> return;
> }
>
> - guard(rwsem_read)(&cxl_cper_rw_sem);
> + guard(read_lock_irqsave)(&cxl_callback_lock);
> if (cper_callback)
> cper_callback(event_type, rec);

This does not help because cxl_cper_event_call() takes a sleeping
device_lock().