Re: 5.10.225 stable kernel cgroup_mutex not held assertion failure

Re: 5.10.225 stable kernel cgroup_mutex not held assertion failure

[Siddh Raman Pant]

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

Hello maintainers,

On Fri, 20 Sep 2024 02:28:03 -0700, Shivani Agarwal wrote:
> Thanks Fedor.
> 
> Upstream commit 1be59c97c83c is merged in 5.4 with commit 10aeaa47e4aa and
> in 4.19 with commit 27d6dbdc6485. The issue is reproducible in 5.4 and 4.19
> also.
> 
> I am sending the backport patch of d23b5c577715 and a7fb0423c201 for 5.4 and
> 4.19 in the next email.

Please backport these changes to stable.

"cgroup/cpuset: Prevent UAF in proc_cpuset_show()" has already been
backported and bears CVE-2024-43853. As reported, we may already have
introduced another problem due to the missing backport.

Thanks,
Siddh

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: 5.10.225 stable kernel cgroup_mutex not held assertion failure

[gregkh]

On Wed, Oct 30, 2024 at 07:29:38AM +0000, Siddh Raman Pant wrote:
> Hello maintainers,
> 
> On Fri, 20 Sep 2024 02:28:03 -0700, Shivani Agarwal wrote:
> > Thanks Fedor.
> > 
> > Upstream commit 1be59c97c83c is merged in 5.4 with commit 10aeaa47e4aa and
> > in 4.19 with commit 27d6dbdc6485. The issue is reproducible in 5.4 and 4.19
> > also.
> > 
> > I am sending the backport patch of d23b5c577715 and a7fb0423c201 for 5.4 and
> > 4.19 in the next email.
> 
> Please backport these changes to stable.
> 
> "cgroup/cpuset: Prevent UAF in proc_cpuset_show()" has already been
> backported and bears CVE-2024-43853. As reported, we may already have
> introduced another problem due to the missing backport.

What exact commits are needed here?  Please submit backported and tested
commits and we will be glad to queue them up.

thanks,

greg k-h

Re: 5.10.225 stable kernel cgroup_mutex not held assertion failure

[Siddh Raman Pant]

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

On Wed, Nov 06 2024 at 11:40:39 +0530, gregkh@linuxfoundation.org
wrote:
> On Wed, Oct 30, 2024 at 07:29:38AM +0000, Siddh Raman Pant wrote:
> > Hello maintainers,
> > 
> > On Fri, 20 Sep 2024 02:28:03 -0700, Shivani Agarwal wrote:
> > > Thanks Fedor.
> > > 
> > > Upstream commit 1be59c97c83c is merged in 5.4 with commit 10aeaa47e4aa and
> > > in 4.19 with commit 27d6dbdc6485. The issue is reproducible in 5.4 and 4.19
> > > also.
> > > 
> > > I am sending the backport patch of d23b5c577715 and a7fb0423c201 for 5.4 and
> > > 4.19 in the next email.
> > 
> > Please backport these changes to stable.
> > 
> > "cgroup/cpuset: Prevent UAF in proc_cpuset_show()" has already been
> > backported and bears CVE-2024-43853. As reported, we may already have
> > introduced another problem due to the missing backport.
> 
> What exact commits are needed here?  Please submit backported and tested
> commits and we will be glad to queue them up.
> 
> thanks,
> 
> greg k-h

Please see the following thread where Shivani posted the patches:

https://lore.kernel.org/all/20240920092803.101047-1-shivani.agarwal@broadcom.com/

Thanks,
Siddh

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: 5.10.225 stable kernel cgroup_mutex not held assertion failure

[Siddh Raman Pant]

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

On Wed, Nov 06 2024 at 11:54:32 +0530, Siddh Raman Pant wrote:
> On Wed, Nov 06 2024 at 11:40:39 +0530, gregkh@linuxfoundation.org
> wrote:
> > On Wed, Oct 30, 2024 at 07:29:38AM +0000, Siddh Raman Pant wrote:
> > > Hello maintainers,
> > > 
> > > On Fri, 20 Sep 2024 02:28:03 -0700, Shivani Agarwal wrote:
> > > > Thanks Fedor.
> > > > 
> > > > Upstream commit 1be59c97c83c is merged in 5.4 with commit 10aeaa47e4aa and
> > > > in 4.19 with commit 27d6dbdc6485. The issue is reproducible in 5.4 and 4.19
> > > > also.
> > > > 
> > > > I am sending the backport patch of d23b5c577715 and a7fb0423c201 for 5.4 and
> > > > 4.19 in the next email.
> > > 
> > > Please backport these changes to stable.
> > > 
> > > "cgroup/cpuset: Prevent UAF in proc_cpuset_show()" has already been
> > > backported and bears CVE-2024-43853. As reported, we may already have
> > > introduced another problem due to the missing backport.
> > 
> > What exact commits are needed here?  Please submit backported and tested
> > commits and we will be glad to queue them up.
> > 
> > thanks,
> > 
> > greg k-h
> 
> Please see the following thread where Shivani posted the patches:
> 
> https://lore.kernel.org/all/20240920092803.101047-1-shivani.agarwal@broadcom.com/
> 
> Thanks,
> Siddh

Ping...

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: 5.10.225 stable kernel cgroup_mutex not held assertion failure

[gregkh]

On Wed, Nov 20, 2024 at 02:46:32PM +0000, Siddh Raman Pant wrote:
> On Wed, Nov 06 2024 at 11:54:32 +0530, Siddh Raman Pant wrote:
> > On Wed, Nov 06 2024 at 11:40:39 +0530, gregkh@linuxfoundation.org
> > wrote:
> > > On Wed, Oct 30, 2024 at 07:29:38AM +0000, Siddh Raman Pant wrote:
> > > > Hello maintainers,
> > > > 
> > > > On Fri, 20 Sep 2024 02:28:03 -0700, Shivani Agarwal wrote:
> > > > > Thanks Fedor.
> > > > > 
> > > > > Upstream commit 1be59c97c83c is merged in 5.4 with commit 10aeaa47e4aa and
> > > > > in 4.19 with commit 27d6dbdc6485. The issue is reproducible in 5.4 and 4.19
> > > > > also.
> > > > > 
> > > > > I am sending the backport patch of d23b5c577715 and a7fb0423c201 for 5.4 and
> > > > > 4.19 in the next email.
> > > > 
> > > > Please backport these changes to stable.
> > > > 
> > > > "cgroup/cpuset: Prevent UAF in proc_cpuset_show()" has already been
> > > > backported and bears CVE-2024-43853. As reported, we may already have
> > > > introduced another problem due to the missing backport.
> > > 
> > > What exact commits are needed here?  Please submit backported and tested
> > > commits and we will be glad to queue them up.
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > 
> > Please see the following thread where Shivani posted the patches:
> > 
> > https://lore.kernel.org/all/20240920092803.101047-1-shivani.agarwal@broadcom.com/
> > 
> > Thanks,
> > Siddh
> 
> Ping...

I don't understand what you want here, sorry.

greg k-h

Re: 5.10.225 stable kernel cgroup_mutex not held assertion failure

[Siddh Raman Pant]

[-- Attachment #1.1: Type: text/plain, Size: 1969 bytes --]

On Wed, Nov 20 2024 at 20:28:36 +0530, gregkh@linuxfoundation.org
wrote:
> On Wed, Nov 20, 2024 at 02:46:32PM +0000, Siddh Raman Pant wrote:
> > On Wed, Nov 06 2024 at 11:54:32 +0530, Siddh Raman Pant wrote:
> > > On Wed, Nov 06 2024 at 11:40:39 +0530, gregkh@linuxfoundation.org
> > > wrote:
> > > > On Wed, Oct 30, 2024 at 07:29:38AM +0000, Siddh Raman Pant wrote:
> > > > > Hello maintainers,
> > > > > 
> > > > > On Fri, 20 Sep 2024 02:28:03 -0700, Shivani Agarwal wrote:
> > > > > > Thanks Fedor.
> > > > > > 
> > > > > > Upstream commit 1be59c97c83c is merged in 5.4 with commit 10aeaa47e4aa and
> > > > > > in 4.19 with commit 27d6dbdc6485. The issue is reproducible in 5.4 and 4.19
> > > > > > also.
> > > > > > 
> > > > > > I am sending the backport patch of d23b5c577715 and a7fb0423c201 for 5.4 and
> > > > > > 4.19 in the next email.
> > > > > 
> > > > > Please backport these changes to stable.
> > > > > 
> > > > > "cgroup/cpuset: Prevent UAF in proc_cpuset_show()" has already been
> > > > > backported and bears CVE-2024-43853. As reported, we may already have
> > > > > introduced another problem due to the missing backport.
> > > > 
> > > > What exact commits are needed here?  Please submit backported and tested
> > > > commits and we will be glad to queue them up.
> > > > 
> > > > thanks,
> > > > 
> > > > greg k-h
> > > 
> > > Please see the following thread where Shivani posted the patches:
> > > 
> > > https://lore.kernel.org/all/20240920092803.101047-1-shivani.agarwal@broadcom.com/ 
> > > 
> > > Thanks,
> > > Siddh
> > 
> > Ping...
> 
> I don't understand what you want here, sorry.

Please find attached the patch emails for 5.4 with this email. They
apply cleanly to the linux-5.4.y branch.

I was earlier referring to the email thread where the fixed-up commits
for backporting were posted by someone else (i.e. I'm not the original
backporter / conflict fix author).

Thanks,
Siddh

[-- Attachment #1.2: PATCH-v5.4-cgroup-Move-rcu_head-up-near-the-top-of-cgroup_root.mbox --]
[-- Type: application/mbox, Size: 7761 bytes --]

[-- Attachment #1.3: PATCH-v5.4-cgroup-Make-operations-on-the-cgroup-root_list-RCU-safe.mbox --]
[-- Type: application/mbox, Size: 9706 bytes --]

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: 5.10.225 stable kernel cgroup_mutex not held assertion failure

[Siddh Raman Pant]

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

On Wed, Nov 20 2024 at 23:17:28 +0530, Siddh Raman Pant wrote:
> On Wed, Nov 20 2024 at 20:28:36 +0530, gregkh@linuxfoundation.org
> wrote:
> > On Wed, Nov 20, 2024 at 02:46:32PM +0000, Siddh Raman Pant wrote:
> > > On Wed, Nov 06 2024 at 11:54:32 +0530, Siddh Raman Pant wrote:
> > > > On Wed, Nov 06 2024 at 11:40:39 +0530, gregkh@linuxfoundation.org
> > > > wrote:
> > > > > On Wed, Oct 30, 2024 at 07:29:38AM +0000, Siddh Raman Pant wrote:
> > > > > > Hello maintainers,
> > > > > > 
> > > > > > On Fri, 20 Sep 2024 02:28:03 -0700, Shivani Agarwal wrote:
> > > > > > > Thanks Fedor.
> > > > > > > 
> > > > > > > Upstream commit 1be59c97c83c is merged in 5.4 with commit 10aeaa47e4aa and
> > > > > > > in 4.19 with commit 27d6dbdc6485. The issue is reproducible in 5.4 and 4.19
> > > > > > > also.
> > > > > > > 
> > > > > > > I am sending the backport patch of d23b5c577715 and a7fb0423c201 for 5.4 and
> > > > > > > 4.19 in the next email.
> > > > > > 
> > > > > > Please backport these changes to stable.
> > > > > > 
> > > > > > "cgroup/cpuset: Prevent UAF in proc_cpuset_show()" has already been
> > > > > > backported and bears CVE-2024-43853. As reported, we may already have
> > > > > > introduced another problem due to the missing backport.
> > > > > 
> > > > > What exact commits are needed here?  Please submit backported and tested
> > > > > commits and we will be glad to queue them up.
> > > > > 
> > > > > thanks,
> > > > > 
> > > > > greg k-h
> > > > 
> > > > Please see the following thread where Shivani posted the patches:
> > > > 
> > > > https://lore.kernel.org/all/20240920092803.101047-1-shivani.agarwal@broadcom.com/ 
> > > > 
> > > > Thanks,
> > > > Siddh
> > > 
> > > Ping...
> > 
> > I don't understand what you want here, sorry.
> 
> Please find attached the patch emails for 5.4 with this email. They
> apply cleanly to the linux-5.4.y branch.

Ping?

To be clear again - I want the patches I had posted in the previous
email to be applied to 5.4.y branch.

Thanks,
Siddh

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: 5.10.225 stable kernel cgroup_mutex not held assertion failure

[gregkh]

On Wed, Nov 20, 2024 at 05:47:36PM +0000, Siddh Raman Pant wrote:
> On Wed, Nov 20 2024 at 20:28:36 +0530, gregkh@linuxfoundation.org
> wrote:
> > On Wed, Nov 20, 2024 at 02:46:32PM +0000, Siddh Raman Pant wrote:
> > > On Wed, Nov 06 2024 at 11:54:32 +0530, Siddh Raman Pant wrote:
> > > > On Wed, Nov 06 2024 at 11:40:39 +0530, gregkh@linuxfoundation.org
> > > > wrote:
> > > > > On Wed, Oct 30, 2024 at 07:29:38AM +0000, Siddh Raman Pant wrote:
> > > > > > Hello maintainers,
> > > > > > 
> > > > > > On Fri, 20 Sep 2024 02:28:03 -0700, Shivani Agarwal wrote:
> > > > > > > Thanks Fedor.
> > > > > > > 
> > > > > > > Upstream commit 1be59c97c83c is merged in 5.4 with commit 10aeaa47e4aa and
> > > > > > > in 4.19 with commit 27d6dbdc6485. The issue is reproducible in 5.4 and 4.19
> > > > > > > also.
> > > > > > > 
> > > > > > > I am sending the backport patch of d23b5c577715 and a7fb0423c201 for 5.4 and
> > > > > > > 4.19 in the next email.
> > > > > > 
> > > > > > Please backport these changes to stable.
> > > > > > 
> > > > > > "cgroup/cpuset: Prevent UAF in proc_cpuset_show()" has already been
> > > > > > backported and bears CVE-2024-43853. As reported, we may already have
> > > > > > introduced another problem due to the missing backport.
> > > > > 
> > > > > What exact commits are needed here?  Please submit backported and tested
> > > > > commits and we will be glad to queue them up.
> > > > > 
> > > > > thanks,
> > > > > 
> > > > > greg k-h
> > > > 
> > > > Please see the following thread where Shivani posted the patches:
> > > > 
> > > > https://lore.kernel.org/all/20240920092803.101047-1-shivani.agarwal@broadcom.com/ 
> > > > 
> > > > Thanks,
> > > > Siddh
> > > 
> > > Ping...
> > 
> > I don't understand what you want here, sorry.
> 
> Please find attached the patch emails for 5.4 with this email. They
> apply cleanly to the linux-5.4.y branch.

Please resend these as patches, in the correct order, not as attachments
as it's hard to review and handle them this way.

thanks,

greg k-h

[PATCH 1/2] cgroup: Make operations on the cgroup root_list RCU safe

[Siddh Raman Pant]

From: Yafang Shao <laoar.shao@gmail.com>

commit d23b5c577715892c87533b13923306acc6243f93 upstream.

At present, when we perform operations on the cgroup root_list, we must
hold the cgroup_mutex, which is a relatively heavyweight lock. In reality,
we can make operations on this list RCU-safe, eliminating the need to hold
the cgroup_mutex during traversal. Modifications to the list only occur in
the cgroup root setup and destroy paths, which should be infrequent in a
production environment. In contrast, traversal may occur frequently.
Therefore, making it RCU-safe would be beneficial.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Signed-off-by: Tejun Heo <tj@kernel.org>
[fp: adapt to 5.10 mainly because of changes made by e210a89f5b07
 ("cgroup.c: add helper __cset_cgroup_from_root to cleanup duplicated
 codes")]
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
[Shivani: Modified to apply on v5.4.y]
Signed-off-by: Shivani Agarwal <shivani.agarwal@broadcom.com>
Reviewed-by: Siddh Raman Pant <siddh.raman.pant@oracle.com>
Signed-off-by: Siddh Raman Pant <siddh.raman.pant@oracle.com>
---
 include/linux/cgroup-defs.h     |  1 +
 kernel/cgroup/cgroup-internal.h |  3 ++-
 kernel/cgroup/cgroup.c          | 23 ++++++++++++++++-------
 3 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index d15884957e7f..c64f11674850 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -517,6 +517,7 @@ struct cgroup_root {
 
 	/* A list running through the active hierarchies */
 	struct list_head root_list;
+	struct rcu_head rcu;
 
 	/* Hierarchy-specific flags */
 	unsigned int flags;
diff --git a/kernel/cgroup/cgroup-internal.h b/kernel/cgroup/cgroup-internal.h
index 803989eae99e..bb85acc1114e 100644
--- a/kernel/cgroup/cgroup-internal.h
+++ b/kernel/cgroup/cgroup-internal.h
@@ -172,7 +172,8 @@ extern struct list_head cgroup_roots;
 
 /* iterate across the hierarchies */
 #define for_each_root(root)						\
-	list_for_each_entry((root), &cgroup_roots, root_list)
+	list_for_each_entry_rcu((root), &cgroup_roots, root_list,	\
+				lockdep_is_held(&cgroup_mutex))
 
 /**
  * for_each_subsys - iterate all enabled cgroup subsystems
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 79e57b6df731..273a8a42cb72 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -1314,7 +1314,7 @@ void cgroup_free_root(struct cgroup_root *root)
 {
 	if (root) {
 		idr_destroy(&root->cgroup_idr);
-		kfree(root);
+		kfree_rcu(root, rcu);
 	}
 }
 
@@ -1348,7 +1348,7 @@ static void cgroup_destroy_root(struct cgroup_root *root)
 	spin_unlock_irq(&css_set_lock);
 
 	if (!list_empty(&root->root_list)) {
-		list_del(&root->root_list);
+		list_del_rcu(&root->root_list);
 		cgroup_root_count--;
 	}
 
@@ -1401,7 +1401,6 @@ static struct cgroup *cset_cgroup_from_root(struct css_set *cset,
 {
 	struct cgroup *res = NULL;
 
-	lockdep_assert_held(&cgroup_mutex);
 	lockdep_assert_held(&css_set_lock);
 
 	if (cset == &init_css_set) {
@@ -1421,13 +1420,23 @@ static struct cgroup *cset_cgroup_from_root(struct css_set *cset,
 		}
 	}
 
-	BUG_ON(!res);
+	/*
+	 * If cgroup_mutex is not held, the cgrp_cset_link will be freed
+	 * before we remove the cgroup root from the root_list. Consequently,
+	 * when accessing a cgroup root, the cset_link may have already been
+	 * freed, resulting in a NULL res_cgroup. However, by holding the
+	 * cgroup_mutex, we ensure that res_cgroup can't be NULL.
+	 * If we don't hold cgroup_mutex in the caller, we must do the NULL
+	 * check.
+	 */
 	return res;
 }
 
 /*
  * Return the cgroup for "task" from the given hierarchy. Must be
- * called with cgroup_mutex and css_set_lock held.
+ * called with css_set_lock held to prevent task's groups from being modified.
+ * Must be called with either cgroup_mutex or rcu read lock to prevent the
+ * cgroup root from being destroyed.
  */
 struct cgroup *task_cgroup_from_root(struct task_struct *task,
 				     struct cgroup_root *root)
@@ -2012,7 +2021,7 @@ void init_cgroup_root(struct cgroup_fs_context *ctx)
 	struct cgroup_root *root = ctx->root;
 	struct cgroup *cgrp = &root->cgrp;
 
-	INIT_LIST_HEAD(&root->root_list);
+	INIT_LIST_HEAD_RCU(&root->root_list);
 	atomic_set(&root->nr_cgrps, 1);
 	cgrp->root = root;
 	init_cgroup_housekeeping(cgrp);
@@ -2094,7 +2103,7 @@ int cgroup_setup_root(struct cgroup_root *root, u16 ss_mask)
 	 * care of subsystems' refcounts, which are explicitly dropped in
 	 * the failure exit path.
 	 */
-	list_add(&root->root_list, &cgroup_roots);
+	list_add_rcu(&root->root_list, &cgroup_roots);
 	cgroup_root_count++;
 
 	/*
-- 
2.45.2

[PATCH 2/2] cgroup: Move rcu_head up near the top of cgroup_root

[Siddh Raman Pant]

From: Waiman Long <longman@redhat.com>

commit a7fb0423c201ba12815877a0b5a68a6a1710b23a upstream.

Commit d23b5c577715 ("cgroup: Make operations on the cgroup root_list RCU
safe") adds a new rcu_head to the cgroup_root structure and kvfree_rcu()
for freeing the cgroup_root.

The current implementation of kvfree_rcu(), however, has the limitation
that the offset of the rcu_head structure within the larger data
structure must be less than 4096 or the compilation will fail. See the
macro definition of __is_kvfree_rcu_offset() in include/linux/rcupdate.h
for more information.

By putting rcu_head below the large cgroup structure, any change to the
cgroup structure that makes it larger run the risk of causing build
failure under certain configurations. Commit 77070eeb8821 ("cgroup:
Avoid false cacheline sharing of read mostly rstat_cpu") happens to be
the last straw that breaks it. Fix this problem by moving the rcu_head
structure up before the cgroup structure.

Fixes: d23b5c577715 ("cgroup: Make operations on the cgroup root_list RCU safe")
Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Closes: https://lore.kernel.org/lkml/20231207143806.114e0a74@canb.auug.org.au/
Signed-off-by: Waiman Long <longman@redhat.com>
Acked-by: Yafang Shao <laoar.shao@gmail.com>
Reviewed-by: Yosry Ahmed <yosryahmed@google.com>
Reviewed-by: Michal Koutný <mkoutny@suse.com>
Signed-off-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
[Shivani: Modified to apply on v5.4.y]
Signed-off-by: Shivani Agarwal <shivani.agarwal@broadcom.com>
Reviewed-by: Siddh Raman Pant <siddh.raman.pant@oracle.com>
Signed-off-by: Siddh Raman Pant <siddh.raman.pant@oracle.com>
---
 include/linux/cgroup-defs.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index c64f11674850..f0798d98be8e 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -506,6 +506,10 @@ struct cgroup_root {
 	/* Unique id for this hierarchy. */
 	int hierarchy_id;
 
+	/* A list running through the active hierarchies */
+	struct list_head root_list;
+	struct rcu_head rcu;
+
 	/* The root cgroup.  Root is destroyed on its release. */
 	struct cgroup cgrp;
 
@@ -515,10 +519,6 @@ struct cgroup_root {
 	/* Number of cgroups in the hierarchy, used only for /proc/cgroups */
 	atomic_t nr_cgrps;
 
-	/* A list running through the active hierarchies */
-	struct list_head root_list;
-	struct rcu_head rcu;
-
 	/* Hierarchy-specific flags */
 	unsigned int flags;
 
-- 
2.45.2

Re: [PATCH 1/2] cgroup: Make operations on the cgroup root_list RCU safe

[Siddh Raman Pant]

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

On Mon, Dec 02 2024 at 15:29:25 +0530, Siddh Raman Pant wrote:
> Reviewed-by: Siddh Raman Pant <siddh.raman.pant@oracle.com>
> Signed-off-by: Siddh Raman Pant <siddh.raman.pant@oracle.com>

Oops, please remove signed-off-by it was added by git format-patch
automatically.

Thanks,
Siddh

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] cgroup: Make operations on the cgroup root_list RCU safe

[gregkh]

On Mon, Dec 02, 2024 at 10:01:54AM +0000, Siddh Raman Pant wrote:
> On Mon, Dec 02 2024 at 15:29:25 +0530, Siddh Raman Pant wrote:
> > Reviewed-by: Siddh Raman Pant <siddh.raman.pant@oracle.com>
> > Signed-off-by: Siddh Raman Pant <siddh.raman.pant@oracle.com>
> 
> Oops, please remove signed-off-by it was added by git format-patch
> automatically.

Please fix things up so maintainers do not have to manually hand-edit
patches :(

[PATCH 1/2] cgroup: Make operations on the cgroup root_list RCU safe

[Siddh Raman Pant]

From: Yafang Shao <laoar.shao@gmail.com>

commit d23b5c577715892c87533b13923306acc6243f93 upstream.

At present, when we perform operations on the cgroup root_list, we must
hold the cgroup_mutex, which is a relatively heavyweight lock. In reality,
we can make operations on this list RCU-safe, eliminating the need to hold
the cgroup_mutex during traversal. Modifications to the list only occur in
the cgroup root setup and destroy paths, which should be infrequent in a
production environment. In contrast, traversal may occur frequently.
Therefore, making it RCU-safe would be beneficial.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Signed-off-by: Tejun Heo <tj@kernel.org>
[fp: adapt to 5.10 mainly because of changes made by e210a89f5b07
 ("cgroup.c: add helper __cset_cgroup_from_root to cleanup duplicated
 codes")]
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
[Shivani: Modified to apply on v5.4.y]
Signed-off-by: Shivani Agarwal <shivani.agarwal@broadcom.com>
Reviewed-by: Siddh Raman Pant <siddh.raman.pant@oracle.com>
---
 include/linux/cgroup-defs.h     |  1 +
 kernel/cgroup/cgroup-internal.h |  3 ++-
 kernel/cgroup/cgroup.c          | 23 ++++++++++++++++-------
 3 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index d15884957e7f..c64f11674850 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -517,6 +517,7 @@ struct cgroup_root {
 
 	/* A list running through the active hierarchies */
 	struct list_head root_list;
+	struct rcu_head rcu;
 
 	/* Hierarchy-specific flags */
 	unsigned int flags;
diff --git a/kernel/cgroup/cgroup-internal.h b/kernel/cgroup/cgroup-internal.h
index 803989eae99e..bb85acc1114e 100644
--- a/kernel/cgroup/cgroup-internal.h
+++ b/kernel/cgroup/cgroup-internal.h
@@ -172,7 +172,8 @@ extern struct list_head cgroup_roots;
 
 /* iterate across the hierarchies */
 #define for_each_root(root)						\
-	list_for_each_entry((root), &cgroup_roots, root_list)
+	list_for_each_entry_rcu((root), &cgroup_roots, root_list,	\
+				lockdep_is_held(&cgroup_mutex))
 
 /**
  * for_each_subsys - iterate all enabled cgroup subsystems
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 79e57b6df731..273a8a42cb72 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -1314,7 +1314,7 @@ void cgroup_free_root(struct cgroup_root *root)
 {
 	if (root) {
 		idr_destroy(&root->cgroup_idr);
-		kfree(root);
+		kfree_rcu(root, rcu);
 	}
 }
 
@@ -1348,7 +1348,7 @@ static void cgroup_destroy_root(struct cgroup_root *root)
 	spin_unlock_irq(&css_set_lock);
 
 	if (!list_empty(&root->root_list)) {
-		list_del(&root->root_list);
+		list_del_rcu(&root->root_list);
 		cgroup_root_count--;
 	}
 
@@ -1401,7 +1401,6 @@ static struct cgroup *cset_cgroup_from_root(struct css_set *cset,
 {
 	struct cgroup *res = NULL;
 
-	lockdep_assert_held(&cgroup_mutex);
 	lockdep_assert_held(&css_set_lock);
 
 	if (cset == &init_css_set) {
@@ -1421,13 +1420,23 @@ static struct cgroup *cset_cgroup_from_root(struct css_set *cset,
 		}
 	}
 
-	BUG_ON(!res);
+	/*
+	 * If cgroup_mutex is not held, the cgrp_cset_link will be freed
+	 * before we remove the cgroup root from the root_list. Consequently,
+	 * when accessing a cgroup root, the cset_link may have already been
+	 * freed, resulting in a NULL res_cgroup. However, by holding the
+	 * cgroup_mutex, we ensure that res_cgroup can't be NULL.
+	 * If we don't hold cgroup_mutex in the caller, we must do the NULL
+	 * check.
+	 */
 	return res;
 }
 
 /*
  * Return the cgroup for "task" from the given hierarchy. Must be
- * called with cgroup_mutex and css_set_lock held.
+ * called with css_set_lock held to prevent task's groups from being modified.
+ * Must be called with either cgroup_mutex or rcu read lock to prevent the
+ * cgroup root from being destroyed.
  */
 struct cgroup *task_cgroup_from_root(struct task_struct *task,
 				     struct cgroup_root *root)
@@ -2012,7 +2021,7 @@ void init_cgroup_root(struct cgroup_fs_context *ctx)
 	struct cgroup_root *root = ctx->root;
 	struct cgroup *cgrp = &root->cgrp;
 
-	INIT_LIST_HEAD(&root->root_list);
+	INIT_LIST_HEAD_RCU(&root->root_list);
 	atomic_set(&root->nr_cgrps, 1);
 	cgrp->root = root;
 	init_cgroup_housekeeping(cgrp);
@@ -2094,7 +2103,7 @@ int cgroup_setup_root(struct cgroup_root *root, u16 ss_mask)
 	 * care of subsystems' refcounts, which are explicitly dropped in
 	 * the failure exit path.
 	 */
-	list_add(&root->root_list, &cgroup_roots);
+	list_add_rcu(&root->root_list, &cgroup_roots);
 	cgroup_root_count++;
 
 	/*
-- 
2.45.2

[PATCH 2/2] cgroup: Move rcu_head up near the top of cgroup_root

[Siddh Raman Pant]

From: Waiman Long <longman@redhat.com>

commit a7fb0423c201ba12815877a0b5a68a6a1710b23a upstream.

Commit d23b5c577715 ("cgroup: Make operations on the cgroup root_list RCU
safe") adds a new rcu_head to the cgroup_root structure and kvfree_rcu()
for freeing the cgroup_root.

The current implementation of kvfree_rcu(), however, has the limitation
that the offset of the rcu_head structure within the larger data
structure must be less than 4096 or the compilation will fail. See the
macro definition of __is_kvfree_rcu_offset() in include/linux/rcupdate.h
for more information.

By putting rcu_head below the large cgroup structure, any change to the
cgroup structure that makes it larger run the risk of causing build
failure under certain configurations. Commit 77070eeb8821 ("cgroup:
Avoid false cacheline sharing of read mostly rstat_cpu") happens to be
the last straw that breaks it. Fix this problem by moving the rcu_head
structure up before the cgroup structure.

Fixes: d23b5c577715 ("cgroup: Make operations on the cgroup root_list RCU safe")
Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Closes: https://lore.kernel.org/lkml/20231207143806.114e0a74@canb.auug.org.au/
Signed-off-by: Waiman Long <longman@redhat.com>
Acked-by: Yafang Shao <laoar.shao@gmail.com>
Reviewed-by: Yosry Ahmed <yosryahmed@google.com>
Reviewed-by: Michal Koutný <mkoutny@suse.com>
Signed-off-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
[Shivani: Modified to apply on v5.4.y]
Signed-off-by: Shivani Agarwal <shivani.agarwal@broadcom.com>
Reviewed-by: Siddh Raman Pant <siddh.raman.pant@oracle.com>
---
 include/linux/cgroup-defs.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index c64f11674850..f0798d98be8e 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -506,6 +506,10 @@ struct cgroup_root {
 	/* Unique id for this hierarchy. */
 	int hierarchy_id;
 
+	/* A list running through the active hierarchies */
+	struct list_head root_list;
+	struct rcu_head rcu;
+
 	/* The root cgroup.  Root is destroyed on its release. */
 	struct cgroup cgrp;
 
@@ -515,10 +519,6 @@ struct cgroup_root {
 	/* Number of cgroups in the hierarchy, used only for /proc/cgroups */
 	atomic_t nr_cgrps;
 
-	/* A list running through the active hierarchies */
-	struct list_head root_list;
-	struct rcu_head rcu;
-
 	/* Hierarchy-specific flags */
 	unsigned int flags;
 
-- 
2.45.2

Re: [PATCH 1/2] cgroup: Make operations on the cgroup root_list RCU safe

[Greg Kroah-Hartman]

On Mon, Dec 02, 2024 at 03:41:01PM +0530, Siddh Raman Pant wrote:
> From: Yafang Shao <laoar.shao@gmail.com>
> 
> commit d23b5c577715892c87533b13923306acc6243f93 upstream.
> 
> At present, when we perform operations on the cgroup root_list, we must
> hold the cgroup_mutex, which is a relatively heavyweight lock. In reality,
> we can make operations on this list RCU-safe, eliminating the need to hold
> the cgroup_mutex during traversal. Modifications to the list only occur in
> the cgroup root setup and destroy paths, which should be infrequent in a
> production environment. In contrast, traversal may occur frequently.
> Therefore, making it RCU-safe would be beneficial.
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> [fp: adapt to 5.10 mainly because of changes made by e210a89f5b07
>  ("cgroup.c: add helper __cset_cgroup_from_root to cleanup duplicated
>  codes")]
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> [Shivani: Modified to apply on v5.4.y]
> Signed-off-by: Shivani Agarwal <shivani.agarwal@broadcom.com>
> Reviewed-by: Siddh Raman Pant <siddh.raman.pant@oracle.com>

I'm confused.  You do know what signed-off-by means, right?  When
sending a patch on, you MUST sign off on it.

Please work with other developers of the Oracle Linux kernel team to get
this right and send it as a whole new series, properly versioned, and
tested in in a format that we can take this in.

thanks,

greg k-h

Re: [PATCH 1/2] cgroup: Make operations on the cgroup root_list RCU safe

[Siddh Raman Pant]

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

On Mon, Dec 02 2024 at 15:47:00 +0530, Greg Kroah-Hartman wrote:
> On Mon, Dec 02, 2024 at 03:41:01PM +0530, Siddh Raman Pant wrote:
> > From: Yafang Shao <laoar.shao@gmail.com>
> > 
> > commit d23b5c577715892c87533b13923306acc6243f93 upstream.
> > 
> > At present, when we perform operations on the cgroup root_list, we must
> > hold the cgroup_mutex, which is a relatively heavyweight lock. In reality,
> > we can make operations on this list RCU-safe, eliminating the need to hold
> > the cgroup_mutex during traversal. Modifications to the list only occur in
> > the cgroup root setup and destroy paths, which should be infrequent in a
> > production environment. In contrast, traversal may occur frequently.
> > Therefore, making it RCU-safe would be beneficial.
> > 
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> > [fp: adapt to 5.10 mainly because of changes made by e210a89f5b07
> >  ("cgroup.c: add helper __cset_cgroup_from_root to cleanup duplicated
> >  codes")]
> > Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> > [Shivani: Modified to apply on v5.4.y]
> > Signed-off-by: Shivani Agarwal <shivani.agarwal@broadcom.com>
> > Reviewed-by: Siddh Raman Pant <siddh.raman.pant@oracle.com>
> 
> I'm confused.  You do know what signed-off-by means, right?  When
> sending a patch on, you MUST sign off on it.

Even if I'm just *forwarding* the patch already posted on the mailing
list? I just added an r-b for the patch because I reviewed it, and did
no changes.

I'm sorry if I mistook the convention. In that case, the previous
patche emails I had sent has the signoff. I had thought that was
incorrect.

Thanks,
Siddh



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] cgroup: Make operations on the cgroup root_list RCU safe

[gregkh]

On Mon, Dec 02, 2024 at 10:26:54AM +0000, Siddh Raman Pant wrote:
> On Mon, Dec 02 2024 at 15:47:00 +0530, Greg Kroah-Hartman wrote:
> > On Mon, Dec 02, 2024 at 03:41:01PM +0530, Siddh Raman Pant wrote:
> > > From: Yafang Shao <laoar.shao@gmail.com>
> > > 
> > > commit d23b5c577715892c87533b13923306acc6243f93 upstream.
> > > 
> > > At present, when we perform operations on the cgroup root_list, we must
> > > hold the cgroup_mutex, which is a relatively heavyweight lock. In reality,
> > > we can make operations on this list RCU-safe, eliminating the need to hold
> > > the cgroup_mutex during traversal. Modifications to the list only occur in
> > > the cgroup root setup and destroy paths, which should be infrequent in a
> > > production environment. In contrast, traversal may occur frequently.
> > > Therefore, making it RCU-safe would be beneficial.
> > > 
> > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > > Signed-off-by: Tejun Heo <tj@kernel.org>
> > > [fp: adapt to 5.10 mainly because of changes made by e210a89f5b07
> > >  ("cgroup.c: add helper __cset_cgroup_from_root to cleanup duplicated
> > >  codes")]
> > > Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> > > [Shivani: Modified to apply on v5.4.y]
> > > Signed-off-by: Shivani Agarwal <shivani.agarwal@broadcom.com>
> > > Reviewed-by: Siddh Raman Pant <siddh.raman.pant@oracle.com>
> > 
> > I'm confused.  You do know what signed-off-by means, right?  When
> > sending a patch on, you MUST sign off on it.
> 
> Even if I'm just *forwarding* the patch already posted on the mailing
> list?

Yes.