Re: [PATCH v8 3/6] cgroup/dmem: Add reclaim callback for lowering max below current usage

[sashiko-bot@kernel.org]

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 <thomas.hellstrom@linux.intel.com>

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/20260723100350.16895-1-thomas.hellstrom@linux.intel.com?part=3