Re: [PATCH v2] RDMA/ipoib: disable the AH reaper work for good on teardown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] RDMA/ipoib: disable_delayed_work_sync() permanently kills ah_reap_task on stop
--

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

RDMA/ipoib: disable the AH reaper work for good on teardown

This commit replaces cancel_delayed_work() with disable_delayed_work_sync()
in ipoib_stop_ah_reaper() to prevent a running ipoib_reap_ah() callback from
re-arming itself afterIPOIB_STOP_REAPER is set.

> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_ib.c b/drivers/infiniband/ulp/ipoib/ipoib_ib.c
> index 81bbb3f7c1132..b53906e463cd9 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);
> +	disable_delayed_work_sync(&priv->ah_reap_task);

[Severity: High]
Does calling disable_delayed_work_sync() here permanently disable
priv->ah_reap_task across interface restarts and heavy flushes, causing
retired struct ipoib_ah and struct ib_ah entries on priv->dead_ahs to stop
being reaped?

ipoib_stop_ah_reaper() is called from ipoib_ib_dev_stop(), which runs not
only on final teardown, but also during ndo_stop (ipoib_stop()), TX timeout
recovery (ipoib_ib_tx_timeout_work()), and heavy device flushes in
__ipoib_ib_dev_flush():

    if (level == IPOIB_FLUSH_HEAVY) {
        netdev_lock_ops(dev);
        if (test_bit(IPOIB_FLAG_INITIALIZED, &priv->flags))
            ipoib_ib_dev_stop(dev);

        result = ipoib_ib_dev_open(dev);
        netdev_unlock_ops(dev);

disable_delayed_work_sync() leaves the work item's disable count incremented.
When the device is subsequently reopened via ipoib_ib_dev_open() ->
ipoib_start_ah_reaper():

    static void ipoib_start_ah_reaper(struct ipoib_dev_priv *priv)
    {
        clear_bit(IPOIB_STOP_REAPER, &priv->flags);
        queue_delayed_work(priv->wq, &priv->ah_reap_task,
                           round_jiffies_relative(HZ));
    }

priv->ah_reap_task is never re-enabled with enable_delayed_work(), so
queue_delayed_work() silently returns false and ipoib_reap_ah() never runs
again while the interface is up, leaving AHs added by ipoib_free_ah() on
priv->dead_ahs without periodic reaping.

Would cancel_delayed_work_sync() suffice here instead? Because
set_bit(IPOIB_STOP_REAPER, &priv->flags) is called before
cancel_delayed_work_sync(), __cancel_work_sync() temporarily disables the
work while waiting for any in-flight ipoib_reap_ah() callback to finish (which
swallows a racing re-arm) and then re-enables the work before returning.

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