Re: [RFC v3 1/2] workqueue: Add support for real-time workers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Uninitialized pointer `worker` is freed if `ida_alloc()` fails in `create_worker()`, causing memory corruption.
- [High] Worker pools can become permanently deadlocked when the global RT worker limit is reached due to an infinite sleep loop in the worker manager.
- [High] The global `total_rtpri_workers` atomic counter is permanently leaked when worker allocation or thread creation fails.
- [High] Strict CPU affinity meant for RT workqueues is immediately discarded and overwritten by standard NUMA attributes.
- [High] Creating an RT workqueue with WQ_AFFN_CPU affinity deterministically fails due to hitting the total_rtpri_workers limit on initialization.
- [Medium] The safety limit on max_active designed to prevent RT scheduler starvation can be bypassed via sysfs.
--

--- Patch [1]: [RFC v3 1/2] workqueue: Add support for real-time workers ---
commit 9672bfd2ee579be7697316d4370a3f3836fac4ce
Author: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

workqueue: Add support for real-time workers

Adds WQ_RTPRI for real-time priority unbound workqueues to avoid latency
spikes from background CPU load. Limits RT workers system-wide to prevent
starvation.

> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 78068ae8f28a6..30fed46a31640 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
[ ... ]
> @@ -2838,12 +2841,30 @@ static struct worker *create_worker(struct worker_pool *pool)
>  	struct worker *worker;
>  	int id;
>  
> +	/*
> +	 * Do not consume all CPUs with RT workers to avoid scheduler
> +	 * starvation.
> +	 */
> +	if (pool->attrs->prio == WQ_PRIO_RT) {
> +		unsigned int max = num_online_cpus();
> +
> +		if (max > 2)
> +			max = max - 1;
> +		else
> +			max = 1;
> +
> +		if (atomic_inc_return(&total_rtpri_workers) > max) {
> +			atomic_dec(&total_rtpri_workers);
> +			return NULL;

[Severity: High]
Does returning NULL here cause a deadlock when called from
maybe_create_worker()?

maybe_create_worker() runs in a while (true) loop and expects create_worker()
failures to be temporary memory allocation issues:

maybe_create_worker() {
    while (true) {
        if (create_worker(pool) || !need_to_create_worker(pool))
            break;
        schedule_timeout_interruptible(CREATE_COOLDOWN);
    }
}

If the total_rtpri_workers limit is reached, create_worker() deterministically
returns NULL. Does this cause the manager thread to enter an infinite sleep
loop and deadlock the pool?

> +		}
> +	}
> +
>  	/* ID is needed to determine kthread name */
>  	id = ida_alloc(&pool->worker_ida, GFP_KERNEL);
>  	if (id < 0) {
>  		pr_err_once("workqueue: Failed to allocate a worker ID: %pe\n",
>  			    ERR_PTR(id));
> -		return NULL;
> +		goto fail_ida;

[Severity: Critical]
Is the uninitialized pointer worker freed if ida_alloc() fails?

At this point worker has not been allocated. If ida_alloc() fails, the code
jumps to fail_ida, which falls through to fail:

fail_ida:
    if (pool->attrs->prio == WQ_PRIO_RT)
        atomic_dec(&total_rtpri_workers);
fail:
    ida_free(&pool->worker_ida, id);
    kfree(worker);

Will this call kfree() on a garbage pointer?

>  	}
>  
>  	worker = alloc_worker(pool->node);
> @@ -2871,7 +2892,11 @@ static struct worker *create_worker(struct worker_pool *pool)
>  			goto fail;

[Severity: High]
Does jumping to fail skip the total_rtpri_workers decrement?

If alloc_worker() or kthread_create_on_node() fails, they jump to the fail
label, which is placed below fail_ida. Doesn't this skip the atomic_dec() and
permanently leak the global total_rtpri_workers counter?

>  		}
>  
> -		set_user_nice(worker->task, pool->attrs->nice);
> +		if (pool->attrs->prio == WQ_PRIO_RT)
> +			sched_set_fifo_low(worker->task);
> +		else
> +			set_user_nice(worker->task, pool->attrs->nice);
> +
>  		kthread_bind_mask(worker->task, pool_allowed_cpus(pool));
>  	}
[ ... ]
> @@ -5622,7 +5663,7 @@ static int alloc_and_link_pwqs(struct workqueue_struct *wq)
[ ... ]
>  	if (wq->flags & __WQ_ORDERED) {
>  		struct pool_workqueue *dfl_pwq;
>  
> -		ret = apply_workqueue_attrs_locked(wq, ordered_wq_attrs[highpri]);
> +		ret = apply_workqueue_attrs_locked(wq, ordered_wq_attrs[prio]);
>  		/* there should only be single pwq for ordering guarantee */
>  		dfl_pwq = rcu_access_pointer(wq->dfl_pwq);
>  		WARN(!ret && (wq->pwqs.next != &dfl_pwq->pwqs_node ||
>  			      wq->pwqs.prev != &dfl_pwq->pwqs_node),
>  		     "ordering guarantee broken for workqueue %s\n", wq->name);
>  	} else {
> -		ret = apply_workqueue_attrs_locked(wq, unbound_std_wq_attrs[highpri]);
> +		ret = apply_workqueue_attrs_locked(wq, unbound_std_wq_attrs[prio]);

[Severity: High]
Does this overwrite the strict CPU affinity intended for RT workqueues?

In __alloc_workqueue(), wq->unbound_attrs is correctly configured with
WQ_AFFN_CPU and affn_strict = true for RT workqueues. However,
alloc_and_link_pwqs() immediately calls apply_workqueue_attrs_locked()
with unbound_std_wq_attrs[prio]. Does this replace the strict affinity
with the default NUMA-scoped non-strict attributes?

>  	}
>  
>  	if (ret)
[ ... ]
> @@ -5857,7 +5904,17 @@ static struct workqueue_struct *__alloc_workqueue(const char *fmt,
>  		flags &= ~WQ_PERCPU;
>  	}
>  
> -	if (flags & WQ_BH) {
> +	if (flags & WQ_RTPRI) {
> +		/*
> +		 * RT workqueues are limited to max half of possible CPUs to
> +		 * avoid scheduling starvation and have strict CPU affinity for
> +		 * low latency execution.
> +		 */
> +		max_active = min_t(int, max_active,
> +				   DIV_ROUND_UP(num_possible_cpus(), 2));

[Severity: Medium]
Can this max_active safety limit be bypassed via sysfs?

The sysfs max_active attribute uses wq_clamp_max_active(), which does not
recognize WQ_RTPRI and broadly clamps the value to 512 (WQ_MAX_ACTIVE).

Does this allow a privileged user to overwrite this initialization cap and
potentially cause RT scheduler starvation?

> +		wq->unbound_attrs->affn_scope = WQ_AFFN_CPU;
> +		wq->unbound_attrs->affn_strict = true;

[Severity: High]
Does applying WQ_AFFN_CPU affinity deterministically fail to initialize due to
the global RT worker limit?

If WQ_AFFN_CPU is applied, apply_wqattrs_prepare() will loop over
for_each_possible_cpu() and create a distinct pool for every CPU.

Since total_rtpri_workers is globally capped to num_online_cpus() - 1, won't
attempting to sequentially spawn num_possible_cpus() workers deterministically
hit the hard limit and fail during initialization?

> +	} else if (flags & WQ_BH) {
>  		/*
>  		 * BH workqueues always share a single execution context per CPU
>  		 * and don't impose any max_active limit.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717090209.26931-1-tvrtko.ursulin@igalia.com?part=1