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 <[email protected]> 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/[email protected]?part=1
