[PATCH] cgroup/dmem: Use size_t for try_charge() and uncharge() size

[PATCH] cgroup/dmem: Use size_t for try_charge() and uncharge() size

[hongfu.li]

From: Hongfu Li <hongfu.li@linux.dev>

The charge APIs currently accept the allocation size as u64, but every
caller passes a size_t value derived from drm_gem_object::size.
Using u64 is unnecessary and, on 32-bit architectures, may lead to
truncation when the value is later cast to unsigned long for internal
page_counter functions.

Change the public function signatures to take size_t instead of u64.
Inside the implementation, pass the size directly to
page_counter_try_charge() and page_counter_uncharge(), which already
expect unsigned long.

Signed-off-by: Hongfu Li <hongfu.li@linux.dev>
---
 include/linux/cgroup_dmem.h | 8 ++++----
 kernel/cgroup/dmem.c        | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/linux/cgroup_dmem.h b/include/linux/cgroup_dmem.h
index dd4869f1d736..ed7440eb9c28 100644
--- a/include/linux/cgroup_dmem.h
+++ b/include/linux/cgroup_dmem.h
@@ -17,10 +17,10 @@ struct dmem_cgroup_region;
 #if IS_ENABLED(CONFIG_CGROUP_DMEM)
 struct dmem_cgroup_region *dmem_cgroup_register_region(u64 size, const char *name_fmt, ...) __printf(2,3);
 void dmem_cgroup_unregister_region(struct dmem_cgroup_region *region);
-int dmem_cgroup_try_charge(struct dmem_cgroup_region *region, u64 size,
+int dmem_cgroup_try_charge(struct dmem_cgroup_region *region, size_t size,
 			   struct dmem_cgroup_pool_state **ret_pool,
 			   struct dmem_cgroup_pool_state **ret_limit_pool);
-void dmem_cgroup_uncharge(struct dmem_cgroup_pool_state *pool, u64 size);
+void dmem_cgroup_uncharge(struct dmem_cgroup_pool_state *pool, size_t size);
 bool dmem_cgroup_state_evict_valuable(struct dmem_cgroup_pool_state *limit_pool,
 				      struct dmem_cgroup_pool_state *test_pool,
 				      bool ignore_low, bool *ret_hit_low);
@@ -36,7 +36,7 @@ dmem_cgroup_register_region(u64 size, const char *name_fmt, ...)
 static inline void dmem_cgroup_unregister_region(struct dmem_cgroup_region *region)
 { }
 
-static inline int dmem_cgroup_try_charge(struct dmem_cgroup_region *region, u64 size,
+static inline int dmem_cgroup_try_charge(struct dmem_cgroup_region *region, size_t size,
 					 struct dmem_cgroup_pool_state **ret_pool,
 					 struct dmem_cgroup_pool_state **ret_limit_pool)
 {
@@ -48,7 +48,7 @@ static inline int dmem_cgroup_try_charge(struct dmem_cgroup_region *region, u64
 	return 0;
 }
 
-static inline void dmem_cgroup_uncharge(struct dmem_cgroup_pool_state *pool, u64 size)
+static inline void dmem_cgroup_uncharge(struct dmem_cgroup_pool_state *pool, size_t size)
 { }
 
 static inline
diff --git a/kernel/cgroup/dmem.c b/kernel/cgroup/dmem.c
index 4753a67d0f0f..38108536d2d6 100644
--- a/kernel/cgroup/dmem.c
+++ b/kernel/cgroup/dmem.c
@@ -619,7 +619,7 @@ get_cg_pool_unlocked(struct dmemcg_state *cg, struct dmem_cgroup_region *region)
  * Must be called with the returned pool as argument,
  * and same @index and @size.
  */
-void dmem_cgroup_uncharge(struct dmem_cgroup_pool_state *pool, u64 size)
+void dmem_cgroup_uncharge(struct dmem_cgroup_pool_state *pool, size_t size)
 {
 	if (!pool)
 		return;
@@ -649,7 +649,7 @@ EXPORT_SYMBOL_GPL(dmem_cgroup_uncharge);
  *
  * Return: 0 on success, -EAGAIN on hitting a limit, or a negative errno on failure.
  */
-int dmem_cgroup_try_charge(struct dmem_cgroup_region *region, u64 size,
+int dmem_cgroup_try_charge(struct dmem_cgroup_region *region, size_t size,
 			  struct dmem_cgroup_pool_state **ret_pool,
 			  struct dmem_cgroup_pool_state **ret_limit_pool)
 {
-- 
2.54.0

Re: [PATCH] cgroup/dmem: Use size_t for try_charge() and uncharge() size

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] dmem cgroup controller incorrectly tracks bytes using page_counter, causing 2GB limit overflow on 32-bit architectures
- [High] Concurrent limit updates can corrupt page counter limits due to lack of serialization
- [High] Missing release barrier when publishing pool initialization allows bypassing hierarchical limits
--

commit 265d8a6cb2da9910a15e7066ffcacbb4c27c4ff0
Author: Hongfu Li <hongfu.li@linux.dev>
Subject: cgroup/dmem: Use size_t for try_charge() and uncharge() size

This commit changes the charge and uncharge APIs to accept a size_t value
for the allocation size rather than a u64. This prevents potential truncation
on 32-bit architectures when the size is passed to the internal page_counter
functions.

> diff --git a/kernel/cgroup/dmem.c b/kernel/cgroup/dmem.c
> index 4753a67d0f0f2..38108536d2d60 100644
> --- a/kernel/cgroup/dmem.c
> +++ b/kernel/cgroup/dmem.c
> @@ -649,7 +649,7 @@ EXPORT_SYMBOL_GPL(dmem_cgroup_uncharge);
>   *
>   * Return: 0 on success, -EAGAIN on hitting a limit, or a negative errno on failure.
>   */
> -int dmem_cgroup_try_charge(struct dmem_cgroup_region *region, u64 size,
> +int dmem_cgroup_try_charge(struct dmem_cgroup_region *region, size_t size,
>  			  struct dmem_cgroup_pool_state **ret_pool,
>  			  struct dmem_cgroup_pool_state **ret_limit_pool)
>  {

[ ... ]

>  	if (!page_counter_try_charge(&pool->cnt, size, &fail)) {

[Severity: High]
This is a pre-existing issue, but does using page_counter to track device
memory in bytes cause cumulative overflow on 32-bit architectures?

The page_counter API stores counts using unsigned long and internally
accumulates using atomic_long_add_return(). On 32-bit architectures, this
limits the tracking capability to LONG_MAX (2GB).

When cumulative device memory exceeds 2GB, the usage value wraps to negative,
and the signed bounds check in page_counter_try_charge() consistently fails,
causing valid allocations to be permanently rejected.

Similarly, in set_resource_max() and reset_all_resource_limits(), passing a
64-bit byte limit directly to page_counter_set_max() silently truncates
values above 4GB, and setting the default limit to PAGE_COUNTER_MAX restricts
usage to 2GB on 32-bit systems.

Could we track memory in pages instead of bytes to avoid this overflow?


[Severity: High]
This is a pre-existing issue, but can concurrent limit updates corrupt the
page counter limits?

page_counter_set_max() explicitly requires callers to serialize invocations.
Without serialization, concurrent writes can cause the optimistic
compare-and-swap retry loop to interleave, silently corrupting the maximum
limits and overwriting new limits with stale values.

This happens in dmemcg_limit_write() when applying limits:

kernel/cgroup/dmem.c:dmemcg_limit_write() {
    ...
		/* And commit */
		apply(pool, new_limit);
    ...
}

It also happens during cgroup offline when resetting limits under just an
RCU read lock:

kernel/cgroup/dmem.c:dmemcs_offline() {
    ...
	list_for_each_entry_rcu(pool, &dmemcs->pools, css_node)
		reset_all_resource_limits(pool);
    ...
}

Does this code need a lock to serialize these limit updates?


[Severity: High]
This is a pre-existing issue, but can the lack of memory barriers during pool
initialization allow hierarchical limits to be bypassed?

In alloc_pool_single(), a new pool is linked into the RCU list before it is
fully initialized. Later in get_cg_pool_locked(), pool->inited is set without
a release barrier:

kernel/cgroup/dmem.c:get_cg_pool_locked() {
    ...
		pool->cnt.parent = &ppool->cnt;
		if (ppool && !pool->parent) {
			pool->parent = ppool;
			dmemcg_pool_get(ppool);
		}
		pool->inited = true;
    ...
}

A concurrent reader in get_cg_pool_unlocked() uses READ_ONCE(pool->inited)
without an acquire barrier:

kernel/cgroup/dmem.c:get_cg_pool_unlocked() {
    ...
	pool = find_cg_pool_locked(cg, region);
	if (pool && !READ_ONCE(pool->inited))
		pool = NULL;
    ...
}

Due to memory reordering, the reader can observe pool->inited == true but
still see a NULL parent pointer. Would adding smp_store_release() and
smp_load_acquire() prevent this race?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626081231.47464-1-hongfu.li@linux.dev?part=1