Re: [PATCH 18/23] cpu/hotplug: Add a new cpuhp_offline_cb() API

[sashiko-bot@kernel.org]

Sashiko AI review found 4 potential issue(s):
- [High] Early loop break in the rollback path leaves successfully offlined CPUs permanently offline.
- [High] Race condition in the `device_online()` retry loop bypasses recovery if hotplug is re-enabled immediately.
- [High] Hardcoded 100ms timeout for `cpu_hotplug_disabled` is brittle due to lock dropping, leading to permanently stranded CPUs.
- [Medium] Time-of-check to time-of-use (TOCTOU) race on `cpu_online_mask` validation triggers spurious kernel warnings and API failure.
--

commit ebe2a08265b3e2e79b0d74906f6ed951ac37520b
Author: Waiman Long <longman@redhat.com>

cpu/hotplug: Add a new cpuhp_offline_cb() API

This patch introduces the cpuhp_offline_cb() API to offline a set of CPUs,
execute a callback function, and then bring them back online. It allows
runtime adjustment of command line options like nohz_full and isolcpus.
A new flag, cpuhp_offline_cb_mode, is added to optimize hotplug operations
while the system is in this transient state.

> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 0d02b5d7a7baf..9b32f742cd1da 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -1520,6 +1520,76 @@ int remove_cpu(unsigned int cpu)
>  }
>  EXPORT_SYMBOL_GPL(remove_cpu);
>  
> +bool cpuhp_offline_cb_mode;
> +
> +/**
> + * cpuhp_offline_cb - offline CPUs, invoke callback function & online CPUs afterward
> + * @mask: A mask of CPUs to be taken offline and then online
> + * @func: A callback function to be invoked while the given CPUs are offline
> + * @arg:  Argument to be passed back to the callback function
> + *
> + * Return: 0 if successful, an error code otherwise
> + */
> +int cpuhp_offline_cb(struct cpumask *mask, cpuhp_cb_t func, void *arg)
> +{
> +	int off_cpu, on_cpu, ret, ret2 = 0;
> +
> +	if (WARN_ON_ONCE(cpumask_empty(mask) ||
> +	   !cpumask_subset(mask, cpu_online_mask)))
> +		return -EINVAL;

Since the mask validation checks cpu_online_mask before acquiring
lock_device_hotplug(), could a concurrent user-initiated sysfs CPU offline
operation remove a CPU from cpu_online_mask after the caller generates the
mask? This would trigger a spurious WARN_ON_ONCE and fail with -EINVAL.
Should this validation be moved under the hotplug lock?

> +
> +	pr_debug("%s: begin (CPU list = %*pbl)\n", __func__, cpumask_pr_args(mask));
> +	lock_device_hotplug();
> +	cpuhp_offline_cb_mode = true;

[ ... ]

> +	/* Bring previously offline CPUs back online */
> +	for_each_cpu(on_cpu, mask) {
> +		int retries = 0;
> +
> +		if (on_cpu == off_cpu)
> +			break;
> +
> +retry:
> +		ret2 = device_online(get_cpu_device(on_cpu));
> +
> +		/*
> +		 * With the unlikely event that CPU hotplug is disabled while
> +		 * this operation is in progress, we will need to wait a bit
> +		 * for hotplug to hopefully be re-enabled again. If not, print
> +		 * a warning and return the error.
> +		 *
> +		 * cpu_hotplug_disabled is supposed to be accessed while
> +		 * holding the cpu_add_remove_lock mutex. So we need to
> +		 * use the data_race() macro to access it here.
> +		 */
> +		while ((ret2 == -EBUSY) && data_race(cpu_hotplug_disabled) &&
> +		       (++retries <= 5)) {

If device_online() returns -EBUSY, but another thread re-enables CPU hotplug
right before the while loop condition is evaluated,
data_race(cpu_hotplug_disabled) will evaluate to false. Doesn't this
short-circuit the loop entirely, leaving ret2 == -EBUSY and bypassing the
retry mechanism exactly when recovery would have succeeded?

Additionally, is the hardcoded 100ms (5 * 20ms) timeout sufficient?
Concurrent paths like PCI probing or idle injection might hold the
cpu_add_remove_lock for several seconds via cpu_hotplug_disable(). If the
timeout expires during the online phase, won't this abort the API and
permanently abandon the CPUs in an offline state?

> +			msleep(20);
> +			if (!data_race(cpu_hotplug_disabled))
> +				goto retry;
> +		}
> +		if (ret2) {
> +			pr_warn("%s: Failed to bring CPU %d back online!\n",
> +				__func__, on_cpu);
> +			break;

If device_online() fails to bring a CPU back online and breaks out of the
loop here, doesn't it prematurely exit the restoration loop? This would skip
any remaining CPUs in the mask that were successfully taken offline, leaving
them permanently offline and reducing system capacity. Should this use
continue instead of break to ensure all offlined CPUs have a chance to be
restored?

> +		}
> +	}
> +	cpuhp_offline_cb_mode = false;
> +	unlock_device_hotplug();
> +	pr_debug("%s: end\n", __func__);
> +	return ret ? ret : ret2;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260421030351.281436-1-longman@redhat.com?part=18