[PATCH] accel/habanalabs: disable pending-reset work in hl_device_fini()

[PATCH] accel/habanalabs: disable pending-reset work in hl_device_fini()

[Chengyu Liu]

hl_device_fini() sets device_fini_pending and flush_delayed_work()s the
pending-reset work, but flush only waits for a running callback - it
does not prevent the callback from re-arming.
device_hard_reset_pending() re-arms itself via queue_delayed_work() when
hl_device_reset() fails with -EBUSY and the (unlocked)
device_fini_pending check still reads 0, so a callback that passed that
check before fini set it re-arms during the flush, and the fresh timer
survives: destroy_workqueue(hdev->reset_wq) cannot see a delayed work
pending on the timer wheel, so the work fires later, queues onto a
destroyed workqueue and runs with a freed hdev.  Use-after-free.

On a busy device (live compute contexts make hl_device_reset() keep
failing with -EBUSY) this work sits in a per-second retry loop and the
callback body is a real reset attempt, so hitting the window on device
removal/FLR is observable in practice, and retries are free.

Use disable_delayed_work_sync(), which waits for a running callback to
finish and leaves the work item disabled, so the callback's
queue_delayed_work() re-arm has no effect.

Found by static analysis.  The race was reproduced on a KASAN build
with a small module exercising this exact flush/re-arm/free pattern
(no Gaudi hardware available); PoC and splat are below.

Cc: stable@vger.kernel.org
Signed-off-by: Chengyu Liu <lui2012qwq@gmail.com>
---

Reproducer notes (PoC source below; a module parameter selects the
shape): mode=0 (flush_delayed_work + lockless guard, this bug's shape) produces

    [    2.22] BUG: KASAN: slab-use-after-free in __run_timer_base.part.0+0x807/0x900

after the teardown frees the container; mode=1
(cancel_delayed_work_sync, i.e. this fix) stays clean.

---------------------------- reproducer: pm_flush_poc.c ----------------------------
// SPDX-License-Identifier: GPL-2.0
/*
 * pm_teardown_poc - mechanism PoC for the drv-teardown-sync C/D variants
 * (F-026 ipoib / F-028 epf-ntb / F-029 habanalabs).
 *
 * Modes (module parameter):
 *   0  F-029 vuln shape:  flush_delayed_work + lockless guard
 *   1  control:           cancel_delayed_work_sync (>=6.9 swallows re-arm)
 *   2  F-028 vuln shape:  cancel_delayed_work (non-sync) + unconditional re-arm
 *   3  F-026 vuln shape:  cancel_delayed_work (non-sync) + guard check/queue
 *                         race window (test passes, teardown slips in before
 *                         the queue_delayed_work)
 *
 * Every vuln mode must produce KASAN slab-use-after-free when the orphaned
 * work fires after kfree; the control mode must stay clean.
 */
#include <linux/module.h>
#include <linux/workqueue.h>
#include <linux/slab.h>
#include <linux/delay.h>

static int mode;
module_param(mode, int, 0444);
MODULE_PARM_DESC(mode, "0=flush(F-029) 1=cancel_sync(ctrl) 2=cancel_nosync(F-028) 3=cancel_nosync+guard-race(F-026)");

struct pm_td_ctx {
	bool			fini_pending;
	struct delayed_work	work;
};

static struct pm_td_ctx *ctx;

static void pm_td_cb(struct work_struct *w)
{
	struct pm_td_ctx *c = container_of(to_delayed_work(w),
					   struct pm_td_ctx, work);

	pr_info("PM-POC: cb running, fini_pending=%d\n", c->fini_pending);

	switch (mode) {
	case 0:		/* F-029: lockless guard then long body then re-arm */
		if (c->fini_pending)
			return;
		msleep(300);
		schedule_delayed_work(&c->work, HZ / 10);
		pr_info("PM-POC: re-armed (flush shape)\n");
		break;
	case 2:		/* F-028: unconditional re-arm, cancel must catch running cb */
		msleep(300);	/* long body so cancel lands mid-run */
		schedule_delayed_work(&c->work, HZ / 10);
		pr_info("PM-POC: re-armed (no guard)\n");
		break;
	case 3:		/* F-026: guard check passes, THEN teardown slips in */
		if (c->fini_pending)
			return;
		/* widen the check->queue race window like ipoib's 724->725 */
		msleep(300);
		schedule_delayed_work(&c->work, HZ / 10);
		pr_info("PM-POC: re-armed past guard (test-queue race)\n");
		break;
	default:
		break;
	}
}

static int __init pm_poc_init(void)
{
	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
	if (!ctx)
		return -ENOMEM;
	ctx->fini_pending = false;
	INIT_DELAYED_WORK(&ctx->work, pm_td_cb);

	pr_info("PM-POC: arming, mode=%d\n", mode);
	schedule_delayed_work(&ctx->work, 0);

	msleep(50);	/* callback is mid-run now */

	/* teardown sequence for the selected variant */
	ctx->fini_pending = true;
	switch (mode) {
	case 1:
		cancel_delayed_work_sync(&ctx->work);
		break;
	case 0:
		flush_delayed_work(&ctx->work);
		break;
	default:
		cancel_delayed_work(&ctx->work);	/* non-sync: C variants */
		break;
	}

	pr_info("PM-POC: teardown done, freeing ctx\n");
	kfree(ctx);

	msleep(600);
	pr_info("PM-POC: SURVIVED (no KASAN splat)\n");
	return 0;
}

static void __exit pm_poc_exit(void) { }

module_init(pm_poc_init);
module_exit(pm_poc_exit);
MODULE_LICENSE("GPL");
MODULE_DESCRIPTION("drv-teardown-sync C/D variant mechanism PoC (F-026/028/029)");

 drivers/accel/habanalabs/common/device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/accel/habanalabs/common/device.c b/drivers/accel/habanalabs/common/device.c
index 09b27bac3a..4635dc8572 100644
--- a/drivers/accel/habanalabs/common/device.c
+++ b/drivers/accel/habanalabs/common/device.c
@@ -2487,7 +2487,7 @@ void hl_device_fini(struct hl_device *hdev)
 	dev_info(hdev->dev, "Removing device %s\n", dev_name(&(hdev)->pdev->dev));
 
 	hdev->device_fini_pending = 1;
-	flush_delayed_work(&hdev->device_reset_work.reset_work);
+	disable_delayed_work_sync(&hdev->device_reset_work.reset_work);
 
 	if (hdev->pldm)
 		reset_sec = HL_PLDM_HARD_RESET_MAX_TIMEOUT;
-- 
2.25.1

Re: [PATCH] accel/habanalabs: disable pending-reset work in hl_device_fini()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] accel/habanalabs: disabling reset_work in hl_device_fini() strands in_reset
--

commit eb768a82770d27a90572b30cd01bc34db4fd01ba
Author: Chengyu Liu <lui2012qwq@gmail.com>

accel/habanalabs: disable pending-reset work in hl_device_fini()

This commit replaces flush_delayed_work() with disable_delayed_work_sync() in
hl_device_fini() to prevent device_hard_reset_pending() from re-arming
hdev->device_reset_work.reset_work during device teardown.

> diff --git a/drivers/accel/habanalabs/common/device.c b/drivers/accel/habanalabs/common/device.c
> index 09b27bac3a31d..4635dc8572f03 100644
> --- a/drivers/accel/habanalabs/common/device.c
> +++ b/drivers/accel/habanalabs/common/device.c
> @@ -2487,7 +2487,7 @@ void hl_device_fini(struct hl_device *hdev)
>  	dev_info(hdev->dev, "Removing device %s\n", dev_name(&(hdev)->pdev->dev));
>  
>  	hdev->device_fini_pending = 1;
> -	flush_delayed_work(&hdev->device_reset_work.reset_work);
> +	disable_delayed_work_sync(&hdev->device_reset_work.reset_work);

[Severity: Critical]
Does cancelling and disabling hdev->device_reset_work.reset_work here leave
hdev->reset_info.in_reset stuck at 1 and cause hl_device_fini() to time out
and skip teardown?

When a hard reset is initiated outside the reset thread, hl_device_reset()
sets hdev->reset_info.in_reset = 1 and hands off clearing it to
hdev->device_reset_work.reset_work:

drivers/accel/habanalabs/common/device.c:hl_device_reset() {
	...
		hdev->reset_info.in_compute_reset = !hard_reset;

		hdev->reset_info.in_reset = 1;

		spin_unlock(&hdev->reset_info.lock);
	...
	if ((hard_reset) && (!from_hard_reset_thread)) {
		hdev->reset_info.hard_reset_pending = true;
		...
		queue_delayed_work(hdev->reset_wq, &hdev->device_reset_work.reset_work, 0);

		return 0;
	}
}

When device_hard_reset_pending() runs hl_device_reset() and
device_kill_open_processes() returns -EBUSY while device_fini_pending is 0,
hl_device_reset() returns -EBUSY directly without jumping to out_err, keeping
hdev->reset_info.in_reset set to 1 across the retry delay:

drivers/accel/habanalabs/common/device.c:hl_device_reset() {
	...
		rc = device_kill_open_processes(hdev, 0, false);

		if (rc == -EBUSY) {
			if (hdev->device_fini_pending) {
				dev_crit(hdev->dev,
					"%s Failed to kill all open processes, stopping hard reset\n",
					dev_name(&(hdev)->pdev->dev));
				goto out_err;
			}

			/* signal reset thread to reschedule */
			return rc;
		}
	...
out_err:
	...
	hdev->reset_info.in_reset = 0;

	spin_unlock(&hdev->reset_info.lock);

	return rc;
}

Previously, setting hdev->device_fini_pending = 1 and calling
flush_delayed_work() forced any pending reset_work to execute immediately
with device_fini_pending == 1, which entered the goto out_err path and
cleared hdev->reset_info.in_reset = 0.

With disable_delayed_work_sync(), if reset_work is waiting on its retry timer
(or queued on reset_wq before starting), or if a running callback already read
hdev->device_fini_pending == 0 in hl_device_reset() before returning -EBUSY,
or if a concurrent hl_device_reset() sets in_reset = 1 and queues reset_work
before hl_device_fini() takes hdev->reset_info.lock, the work item never runs
again to clear hdev->reset_info.in_reset.

Won't hl_device_fini() then spin in while (device_in_reset) for the full
HL_HARD_RESET_MAX_TIMEOUT (or HL_PLDM_HARD_RESET_MAX_TIMEOUT) and return
early without cleaning up the device before hl_pci_remove() calls
destroy_hdev()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103163354.1599176-1-lui2012qwq@gmail.com?part=1