[PATCH 1/2] cgroup-v1: Fix missing mutex_unlock in error paths

[PATCH 1/2] cgroup-v1: Fix missing mutex_unlock in error paths

[Zijiang Huang]

In the function, after acquiring the mutex with mutex_lock, multiple return
paths (such as returning ERR_PTR, NULL, or normal pointers)fail to call
mutex_unlock to release the lock, which could lead to deadlock risks.

Signed-off-by: Zijiang Huang <kerayhuang@tencent.com>
Reviewed-by: Hao Peng <flyingpeng@tencent.com>
---
 kernel/cgroup/cgroup-v1.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c
index fa24c032ed6f..73e0fd93111a 100644
--- a/kernel/cgroup/cgroup-v1.c
+++ b/kernel/cgroup/cgroup-v1.c
@@ -423,8 +423,10 @@ static void *cgroup_pidlist_start(struct seq_file *s, loff_t *pos)
 	 */
 	if (!ctx->procs1.pidlist) {
 		ret = pidlist_array_load(cgrp, type, &ctx->procs1.pidlist);
-		if (ret)
+		if (ret) {
+			mutex_unlock(&cgrp->pidlist_mutex);
 			return ERR_PTR(ret);
+		}
 	}
 	l = ctx->procs1.pidlist;
 
@@ -443,11 +445,14 @@ static void *cgroup_pidlist_start(struct seq_file *s, loff_t *pos)
 		}
 	}
 	/* If we're off the end of the array, we're done */
-	if (index >= l->length)
+	if (index >= l->length) {
+		mutex_unlock(&cgrp->pidlist_mutex);
 		return NULL;
+	}
 	/* Update the abstract position to be the actual pid that we found */
 	iter = l->list + index;
 	*pos = *iter;
+	mutex_unlock(&cgrp->pidlist_mutex);
 	return iter;
 }
 
-- 
2.43.5

[PATCH 2/2] cgroup: Fix reference count leak when cft->open is NULL

[Zijiang Huang]

When cft->open is NULL, it will cause ctx namespace reference count leak.

Signed-off-by: Zijiang Huang <kerayhuang@tencent.com>
Reviewed-by: Hao Peng <flyingpeng@tencent.com>
---
 kernel/cgroup/cgroup.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index a723b7dc6e4e..9bde0f4be687 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -4134,8 +4134,10 @@ static int cgroup_file_open(struct kernfs_open_file *of)
 	get_cgroup_ns(ctx->ns);
 	of->priv = ctx;
 
-	if (!cft->open)
+	if (!cft->open) {
+		get_cgroup_ns(ctx->ns);
 		return 0;
+	}
 
 	ret = cft->open(of);
 	if (ret) {
-- 
2.43.5

Re: [PATCH 1/2] cgroup-v1: Fix missing mutex_unlock in error paths

[Markus Elfring]

> In the function, after acquiring the mutex with mutex_lock, multiple return
> paths (such as returning ERR_PTR, NULL, or normal pointers)fail to call
> mutex_unlock to release the lock, which could lead to deadlock risks.

How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?

See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.16-rc6#n94


Will a cover letter become helpful for such a patch series?

Regards,
Markus

Re: [PATCH 2/2] cgroup: Fix reference count leak when cft->open is NULL

[Markus Elfring]

> When cft->open is NULL, it will cause ctx namespace reference count leak.

Would you like to point out that a reference count should be incremented
in another case?

Regards,
Markus

Re: [PATCH 1/2] cgroup-v1: Fix missing mutex_unlock in error paths

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 684 bytes --]

On Fri, Jul 18, 2025 at 07:54:08PM +0800, Zijiang Huang <huangzjsmile@gmail.com> wrote:
> In the function, after acquiring the mutex with mutex_lock, multiple return
> paths (such as returning ERR_PTR, NULL, or normal pointers)fail to call
> mutex_unlock to release the lock, which could lead to deadlock risks.

You've almost convinced me that you've found a case for the scoped
locks. However, this patch doesn't look correct -- the lock is released
in cgroup_pidlist_stop() + the comment in fs/kernfs/file.c:

 * As kernfs_seq_stop() is also called after kernfs_seq_start() or
 * kernfs_seq_next() failure,

Or have you found a path where the pairing is unmatched?

Thanks,
Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/2] cgroup: Fix reference count leak when cft->open is NULL

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 694 bytes --]

On Fri, Jul 18, 2025 at 07:54:09PM +0800, Zijiang Huang <huangzjsmile@gmail.com> wrote:
> @@ -4134,8 +4134,10 @@ static int cgroup_file_open(struct kernfs_open_file *of)
>  	get_cgroup_ns(ctx->ns);
>  	of->priv = ctx;
>  
> -	if (!cft->open)
> +	if (!cft->open) {
> +		get_cgroup_ns(ctx->ns);
>  		return 0;
> +	}
>  
>  	ret = cft->open(of);
>  	if (ret) {

1) You wanted to call put_cgroup_ns() instead of get_cgroup_ns()
2) The refernce needs to be kept during the whole lifetime of
   cgroup_file_ctx, this return path still leads to a valid ctx, so it's
   released in cgroup_file_release().

Or could you decribe more how could a release be missed?

Thanks,
Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]