From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: Re: [PATCH] cgroup/dmem: return -ENOMEM on failed pool preallocation
Date: Sat, 16 May 2026 15:56:05 +1000
Message-ID: <review-patch1-c6b89ff53b603350fe53b0d47f3399ed@kernel.org>
In-Reply-To: <c6b89ff53b603350fe53b0d47f3399ed@kernel.org>
References: <c6b89ff53b603350fe53b0d47f3399ed@kernel.org> <c6b89ff53b603350fe53b0d47f3399ed@kernel.org>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Correctness: Correct.**

Looking at the function flow (lines 585=E2=80=93606 in the current tree):

```c
while (!pool) {
    spin_lock(&dmemcg_lock);
    if (!region->unregistered)
        pool =3D get_cg_pool_locked(cg, region, &allocpool);
    else
        pool =3D ERR_PTR(-ENODEV);
    ...
    spin_unlock(&dmemcg_lock);

    if (pool =3D=3D ERR_PTR(-ENOMEM)) {
        pool =3D NULL;
        if (WARN_ON(allocpool))
            continue;

        allocpool =3D kzalloc_obj(*allocpool);
        if (allocpool) {
            pool =3D NULL;
            continue;
        }
        // <--- BUG: pool is NULL here, loop retries forever
    }
}
```

When `kzalloc_obj()` returns NULL (allocation failure), execution falls thr=
ough the `if (allocpool)` block without setting `pool`. Since `pool` was se=
t to NULL at line 596 (`pool =3D NULL`), the `while (!pool)` condition is s=
till true, and the loop retries. But `allocpool` is also still NULL, so the=
 next iteration will call `get_cg_pool_locked()` without a preallocated poo=
l, which will again return `ERR_PTR(-ENOMEM)`, and the cycle repeats =E2=80=
=94 an infinite loop under memory pressure, exactly when you'd least want i=
t.

The fix:

```c
        allocpool =3D kzalloc_obj(*allocpool);
        if (allocpool) {
            pool =3D NULL;
            continue;
        }
+       pool =3D ERR_PTR(-ENOMEM);
    }
```

This causes the `while (!pool)` loop to terminate (since `ERR_PTR(-ENOMEM)`=
 is non-NULL), and `pool` is returned to the caller. Since callers already =
check for `IS_ERR()` returns (the `-ENODEV` path at line 590 already sets a=
n ERR_PTR), this is consistent with the existing error contract.

**Minor observations:**

1. The `Fixes:` tag references the correct commit that introduced the dmem =
cgroup code.
2. The `kfree(allocpool)` at line 608 is safe since `allocpool` is NULL in =
this error path.
3. One could argue that `GFP_KERNEL` allocation failure should perhaps use =
`__GFP_NOFAIL` or retry with `GFP_NOIO` depending on context, but simply pr=
opagating the error is the safest and most standard approach, and matches h=
ow `get_cg_pool_locked` already signals `-ENOMEM`.

**No issues found. Clean, minimal, correct fix.**

---
Generated by Claude Code Patch Reviewer