Re: [PATCH] RDMA/ipoib: cancel the AH reaper work synchronously

[Leon Romanovsky <leon@kernel.org>]

On Wed, Nov 04, 2026 at 01:07:03AM +0800, Chengyu Liu wrote:
> ipoib_stop_ah_reaper() uses cancel_delayed_work(), which cannot stop a
> callback that is already running.  ipoib_reap_ah() re-arms itself after
> an unlocked test_bit(IPOIB_STOP_REAPER) check, so a callback that has
> passed the check but has not re-armed yet still queues the delayed work
> after the "stop":
> 
>   CPU0 (reaper)                     CPU1 (stop/teardown)
>   test_bit(STOP_REAPER) == 0
>                                     set_bit(STOP_REAPER)
>                                     cancel_delayed_work()  (no-op)
>                                     ... destroy_workqueue(priv->wq)
>   queue_delayed_work(priv->wq, ...)  <- delayed work sits on the timer
> 
> destroy_workqueue() cannot see a delayed work that is pending on the
> timer wheel, so the re-armed work fires after the workqueue is
> destroyed and priv is freed: both the queueing onto the dead queue and
> the callback's container_of() on freed memory are use-after-free.
> 
> Use cancel_delayed_work_sync(), which waits for a running callback and
> prevents its re-arm from taking effect.
> 
> The comment above claims ipoib_reap_dead_ahs() "ensures the work is
> really stopped", but it never touches the work item - it only walks the
> dead_ahs list under priv->lock.  The stop guarantee comes from the
> cancel; fix the comment along with it.
> 
> Found by static analysis.  The race was reproduced on a KASAN build
> with a small module exercising this exact guard/cancel/free pattern
> (no Infiniband hardware available); PoC and splat are below.

Please revisit this once you can reproduce the issue with real IPoIB code,
not code that merely resembles it.

Dropped for now.

Thanks

> 
> 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=3 (cancel_delayed_work + lockless guard race, this bug's shape) produces
> 
>     [    2.05] BUG: KASAN: slab-use-after-free in queue_delayed_work_on+0x27/0x70
>     [    2.05] Workqueue: events pm_td_cb [pm_flush_poc]
> 
> 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/infiniband/ulp/ipoib/ipoib_ib.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_ib.c b/drivers/infiniband/ulp/ipoib/ipoib_ib.c
> index 81bbb3f7c1..5ed4f709cb 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_ib.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_ib.c
> @@ -736,11 +736,11 @@ static void ipoib_start_ah_reaper(struct ipoib_dev_priv *priv)
>  static void ipoib_stop_ah_reaper(struct ipoib_dev_priv *priv)
>  {
>  	set_bit(IPOIB_STOP_REAPER, &priv->flags);
> -	cancel_delayed_work(&priv->ah_reap_task);
> +	cancel_delayed_work_sync(&priv->ah_reap_task);
>  	/*
>  	 * After ipoib_stop_ah_reaper() we always go through
> -	 * ipoib_reap_dead_ahs() which ensures the work is really stopped and
> -	 * does a final flush out of the dead_ah's list
> +	 * ipoib_reap_dead_ahs() which does a final flush out of the
> +	 * dead_ah's list
>  	 */
>  }
>  
> -- 
> 2.25.1
>