Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Unbounded SRCU read lock hold time in set_resource_max() can stall unregistration indefinitely Pre-existing issues: - [High] 32-bit integer overflow and truncation in dmem cgroup limits and accounting -- --- Patch [3]: [PATCH v8 3/6] cgroup/dmem: Add reclaim callback for lowering max below current usage --- commit 1477e3308e9932d9b9ab70b4c6666ef7191157d5 Author: Thomas Hellström <[email protected]> cgroup/dmem: Add reclaim callback for lowering max below current usage This commit adds an optional reclaim callback to dmem_cgroup_region. When dmem.max is lowered below current usage, the driver is asked to evict memory, and concurrent allocations are throttled. It introduces an SRCU domain to serialize these reclaim callbacks against region unregistration. > diff --git a/kernel/cgroup/dmem.c b/kernel/cgroup/dmem.c > index eb46e99b73666..7e427895f23ea 100644 > --- a/kernel/cgroup/dmem.c > +++ b/kernel/cgroup/dmem.c [ ... ] > @@ -145,21 +161,52 @@ static void free_cg_pool(struct dmem_cgroup_pool_state > *pool) > } > > static void > -set_resource_max(struct dmem_cgroup_pool_state *pool, u64 val) > +set_resource_max(struct dmem_cgroup_pool_state *pool, u64 val, bool nonblock) > { > - page_counter_set_max(&pool->cnt, val); > + struct dmem_cgroup_region *region = pool->region; > + unsigned long limit = (unsigned long)val; [Severity: High] This isn't a bug introduced by this patch, but does this truncation break cgroup limits on 32-bit systems? The dmem subsystem appears to be tracking bytes rather than pages using the page_counter API. On 32-bit kernels, an unsigned long is 32 bits, which means any limit or usage exceeding 4GB will be silently truncated when a u64 is cast to unsigned long in set_resource_max(). This same issue seems to apply in dmem_cgroup_try_charge() where the u64 size argument is passed to page_counter_try_charge(): kernel/cgroup/dmem.c:dmem_cgroup_try_charge() { ... if (!page_counter_try_charge(&pool->cnt, size, &fail)) { ... } If the allocation exceeds 4GB, or if the total usage tracked in the atomic_long_t exceeds 4GB, would this cause the counter to wrap and silently bypass the limits? > + > + /* Apply the new limit immediately so concurrent allocations are > throttled. */ > + xchg(&pool->cnt.max, limit); > + > + if (nonblock) > + return; > + > + int srcu_idx = srcu_read_lock(&dmemcg_srcu); > + > + if (!READ_ONCE(region->unregistered) && region->ops && > region->ops->reclaim) { > + for (int retries = DMEM_MAX_RECLAIM_RETRIES; ; ) { > + u64 usage = page_counter_read(&pool->cnt); > + int ret; > + > + if (usage <= limit) > + break; > + > + if (signal_pending(current)) > + break; > + > + ret = region->ops->reclaim(pool, usage - limit, > region->reclaim_priv); > + > + /* -ENOSPC means no progress; other errors are fatal. */ > + if (ret && (ret != -ENOSPC || !retries--)) > + break; > + > + cond_resched(); > + } > + } > + srcu_read_unlock(&dmemcg_srcu, srcu_idx); > } [Severity: High] Can this loop stall indefinitely and block region unregistration? In set_resource_max(), if region->ops->reclaim() continually successfully frees some memory but not enough to fall below the limit, it returns 0. When it returns 0, the retries counter is not decremented: if (ret && (ret != -ENOSPC || !retries--)) break; Since srcu_read_lock(&dmemcg_srcu) is held for the entire duration of this retry loop, an infinite loop here would indefinitely hold the SRCU read lock. If a cgroup administrator sets dmem.max below current usage while concurrent processes continually allocate memory, could this block the synchronize_srcu() call in dmem_cgroup_unregister_region() forever, hanging the system during device hot-unplug or module unload? Would it be safer to periodically drop and reacquire the SRCU read lock inside the loop, or limit the total number of retries regardless of the return code? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
