cgroup/dmem: Don't clobber pool in dmem_cgroup_calculate_protection

Commit Message

Friedrich Vock Jan. 14, 2025, 3:39 p.m. UTC

If the current css doesn't contain any pool that is a descendant of
the "pool" (i.e. when found_descendant == false), then "pool" will
point to some unrelated pool. If the current css has a child, we'll
overwrite parent_pool with this unrelated pool on the next iteration.

Fix this by overwriting "pool" only if it actually is a descendant of
parent_pool, and setting it to NULL otherwise. Also, skip traversing
subtrees if pool == NULL to avoid overwriting parent_pool (and because
it's pointless).

Fixes: b168ed458 ("kernel/cgroup: Add "dmem" memory accounting cgroup")
Signed-off-by: Friedrich Vock <[e-mail]>
---
 kernel/cgroup/dmem.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

--
2.48.0

Comments

Michal Koutný Jan. 14, 2025, 3:58 p.m. UTC | #1

On Tue, Jan 14, 2025 at 04:39:12PM +0100, Friedrich Vock <[e-mail]> wrote:
> If the current css doesn't contain any pool that is a descendant of
> the "pool" (i.e. when found_descendant == false), then "pool" will
> point to some unrelated pool. If the current css has a child, we'll
> overwrite parent_pool with this unrelated pool on the next iteration.

Could this be verified with more idiomatic way with
cgroup_is_descendant()? (The predicate could be used between pools [1]
if they pin respective cgroups).

Thanks,
Michal

[1] https://lore.kernel.org/all/uj6railxyazpu6ocl2ygo6lw4lavbsgg26oq57pxxqe5uzxw42@fhnqvq3tia6n/

Patch

diff --git a/kernel/cgroup/dmem.c b/kernel/cgroup/dmem.c
index 52736ef0ccf25..10d37df5d50f6 100644
--- a/kernel/cgroup/dmem.c
+++ b/kernel/cgroup/dmem.c
@@ -222,8 +222,7 @@  dmem_cgroup_calculate_protection(struct dmem_cgroup_pool_state *limit_pool,
 	struct page_counter *climit;
 	struct cgroup_subsys_state *css, *next_css;
 	struct dmemcg_state *dmemcg_iter;
-	struct dmem_cgroup_pool_state *pool, *parent_pool;
-	bool found_descendant;
+	struct dmem_cgroup_pool_state *pool, *candidate_pool, *parent_pool;

 	climit = &limit_pool->cnt;

@@ -241,7 +240,13 @@  dmem_cgroup_calculate_protection(struct dmem_cgroup_pool_state *limit_pool,
 	 */
 	while (pool != test_pool) {
 		next_css = css_next_child(NULL, css);
-		if (next_css) {
+		/*
+		 * pool is NULL when the current css does not contain a
+		 * pool of the type we're interested in. In that case, it's
+		 * impossible that any child css contains a relevant pool, so
+		 * skip the subtree entirely and move on to the next sibling.
+		 */
+		if (next_css && pool) {
 			parent_pool = pool;
 		} else {
 			while (css != &limit_pool->cs->css) {
@@ -260,16 +265,16 @@  dmem_cgroup_calculate_protection(struct dmem_cgroup_pool_state *limit_pool,
 		}
 		css = next_css;

-		found_descendant = false;
 		dmemcg_iter = container_of(css, struct dmemcg_state, css);

-		list_for_each_entry_rcu(pool, &dmemcg_iter->pools, css_node) {
-			if (pool_parent(pool) == parent_pool) {
-				found_descendant = true;
+		pool = NULL;
+		list_for_each_entry_rcu(candidate_pool, &dmemcg_iter->pools, css_node) {
+			if (pool_parent(candidate_pool) == parent_pool) {
+				pool = candidate_pool;
 				break;
 			}
 		}
-		if (!found_descendant)
+		if (!pool)
 			continue;

 		page_counter_calculate_protection(