[PATCH] EDAC/device: Serialize poll_msec updates against device teardown

[PATCH] EDAC/device: Serialize poll_msec updates against device teardown

[Jad Keskes]

The poll_msec attribute was writable on interrupt-driven controllers,
where the value is meaningless, and a write racing with device removal
could re-arm the polling workqueue after it had been stopped and the
ctl_info freed -- use-after-free.

Restrict the attribute to controllers that are actually polled: _show()
and _store() now return -EPERM unless the controller is in the
OP_RUNNING_POLL state.

That check alone is not sufficient against the teardown race: _store()
can pass it and then be preempted by a concurrent del_device() that sets
OP_OFFLINE, stops the workqueue and frees the ctl_info once the store
completes.  So re-check the state inside reset_delay_period()
under device_ctls_mutex -- the same lock under which del_device()
updates op_state -- making the check atomic with the re-arm.  The mutex
is taken only around the check and re-arm, never across the stop/teardown
path, so it cannot deadlock against other pollers queued on the shared
EDAC workqueue.

Reported-by: Borislav Petkov <bp@alien8.de>
Signed-off-by: Jad Keskes <inasj268@gmail.com>
---
 drivers/edac/edac_device.c       | 11 +++++++++++
 drivers/edac/edac_device_sysfs.c |  8 ++++++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/edac/edac_device.c b/drivers/edac/edac_device.c
index 361dc985497b..519d1c6d0930 100644
--- a/drivers/edac/edac_device.c
+++ b/drivers/edac/edac_device.c
@@ -392,10 +392,21 @@ static void edac_device_workq_teardown(struct edac_device_ctl_info *edac_dev)
  */
 void edac_device_reset_delay_period(struct edac_device_ctl_info *edac_dev, unsigned int msec)
 {
+	mutex_lock(&device_ctls_mutex);
+
+	/* Don't re-arm the workqueue once teardown has begun or when the
+	 * controller isn't polled; this check must be atomic with the
+	 * re-arm below so it can't race a concurrent del_device().
+	 */
+	if (edac_dev->op_state != OP_RUNNING_POLL)
+		goto out;
+
 	edac_dev->poll_msec = msec;
 	edac_dev->delay     = msecs_to_jiffies(msec);
 
 	edac_mod_work(&edac_dev->work, edac_dev->delay);
+out:
+	mutex_unlock(&device_ctls_mutex);
 }
 
 int edac_device_alloc_index(void)
diff --git a/drivers/edac/edac_device_sysfs.c b/drivers/edac/edac_device_sysfs.c
index e12122b2f42e..821a80806086 100644
--- a/drivers/edac/edac_device_sysfs.c
+++ b/drivers/edac/edac_device_sysfs.c
@@ -83,6 +83,10 @@ static ssize_t edac_device_ctl_panic_on_ue_store(struct edac_device_ctl_info
 static ssize_t edac_device_ctl_poll_msec_show(struct edac_device_ctl_info
 					*ctl_info, char *data)
 {
+	/* Interval is only meaningful while running under polling */
+	if (ctl_info->op_state != OP_RUNNING_POLL)
+		return -EPERM;
+
 	return sprintf(data, "%u\n", ctl_info->poll_msec);
 }
 
@@ -93,6 +97,10 @@ static ssize_t edac_device_ctl_poll_msec_store(struct edac_device_ctl_info
 	unsigned int value;
 	int ret;
 
+	/* Only meaningful when the device is running under polling */
+	if (ctl_info->op_state != OP_RUNNING_POLL)
+		return -EPERM;
+
 	/*
 	 * Get the value, make sure it is non-zero, must be at least one
 	 * millisecond for the delay period between scans.
-- 
2.55.0

Re: [PATCH] EDAC/device: Serialize poll_msec updates against device teardown

[Borislav Petkov]

On Sat, Aug 08, 2026 at 11:39:03PM +0100, Jad Keskes wrote:
> The poll_msec attribute was writable on interrupt-driven controllers,

Controllers?

> where the value is meaningless, and a write racing with device removal
> could re-arm the polling workqueue after it had been stopped and the
> ctl_info freed -- use-after-free.
> 
> Restrict the attribute to controllers that are actually polled: _show()
> and _store() now return -EPERM unless the controller is in the
> OP_RUNNING_POLL state.
> 
> That check alone is not sufficient against the teardown race: _store()
> can pass it and then be preempted by a concurrent del_device() that sets
> OP_OFFLINE, stops the workqueue and frees the ctl_info once the store
> completes.  So re-check the state inside reset_delay_period()
> under device_ctls_mutex -- the same lock under which del_device()
> updates op_state -- making the check atomic with the re-arm.  The mutex
> is taken only around the check and re-arm, never across the stop/teardown
> path, so it cannot deadlock against other pollers queued on the shared
> EDAC workqueue.
> 
> Reported-by: Borislav Petkov <bp@alien8.de>

This is not how this tag should be used. Please take the time to read up on
the development process before submitting more patches:

https://www.kernel.org/doc/html/latest/process/development-process.html

This too:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html

Also, how much of this patch has been done with the help of LLM?

We have a tag for that - Assisted-by:

> Signed-off-by: Jad Keskes <inasj268@gmail.com>
> ---
>  drivers/edac/edac_device.c       | 11 +++++++++++
>  drivers/edac/edac_device_sysfs.c |  8 ++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/edac/edac_device.c b/drivers/edac/edac_device.c
> index 361dc985497b..519d1c6d0930 100644
> --- a/drivers/edac/edac_device.c
> +++ b/drivers/edac/edac_device.c
> @@ -392,10 +392,21 @@ static void edac_device_workq_teardown(struct edac_device_ctl_info *edac_dev)
>   */
>  void edac_device_reset_delay_period(struct edac_device_ctl_info *edac_dev, unsigned int msec)
>  {
> +	mutex_lock(&device_ctls_mutex);
> +
> +	/* Don't re-arm the workqueue once teardown has begun or when the
> +	 * controller isn't polled; this check must be atomic with the
> +	 * re-arm below so it can't race a concurrent del_device().
> +	 */

This is not the right comment format.

> +	if (edac_dev->op_state != OP_RUNNING_POLL)
> +		goto out;
> +
>  	edac_dev->poll_msec = msec;
>  	edac_dev->delay     = msecs_to_jiffies(msec);
>  
>  	edac_mod_work(&edac_dev->work, edac_dev->delay);
> +out:
> +	mutex_unlock(&device_ctls_mutex);
>  }
>  
>  int edac_device_alloc_index(void)
> diff --git a/drivers/edac/edac_device_sysfs.c b/drivers/edac/edac_device_sysfs.c
> index e12122b2f42e..821a80806086 100644
> --- a/drivers/edac/edac_device_sysfs.c
> +++ b/drivers/edac/edac_device_sysfs.c
> @@ -83,6 +83,10 @@ static ssize_t edac_device_ctl_panic_on_ue_store(struct edac_device_ctl_info
>  static ssize_t edac_device_ctl_poll_msec_show(struct edac_device_ctl_info
>  					*ctl_info, char *data)
>  {
> +	/* Interval is only meaningful while running under polling */

An LLM usually slaps meaningless comments like that.

> +	if (ctl_info->op_state != OP_RUNNING_POLL)
> +		return -EPERM;
> +
>  	return sprintf(data, "%u\n", ctl_info->poll_msec);
>  }
>  
> @@ -93,6 +97,10 @@ static ssize_t edac_device_ctl_poll_msec_store(struct edac_device_ctl_info
>  	unsigned int value;
>  	int ret;
>  
> +	/* Only meaningful when the device is running under polling */

Here too.

> +	if (ctl_info->op_state != OP_RUNNING_POLL)
> +		return -EPERM;
> +
>  	/*
>  	 * Get the value, make sure it is non-zero, must be at least one
>  	 * millisecond for the delay period between scans.
> -- 
> 2.55.0
> 

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette