Re: [PATCH v23 0/2] ACPI: Add support for ACPI RAS2 feature table

Hi Borislav,

Sorry for the late reply.
Please find my reply to Sashiko reported issues and
apologise that the replay is not in a patch friendly format.

1)
> diff --git a/drivers/ras/acpi_ras2.c b/drivers/ras/acpi_ras2.c
> new file mode 100644
> index 0000000000000..51adfcddfa99a
> --- /dev/null
> +++ b/drivers/ras/acpi_ras2.c
[ ... ]
> +static int ras2_update_patrol_scrub_params_cache(struct ras2_mem_ctx *ras2_ctx)
> +{
[ ... ]
> + if  (ioread32(&ps_sm->params.flags) & RAS2_PS_FLAG_SCRUB_RUNNING) {
> + ras2_ctx->od_scrub = true;
> + ras2_ctx->base = ioread64(&ps_sm->params.actl_addr_range[0]);
> + ras2_ctx->size = ioread64(&ps_sm->params.actl_addr_range[1]);
> + } else {
> + ras2_ctx->od_scrub = false;
> + }
> +
> + return 0;
> +}
Does this logic misidentify a pre-existing background scrub as a demand
scrub?
If the platform firmware has background scrubbing enabled prior to driver
initialization, ioread32() will read RAS2_PS_FLAG_SCRUB_RUNNING. Because
the driver's internal bg_scrub state is initialized to false, this assumes
the active scrub is a demand scrub and sets od_scrub = true.
Would this prevent the user from controlling hardware memory scrubbing via
sysfs, since writing to enable_background might return -EBUSY and
writing to enable_demand might return -EOPNOTSUPP?

Shiju:  This is not a valid comment.
Please find a description from the spec.ACPI spec 6.5 RAS2
Table 5.87: Parameter Block Structure for PATROL_SCRUB:
Flags (OUTPUT) : The platform returns this value in response to
GET_PATROL_PARAMETERS:
Bit [0]: Will be set if memory scrubber is already running for address
range specified in “Actual Address
Range”.Bits [31:1]: Reserved, must be zero.

 ioread32(&ps_sm->params.flags) & RAS2_PS_FLAG_SCRUB_RUNNING
 ACPI spec 6.5 RAS2 Table 5.87: Parameter Block Structure for PATROL_SCRUB
  Field: Flags (OUTPUT): bit[0] - returns status for demand scrub only,
  and thus this condition still be false for the demand scrub as well
even background
  scrub is enabled prior to driver initialization.

2)
> +static int ras2_hw_scrub_set_enabled_od(struct device *dev, void *drv_data, bool enable)
> +{
[ ... ]
> + /*
> + * The validity checks for the address range to scrub need to be updated
> + * with (base + size) > (mem_base + mem_size) check in the future once a
> + * proper method for determining the NUMA node memory range is available.
> + */
> + if (!ras2_ctx->size || ras2_ctx->base < ras2_ctx->mem_base) {
> + dev_err(dev, "%s: Invalid address range, base=0x%llx size=0x%llx\n",
> + __func__, ras2_ctx->base, ras2_ctx->size);
> + ret = -ERANGE;
> + goto enable_bg_scrub;
> + }
Could this permit passing oversized physical memory requests to the ACPI
firmware?
The driver checks if the size is zero and if base < mem_base, but explicitly
skips checking if the requested memory range exceeds the NUMA node
boundaries (base + size > mem_base + mem_size).
Since mem_base and mem_size are initialized in ras2_mem_drv_probe(), would
it be possible to add the boundary check here to prevent out-of-bounds
physical memory accesses?

Shiju: The suggested check for NUMA node upper boundaries is unable to be added
          at present because of the reason given in the comment added
         (proper method for determining the NUMA node memory range is
not available now).

3)
> + goto enable_bg_scrub;
> + }
> +
> + ras2_ctx->od_scrub = enable;
> +
> + /*
> + * May need to return the error code from below function call to the user
> + * if the below function fail to update current scrub parameters even the
> + * demand scrubbing has started successfully.
> + */
> + ras2_update_patrol_scrub_params_cache(ras2_ctx);
> +
> + if (ras2_ctx->reenable_bg_scrub && ras2_ctx->driver_active && !ras2_ctx->thread) {
Can this allow hardware state modification during driver teardown?
Sysfs attributes are unregistered during the devres phase, which executes
after the auxiliary driver's remove() callback completes. If a sysfs write
to enable_demand hits this path during teardown, the command is sent to the
firmware before checking driver_active.
If driver_active is false, it skips creating the monitor thread, leaving the
hardware stranded in demand scrub mode.

Shiju: This is not true. Scrub sysfs attributes are attached with the
edac device and
          are added when the edac device is registered for acpi ras2
in edac_dev_register(),
          where the edac device is the child of this auxdev->dev. Thus
auxiliary driver's
          remove() callback would be called only after the
edac_dev_release(), and
           edac_dev_unreg() are called, where scrub sysfs attributes
are unregistered.
         Also both ras2_mem_drv_remove() and ras2_hw_scrub_set_enabled_od()
         are protected against race condition using pcc_lock.

4)
> + /*
> + * If kthread_run() is called before sending the command for demand scrubbing,
> + * the thread may determine that demand scrubbing is not running, attempt to
> + * restart background scrubbing, and then exit. This can occur because the
> + * thread function may start running before demand scrubbing starts.
> + */
> + ras2_ctx->thread = kthread_run(ras2_scrub_monitor_thread, ras2_ctx,
> + "ras2_scrub_nid%d", ras2_ctx->sys_comp_nid);
> + if (IS_ERR(ras2_ctx->thread)) {
> + ret = PTR_ERR(ras2_ctx->thread);
> + ras2_ctx->thread = NULL;
> + /*
> + * If kthread_run() fails and the demand scrubbing has started running,
> + * re-enabling background scrub will fail. Thus admin/firmware may need
> + * to re-enable background scrub after demand scrubbing has finished.
> + */
> + goto enable_bg_scrub;
> + }
Does this return an error to userspace despite a successful hardware state
change?
If kthread_run() fails due to memory pressure, the driver returns the
resulting PTR_ERR to userspace. However, the driver has already successfully
sent the RAS2_START_PATROL_SCRUBBER command to the ACPI firmware.
The rollback attempt to re-enable background scrub via
__ras2_hw_scrub_set_enabled_bg() might silently fail with -EBUSY because the
demand scrub is already running.

Shiju: In my opinion it is correct to return the error to the
user/admin instead of silent error,
          to indicate that the background scrub will not be enabled
automatically by the driver.
         This enables user/admin can check the status of the demand
scrub by reading
         the sysfs attribute 'enable_demand' periodically and
re-enable the background scrub
         when the demand scrub is finished.

5)
> +static void ras2_mem_drv_remove(struct auxiliary_device *auxdev)
> +{
> + struct ras2_mem_ctx *ras2_ctx = container_of(auxdev, struct ras2_mem_ctx, adev);
> +
> + if (!ras2_ctx)
> + return;
> +
> + mutex_lock(ras2_ctx->pcc_lock);
> + ras2_ctx->driver_active = false;
> + if (ras2_ctx->thread) {
> + /*
> + * Prevent the thread's `task_struct` from being freed if the
> + * thread function exits.
> + */
> + get_task_struct(ras2_ctx->thread);
> + mutex_unlock(ras2_ctx->pcc_lock);
> + kthread_stop_put(ras2_ctx->thread);
> + ras2_ctx->thread = NULL;
> + return;
> + }
> + mutex_unlock(ras2_ctx->pcc_lock);
> +}
Can this cause a use-after-free or Oops during driver removal?
In ras2_scrub_monitor_thread(), when the thread finishes, it sets
ras2_ctx->thread = NULL, unlocks pcc_lock, and gracefully exits:
ras2_scrub_monitor_thread()
...
if (ras2_ctx->driver_active)
ras2_ctx->thread = NULL;
mutex_unlock(ras2_ctx->pcc_lock);
return 0;
If ras2_mem_drv_remove() runs concurrently, it takes pcc_lock, sees
ras2_ctx->thread is NULL, and returns without calling kthread_stop() to wait
for the thread to fully exit.
This would allow the module memory to be freed while the kthread is still
executing its epilogue, which could result in a page fault.

Shiju : Sashiko specified that the thread function gracefully exits.
Thus calling kthread_stop(), as it suggested, in a kernel thread that
has already
fully exited on its own, risks a use-after-free or invalid pointer
dereference, which
Sashiko likely to complain next.

Thanks,
Shiju

On Fri, Aug 7, 2026 at 7:00 AM Borislav Petkov <[email protected]> wrote:
>
> On Sun, Jul 26, 2026 at 10:28:10PM +0100, [email protected] wrote:
> > From: Shiju Jose <[email protected]>
> >
> > Add support for ACPI RAS2 feature table (RAS2) defined in the
> > ACPI 6.5 specification, section 5.2.21 and RAS2 HW based memory
> > scrubbing feature.
> >
> > ACPI RAS2 patches were part of the EDAC series [1].
> >
> > The code is based on linux.git v7.2-rc2 [2].
>
> You're probably going to hate Sashiko by now but there's more issues against
> patch 2:
>
> https://sashiko.dev/#/patchset/20260726212812.89266-1-shijujose2008%40gmail.com
>
> Please sanity-check them if they even make sense. If they don't, you can say
> why and we can ignore.
>
> Thx.
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette