[RFC PATCH bpf-next 0/8] bpf, cgroup: Add BPF support for cgroup1 hierarchy

[RFC PATCH bpf-next 0/8] bpf, cgroup: Add BPF support for cgroup1 hierarchy

[Yafang Shao]

Currently, BPF is primarily confined to cgroup2, with the exception of
cgroup_iter, which supports cgroup1 fds. Unfortunately, this limitation
prevents us from harnessing the full potential of BPF within cgroup1
environments.

In our endeavor to seamlessly integrate BPF within our Kubernetes
environment, which relies on cgroup1, we have been exploring the
possibility of transitioning to cgroup2. While this transition is
forward-looking, it poses challenges due to the necessity for numerous
applications to adapt.

While we acknowledge that cgroup2 represents the future, we also recognize
that such transitions demand time and effort. As a result, we are
considering an alternative approach. Instead of migrating to cgroup2, we
are contemplating modifications to the BPF kernel code to ensure
compatibility with cgroup1. These adjustments appear to be relatively
minor, making this option more feasible.

Given the widespread use of cgroup1 in container environments, this change
would be beneficial to many users.

As discussed with Tejun[1], it has been determined that tying the interface
directly to the cgroup1 hierarchies is acceptable. As a result, this
patchset introduces cgroup1-only interfaces that operate with both
hierarchy ID and cgroup ID as parameters.

Within this patchset, two new cgroup1-only interfaces have been introduced:

- [bpf_]task_cgroup1_id_within_hierarchy
  Retrieves the associated cgroup ID of a task whithin a specific
  cgroup1 hierarchy. The cgroup1 hierarchy is identified by its
  hierarchy ID.
- [bpf_]task_ancestor_cgroup1_id_within_hierarchy
  Retrieves the associated ancestor cgroup ID of a task whithin a
  specific cgroup1 hierarchy. he specific ancestor cgroup is determined by
  the ancestor level within the cgroup1 hierarchy.
 
These two new kfuncs enable the tracing of tasks within a designated
container or its ancestor cgroup directory in BPF programs. Additionally,
they are capable of operating on named cgroups, providing valuable utility
for hybrid cgroup mode scenarios.

[1]. https://lwn.net/ml/cgroups/ZRHU6MfwqRxjBFUH@slm.duckdns.org/

Changes:
- bpf, cgroup: Add bpf support for cgroup controller
  https://lwn.net/Articles/945318/
- bpf, cgroup: Enable cgroup_array map on cgroup1
  https://lore.kernel.org/bpf/20230903142800.3870-1-laoar.shao@gmail.com/

Yafang Shao (8):
  cgroup: Don't have to hold cgroup_mutex in task_cgroup_from_root()
  cgroup: Add new helpers for cgroup1 hierarchy
  bpf: Add kfuncs for cgroup1 hierarchy
  selftests/bpf: Fix issues in setup_classid_environment()
  selftests/bpf: Add parallel support for classid
  selftests/bpf: Add a new cgroup helper get_classid_cgroup_id()
  selftests/bpf: Add a new cgroup helper get_cgroup_hierarchy_id()
  selftests/bpf: Add selftests for cgroup1 hierarchy

 include/linux/cgroup.h                        |   9 +-
 kernel/bpf/helpers.c                          |  26 +++
 kernel/cgroup/cgroup-internal.h               |   2 -
 kernel/cgroup/cgroup-v1.c                     |  67 ++++++++
 kernel/cgroup/cgroup.c                        |   5 +-
 tools/testing/selftests/bpf/cgroup_helpers.c  | 114 +++++++++++--
 tools/testing/selftests/bpf/cgroup_helpers.h  |   4 +-
 .../bpf/prog_tests/cgroup1_hierarchy.c        | 159 ++++++++++++++++++
 .../selftests/bpf/prog_tests/cgroup_v1v2.c    |   2 +-
 .../bpf/progs/test_cgroup1_hierarchy.c        |  62 +++++++
 10 files changed, 426 insertions(+), 24 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup1_hierarchy.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_cgroup1_hierarchy.c

-- 
2.30.1 (Apple Git-130)

[RFC PATCH bpf-next 1/8] cgroup: Don't have to hold cgroup_mutex in task_cgroup_from_root()

[Yafang Shao]

The task cannot modify cgroups if we have already acquired the
css_set_lock, thus eliminating the need to hold the cgroup_mutex. Following
this change, task_cgroup_from_root() can be employed in non-sleepable contexts.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 kernel/cgroup/cgroup.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 1fb7f562289d..bd1692f48be5 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -1453,7 +1453,7 @@ static struct cgroup *cset_cgroup_from_root(struct css_set *cset,
 
 /*
  * 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.
  */
 struct cgroup *task_cgroup_from_root(struct task_struct *task,
 				     struct cgroup_root *root)
@@ -1462,7 +1462,8 @@ struct cgroup *task_cgroup_from_root(struct task_struct *task,
 	 * No need to lock the task - since we hold css_set_lock the
 	 * task can't change groups.
 	 */
-	return cset_cgroup_from_root(task_css_set(task), root);
+	lockdep_assert_held(&css_set_lock);
+	return __cset_cgroup_from_root(task_css_set(task), root);
 }
 
 /*
-- 
2.30.1 (Apple Git-130)

[RFC PATCH bpf-next 2/8] cgroup: Add new helpers for cgroup1 hierarchy

[Yafang Shao]

Two new helpers are added for cgroup1 hierarchy:

- task_cgroup1_id_within_hierarchy
  Retrieves the associated cgroup ID of a task within a specific cgroup1
  hierarchy. The cgroup1 hierarchy is identified by its hierarchy ID.
- task_ancestor_cgroup1_id_within_hierarchy
  Retrieves the associated ancestor cgroup ID of a task whithin a
  specific cgroup1 hierarchy. The specific ancestor level is determined by
  its ancestor level.

These helper functions have been added to facilitate the tracing of tasks
within a particular container or cgroup in BPF programs. It's important to
note that these helpers are designed specifically for cgroup1.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 include/linux/cgroup.h          |  9 ++++-
 kernel/cgroup/cgroup-internal.h |  2 -
 kernel/cgroup/cgroup-v1.c       | 67 +++++++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+), 3 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index b307013b9c6c..65bde6eb41ef 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -71,6 +71,8 @@ struct css_task_iter {
 extern struct file_system_type cgroup_fs_type;
 extern struct cgroup_root cgrp_dfl_root;
 extern struct css_set init_css_set;
+extern struct list_head cgroup_roots;
+extern spinlock_t css_set_lock;
 
 #define SUBSYS(_x) extern struct cgroup_subsys _x ## _cgrp_subsys;
 #include <linux/cgroup_subsys.h>
@@ -159,6 +161,8 @@ void css_task_iter_start(struct cgroup_subsys_state *css, unsigned int flags,
 			 struct css_task_iter *it);
 struct task_struct *css_task_iter_next(struct css_task_iter *it);
 void css_task_iter_end(struct css_task_iter *it);
+struct cgroup *task_cgroup_from_root(struct task_struct *task,
+				     struct cgroup_root *root);
 
 /**
  * css_for_each_child - iterate through children of a css
@@ -388,7 +392,6 @@ static inline void cgroup_unlock(void)
  * as locks used during the cgroup_subsys::attach() methods.
  */
 #ifdef CONFIG_PROVE_RCU
-extern spinlock_t css_set_lock;
 #define task_css_set_check(task, __c)					\
 	rcu_dereference_check((task)->cgroups,				\
 		rcu_read_lock_sched_held() ||				\
@@ -855,4 +858,8 @@ static inline void cgroup_bpf_put(struct cgroup *cgrp) {}
 
 #endif /* CONFIG_CGROUP_BPF */
 
+u64 task_cgroup1_id_within_hierarchy(struct task_struct *tsk, int hierarchy_id);
+u64 task_ancestor_cgroup1_id_within_hierarchy(struct task_struct *tsk, int hierarchy_id,
+					      int ancestor_level);
+
 #endif /* _LINUX_CGROUP_H */
diff --git a/kernel/cgroup/cgroup-internal.h b/kernel/cgroup/cgroup-internal.h
index c56071f150f2..2c32a80c1334 100644
--- a/kernel/cgroup/cgroup-internal.h
+++ b/kernel/cgroup/cgroup-internal.h
@@ -164,9 +164,7 @@ struct cgroup_mgctx {
 #define DEFINE_CGROUP_MGCTX(name)						\
 	struct cgroup_mgctx name = CGROUP_MGCTX_INIT(name)
 
-extern spinlock_t css_set_lock;
 extern struct cgroup_subsys *cgroup_subsys[];
-extern struct list_head cgroup_roots;
 
 /* iterate across the hierarchies */
 #define for_each_root(root)						\
diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c
index c487ffef6652..18064de0a883 100644
--- a/kernel/cgroup/cgroup-v1.c
+++ b/kernel/cgroup/cgroup-v1.c
@@ -1263,6 +1263,73 @@ int cgroup1_get_tree(struct fs_context *fc)
 	return ret;
 }
 
+/**
+ * task_cgroup_id_within_hierarchy - Retrieves the associated cgroup ID from
+ * a task within a specific cgroup1 hierarchy.
+ * @task: The task to be tested
+ * @hierarchy_id: The hierarchy ID of a cgroup1
+ *
+ * We limit it to cgroup1 only.
+ */
+u64 task_cgroup1_id_within_hierarchy(struct task_struct *tsk, int hierarchy_id)
+{
+	struct cgroup_root *root;
+	struct cgroup *cgrp;
+	u64 cgid = 0;
+
+	spin_lock_irq(&css_set_lock);
+	list_for_each_entry(root, &cgroup_roots, root_list) {
+		/* cgroup1 only*/
+		if (root == &cgrp_dfl_root)
+			continue;
+		if (root->hierarchy_id != hierarchy_id)
+			continue;
+		cgrp = task_cgroup_from_root(tsk, root);
+		WARN_ON_ONCE(!cgrp);
+		cgid = cgroup_id(cgrp);
+		break;
+	}
+	spin_unlock_irq(&css_set_lock);
+	return cgid;
+}
+
+/**
+ * task_ancestor_cgroup_id_within_hierarchy - Retrieves the associated ancestor
+ * cgroup ID from a task within a specific cgroup1 hierarchy.
+ * @task: The task to be tested
+ * @hierarchy_id: The hierarchy ID of a cgroup1
+ * @ancestor_level: level of ancestor to find starting from root
+ *
+ * We limit it to cgroup1 only.
+ */
+u64 task_ancestor_cgroup1_id_within_hierarchy(struct task_struct *tsk, int hierarchy_id,
+					      int ancestor_level)
+{
+	struct cgroup *cgrp, *ancestor;
+	struct cgroup_root *root;
+	u64 cgid = 0;
+
+	spin_lock_irq(&css_set_lock);
+	list_for_each_entry(root, &cgroup_roots, root_list) {
+		/* cgroup1 only*/
+		if (root == &cgrp_dfl_root)
+			continue;
+		if (root->hierarchy_id != hierarchy_id)
+			continue;
+
+		cgrp = task_cgroup_from_root(tsk, root);
+		WARN_ON_ONCE(!cgrp);
+		ancestor = cgroup_ancestor(cgrp, ancestor_level);
+		if (!ancestor)
+			break;
+
+		cgid = cgroup_id(ancestor);
+		break;
+	}
+	spin_unlock_irq(&css_set_lock);
+	return cgid;
+}
+
 static int __init cgroup1_wq_init(void)
 {
 	/*
-- 
2.30.1 (Apple Git-130)

[RFC PATCH bpf-next 3/8] bpf: Add kfuncs for cgroup1 hierarchy

[Yafang Shao]

Two new kfuncs are added to retrieve the cgroup1 IDs.

- bpf_task_cgroup1_id_within_hierarchy
  Retrieves the associated cgroup ID of a task whithin a specific
  cgroup1 hierarchy. The cgroup1 hierarchy is identified by its
  hierarchy ID.
- bpf_task_ancestor_cgroup1_id_within_hierarchy
  Retrieves the associated ancestor cgroup ID of a task whithin a
  specific cgroup1 hierarchy. he specific ancestor cgroup is determined by
  the ancestor level within the cgroup1 hierarchy.

These two new kfuncs enable the tracing of tasks within a designated
container or cgroup directory in BPF programs.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 kernel/bpf/helpers.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index dd1c69ee3375..39ec6f9f2027 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2214,6 +2214,30 @@ __bpf_kfunc long bpf_task_under_cgroup(struct task_struct *task,
 {
 	return task_under_cgroup_hierarchy(task, ancestor);
 }
+
+/**
+ * bpf_task_cgroup_id_within_hierarchy - Retrieves the associated cgroup ID of a
+ * task within a specific cgroup1 hierarchy.
+ * @task: The target task
+ * @hierarchy_id: The ID of a cgroup1 hierarchy
+ */
+__bpf_kfunc u64 bpf_task_cgroup1_id_within_hierarchy(struct task_struct *task, int hierarchy_id)
+{
+	return task_cgroup1_id_within_hierarchy(task, hierarchy_id);
+}
+
+/**
+ * bpf_task_ancestor_cgroup_id_within_hierarchy - Retrieves the associated
+ * ancestor cgroup ID of a task within a specific cgroup1 hierarchy.
+ * @task: The target task
+ * @hierarchy_id: The ID of a cgroup1 hierarchy
+ * @ancestor_level: The cgroup level of the ancestor in the cgroup1 hierarchy
+ */
+__bpf_kfunc u64 bpf_task_ancestor_cgroup1_id_within_hierarchy(struct task_struct *task,
+							      int hierarchy_id, int ancestor_level)
+{
+	return task_ancestor_cgroup1_id_within_hierarchy(task, hierarchy_id, ancestor_level);
+}
 #endif /* CONFIG_CGROUPS */
 
 /**
@@ -2520,6 +2544,8 @@ BTF_ID_FLAGS(func, bpf_cgroup_release, KF_RELEASE)
 BTF_ID_FLAGS(func, bpf_cgroup_ancestor, KF_ACQUIRE | KF_RCU | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_cgroup_from_id, KF_ACQUIRE | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_task_under_cgroup, KF_RCU)
+BTF_ID_FLAGS(func, bpf_task_cgroup1_id_within_hierarchy, KF_RCU)
+BTF_ID_FLAGS(func, bpf_task_ancestor_cgroup1_id_within_hierarchy, KF_RCU)
 #endif
 BTF_ID_FLAGS(func, bpf_task_from_pid, KF_ACQUIRE | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_throw)
-- 
2.30.1 (Apple Git-130)

[RFC PATCH bpf-next 4/8] selftests/bpf: Fix issues in setup_classid_environment()

[Yafang Shao]

If the net_cls subsystem is already mounted, attempting to mount it again
in setup_classid_environment() will result in a failure with the error code
EBUSY. Despite this, tmpfs will have been successfully mounted at
/sys/fs/cgroup/net_cls. Consequently, the /sys/fs/cgroup/net_cls directory
will be empty, causing subsequent setup operations to fail.

Here's an error log excerpt illustrating the issue when net_cls has already
been mounted at /sys/fs/cgroup/net_cls prior to running
setup_classid_environment():

- Before that change

  $ tools/testing/selftests/bpf/test_progs --name=cgroup_v1v2
  test_cgroup_v1v2:PASS:server_fd 0 nsec
  test_cgroup_v1v2:PASS:client_fd 0 nsec
  test_cgroup_v1v2:PASS:cgroup_fd 0 nsec
  test_cgroup_v1v2:PASS:server_fd 0 nsec
  run_test:PASS:skel_open 0 nsec
  run_test:PASS:prog_attach 0 nsec
  test_cgroup_v1v2:PASS:cgroup-v2-only 0 nsec
  (cgroup_helpers.c:248: errno: No such file or directory) Opening Cgroup Procs: /sys/fs/cgroup/net_cls/cgroup.procs
  (cgroup_helpers.c:540: errno: No such file or directory) Opening cgroup classid: /sys/fs/cgroup/net_cls/cgroup-test-work-dir/net_cls.classid
  run_test:PASS:skel_open 0 nsec
  run_test:PASS:prog_attach 0 nsec
  (cgroup_helpers.c:248: errno: No such file or directory) Opening Cgroup Procs: /sys/fs/cgroup/net_cls/cgroup-test-work-dir/cgroup.procs
  run_test:FAIL:join_classid unexpected error: 1 (errno 2)
  test_cgroup_v1v2:FAIL:cgroup-v1v2 unexpected error: -1 (errno 2)
  (cgroup_helpers.c:248: errno: No such file or directory) Opening Cgroup Procs: /sys/fs/cgroup/net_cls/cgroup.procs
  #44      cgroup_v1v2:FAIL
  Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED

- After that change
  $ tools/testing/selftests/bpf/test_progs --name=cgroup_v1v2
  #44      cgroup_v1v2:OK
  Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 tools/testing/selftests/bpf/cgroup_helpers.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/cgroup_helpers.c b/tools/testing/selftests/bpf/cgroup_helpers.c
index 24ba56d42f2d..9c36d1db9f94 100644
--- a/tools/testing/selftests/bpf/cgroup_helpers.c
+++ b/tools/testing/selftests/bpf/cgroup_helpers.c
@@ -518,10 +518,20 @@ int setup_classid_environment(void)
 		return 1;
 	}
 
-	if (mount("net_cls", NETCLS_MOUNT_PATH, "cgroup", 0, "net_cls") &&
-	    errno != EBUSY) {
-		log_err("mount cgroup net_cls");
-		return 1;
+	if (mount("net_cls", NETCLS_MOUNT_PATH, "cgroup", 0, "net_cls")) {
+		if (errno != EBUSY) {
+			log_err("mount cgroup net_cls");
+			return 1;
+		}
+
+		if (rmdir(NETCLS_MOUNT_PATH)) {
+			log_err("rmdir cgroup net_cls");
+			return 1;
+		}
+		if (umount(CGROUP_MOUNT_DFLT)) {
+			log_err("umount cgroup base");
+			return 1;
+		}
 	}
 
 	cleanup_classid_environment();
-- 
2.30.1 (Apple Git-130)

[RFC PATCH bpf-next 5/8] selftests/bpf: Add parallel support for classid

[Yafang Shao]

Include the current pid in the classid cgroup path. This way, different
testers relying on classid-based configurations will have distinct classid
cgroup directories, enabling them to run concurrently. Additionally, we
leverage the current pid as the classid, ensuring unique identification.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 tools/testing/selftests/bpf/cgroup_helpers.c  | 19 ++++++++++++-------
 tools/testing/selftests/bpf/cgroup_helpers.h  |  2 +-
 .../selftests/bpf/prog_tests/cgroup_v1v2.c    |  2 +-
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/bpf/cgroup_helpers.c b/tools/testing/selftests/bpf/cgroup_helpers.c
index 9c36d1db9f94..e378fa057757 100644
--- a/tools/testing/selftests/bpf/cgroup_helpers.c
+++ b/tools/testing/selftests/bpf/cgroup_helpers.c
@@ -45,9 +45,12 @@
 #define format_parent_cgroup_path(buf, path) \
 	format_cgroup_path_pid(buf, path, getppid())
 
-#define format_classid_path(buf)				\
-	snprintf(buf, sizeof(buf), "%s%s", NETCLS_MOUNT_PATH,	\
-		 CGROUP_WORK_DIR)
+#define format_classid_path_pid(buf, pid)				\
+	snprintf(buf, sizeof(buf), "%s%s%d", NETCLS_MOUNT_PATH,	\
+		 CGROUP_WORK_DIR, pid)
+
+#define format_classid_path(buf)	\
+	format_classid_path_pid(buf, getpid())
 
 static __thread bool cgroup_workdir_mounted;
 
@@ -546,15 +549,17 @@ int setup_classid_environment(void)
 
 /**
  * set_classid() - Set a cgroupv1 net_cls classid
- * @id: the numeric classid
  *
- * Writes the passed classid into the cgroup work dir's net_cls.classid
+ * Writes the classid into the cgroup work dir's net_cls.classid
  * file in order to later on trigger socket tagging.
  *
+ * To make sure different classid testers have different classids, we use
+ * the current pid as the classid by default.
+ *
  * On success, it returns 0, otherwise on failure it returns 1. If there
  * is a failure, it prints the error to stderr.
  */
-int set_classid(unsigned int id)
+int set_classid(void)
 {
 	char cgroup_workdir[PATH_MAX - 42];
 	char cgroup_classid_path[PATH_MAX + 1];
@@ -570,7 +575,7 @@ int set_classid(unsigned int id)
 		return 1;
 	}
 
-	if (dprintf(fd, "%u\n", id) < 0) {
+	if (dprintf(fd, "%u\n", getpid()) < 0) {
 		log_err("Setting cgroup classid");
 		rc = 1;
 	}
diff --git a/tools/testing/selftests/bpf/cgroup_helpers.h b/tools/testing/selftests/bpf/cgroup_helpers.h
index 5c2cb9c8b546..92fc41daf4a4 100644
--- a/tools/testing/selftests/bpf/cgroup_helpers.h
+++ b/tools/testing/selftests/bpf/cgroup_helpers.h
@@ -29,7 +29,7 @@ int setup_cgroup_environment(void);
 void cleanup_cgroup_environment(void);
 
 /* cgroupv1 related */
-int set_classid(unsigned int id);
+int set_classid(void);
 int join_classid(void);
 
 int setup_classid_environment(void);
diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_v1v2.c b/tools/testing/selftests/bpf/prog_tests/cgroup_v1v2.c
index 9026b42914d3..addf720428f7 100644
--- a/tools/testing/selftests/bpf/prog_tests/cgroup_v1v2.c
+++ b/tools/testing/selftests/bpf/prog_tests/cgroup_v1v2.c
@@ -71,7 +71,7 @@ void test_cgroup_v1v2(void)
 	}
 	ASSERT_OK(run_test(cgroup_fd, server_fd, false), "cgroup-v2-only");
 	setup_classid_environment();
-	set_classid(42);
+	set_classid();
 	ASSERT_OK(run_test(cgroup_fd, server_fd, true), "cgroup-v1v2");
 	cleanup_classid_environment();
 	close(server_fd);
-- 
2.30.1 (Apple Git-130)

[RFC PATCH bpf-next 6/8] selftests/bpf: Add a new cgroup helper get_classid_cgroup_id()

[Yafang Shao]

Introduce a new helper function to retrieve the cgroup ID from a net_cls
cgroup directory.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 tools/testing/selftests/bpf/cgroup_helpers.c | 28 +++++++++++++++-----
 tools/testing/selftests/bpf/cgroup_helpers.h |  1 +
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/cgroup_helpers.c b/tools/testing/selftests/bpf/cgroup_helpers.c
index e378fa057757..7cb2c9597b8f 100644
--- a/tools/testing/selftests/bpf/cgroup_helpers.c
+++ b/tools/testing/selftests/bpf/cgroup_helpers.c
@@ -417,26 +417,23 @@ int create_and_get_cgroup(const char *relative_path)
 }
 
 /**
- * get_cgroup_id() - Get cgroup id for a particular cgroup path
- * @relative_path: The cgroup path, relative to the workdir, to join
+ * get_cgroup_id_from_path - Get cgroup id for a particular cgroup path
+ * @cgroup_workdir: The absolute cgroup path
  *
  * On success, it returns the cgroup id. On failure it returns 0,
  * which is an invalid cgroup id.
  * If there is a failure, it prints the error to stderr.
  */
-unsigned long long get_cgroup_id(const char *relative_path)
+unsigned long long get_cgroup_id_from_path(const char *cgroup_workdir)
 {
 	int dirfd, err, flags, mount_id, fhsize;
 	union {
 		unsigned long long cgid;
 		unsigned char raw_bytes[8];
 	} id;
-	char cgroup_workdir[PATH_MAX + 1];
 	struct file_handle *fhp, *fhp2;
 	unsigned long long ret = 0;
 
-	format_cgroup_path(cgroup_workdir, relative_path);
-
 	dirfd = AT_FDCWD;
 	flags = 0;
 	fhsize = sizeof(*fhp);
@@ -472,6 +469,14 @@ unsigned long long get_cgroup_id(const char *relative_path)
 	return ret;
 }
 
+unsigned long long get_cgroup_id(const char *relative_path)
+{
+	char cgroup_workdir[PATH_MAX + 1];
+
+	format_cgroup_path(cgroup_workdir, relative_path);
+	return get_cgroup_id_from_path(cgroup_workdir);
+}
+
 int cgroup_setup_and_join(const char *path) {
 	int cg_fd;
 
@@ -617,3 +622,14 @@ void cleanup_classid_environment(void)
 	join_cgroup_from_top(NETCLS_MOUNT_PATH);
 	nftw(cgroup_workdir, nftwfunc, WALK_FD_LIMIT, FTW_DEPTH | FTW_MOUNT);
 }
+
+/**
+ * get_classid_cgroup_id - Get the cgroup id of a net_cls cgroup
+ */
+unsigned long long get_classid_cgroup_id(void)
+{
+	char cgroup_workdir[PATH_MAX + 1];
+
+	format_classid_path(cgroup_workdir);
+	return get_cgroup_id_from_path(cgroup_workdir);
+}
diff --git a/tools/testing/selftests/bpf/cgroup_helpers.h b/tools/testing/selftests/bpf/cgroup_helpers.h
index 92fc41daf4a4..e71da4ef031b 100644
--- a/tools/testing/selftests/bpf/cgroup_helpers.h
+++ b/tools/testing/selftests/bpf/cgroup_helpers.h
@@ -31,6 +31,7 @@ void cleanup_cgroup_environment(void);
 /* cgroupv1 related */
 int set_classid(void);
 int join_classid(void);
+unsigned long long get_classid_cgroup_id(void);
 
 int setup_classid_environment(void);
 void cleanup_classid_environment(void);
-- 
2.30.1 (Apple Git-130)

[RFC PATCH bpf-next 7/8] selftests/bpf: Add a new cgroup helper get_cgroup_hierarchy_id()

[Yafang Shao]

A new cgroup helper function, get_cgroup1_hierarchy_id(), has been
introduced to obtain the ID of a cgroup1 hierarchy based on the provided
cgroup name. This cgroup name can be obtained from the /proc/self/cgroup
file.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 tools/testing/selftests/bpf/cgroup_helpers.c | 49 ++++++++++++++++++++
 tools/testing/selftests/bpf/cgroup_helpers.h |  1 +
 2 files changed, 50 insertions(+)

diff --git a/tools/testing/selftests/bpf/cgroup_helpers.c b/tools/testing/selftests/bpf/cgroup_helpers.c
index 7cb2c9597b8f..5cb66fb3d4fe 100644
--- a/tools/testing/selftests/bpf/cgroup_helpers.c
+++ b/tools/testing/selftests/bpf/cgroup_helpers.c
@@ -633,3 +633,52 @@ unsigned long long get_classid_cgroup_id(void)
 	format_classid_path(cgroup_workdir);
 	return get_cgroup_id_from_path(cgroup_workdir);
 }
+
+/**
+ * get_cgroup1_hierarchy_id - Retrieves the ID of a cgroup1 hierarchy from the cgroup1 name
+ * @cgrp_name: The cgroup1 name, which can be retrieved from /proc/self/cgroup.
+ */
+int get_cgroup1_hierarchy_id(const char *cgrp_name)
+{
+	char *c, *c2, *c3, *c4;
+	bool found = false;
+	char line[1024];
+	FILE *file;
+	int i, id;
+
+	if (!cgrp_name)
+		return -1;
+
+	file = fopen("/proc/self/cgroup", "r");
+	if (!file) {
+		log_err("fopen /proc/self/cgroup");
+		return -1;
+	}
+
+	while (fgets(line, 1024, file)) {
+		i = 0;
+		for (c = strtok_r(line, ":", &c2); c && i < 2; c = strtok_r(NULL, ":", &c2)) {
+			if (i == 0) {
+				id = strtol(c, NULL, 10);
+			} else if (i == 1) {
+				if (!strcmp(c, cgrp_name)) {
+					found = true;
+					break;
+				}
+
+				/* Multiple subsystems may share one single mount point */
+				for (c3 = strtok_r(c, ",", &c4); c3;
+				     c3 = strtok_r(NULL, ",", &c4)) {
+					if (!strcmp(c, cgrp_name)) {
+						found = true;
+						break;
+					}
+				}
+			}
+			i++;
+		}
+		if (found)
+			break;
+	}
+	return found ? id : -1;
+}
diff --git a/tools/testing/selftests/bpf/cgroup_helpers.h b/tools/testing/selftests/bpf/cgroup_helpers.h
index e71da4ef031b..a80c41734a26 100644
--- a/tools/testing/selftests/bpf/cgroup_helpers.h
+++ b/tools/testing/selftests/bpf/cgroup_helpers.h
@@ -20,6 +20,7 @@ int get_root_cgroup(void);
 int create_and_get_cgroup(const char *relative_path);
 void remove_cgroup(const char *relative_path);
 unsigned long long get_cgroup_id(const char *relative_path);
+int get_cgroup1_hierarchy_id(const char *cgrp_name);
 
 int join_cgroup(const char *relative_path);
 int join_root_cgroup(void);
-- 
2.30.1 (Apple Git-130)

[RFC PATCH bpf-next 8/8] selftests/bpf: Add selftests for cgroup1 hierarchy

[Yafang Shao]

Add selftests for cgroup1 hierarchy.
The result as follows,

  $ tools/testing/selftests/bpf/test_progs --name=cgroup1_hierarchy
  #36/1    cgroup1_hierarchy/test_cgroup1_hierarchy:OK
  #36/2    cgroup1_hierarchy/test_root_cgid:OK
  #36/3    cgroup1_hierarchy/test_invalid_level:OK
  #36/4    cgroup1_hierarchy/test_invalid_cgid:OK
  #36/5    cgroup1_hierarchy/test_invalid_hid:OK
  #36/6    cgroup1_hierarchy/test_invalid_cgrp_name:OK
  #36/7    cgroup1_hierarchy/test_invalid_cgrp_name2:OK
  #36/8    cgroup1_hierarchy/test_sleepable_prog:OK
  #36      cgroup1_hierarchy:OK
  Summary: 1/8 PASSED, 0 SKIPPED, 0 FAILED

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 .../bpf/prog_tests/cgroup1_hierarchy.c        | 159 ++++++++++++++++++
 .../bpf/progs/test_cgroup1_hierarchy.c        |  62 +++++++
 2 files changed, 221 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup1_hierarchy.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_cgroup1_hierarchy.c

diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup1_hierarchy.c b/tools/testing/selftests/bpf/prog_tests/cgroup1_hierarchy.c
new file mode 100644
index 000000000000..4aafbc921254
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/cgroup1_hierarchy.c
@@ -0,0 +1,159 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2023 Yafang Shao <laoar.shao@gmail.com> */
+
+#include <sys/types.h>
+#include <unistd.h>
+#include <test_progs.h>
+#include "cgroup_helpers.h"
+#include "test_cgroup1_hierarchy.skel.h"
+
+static void bpf_cgroup1(struct test_cgroup1_hierarchy *skel)
+{
+	int err;
+
+	/* Attach LSM prog first */
+	skel->links.lsm_run = bpf_program__attach_lsm(skel->progs.lsm_run);
+	if (!ASSERT_OK_PTR(skel->links.lsm_run, "lsm_attach"))
+		return;
+
+	/* LSM prog will be triggered when attaching fentry */
+	skel->links.fentry_run = bpf_program__attach_trace(skel->progs.fentry_run);
+	ASSERT_NULL(skel->links.fentry_run, "fentry_attach_fail");
+
+	err = bpf_link__destroy(skel->links.lsm_run);
+	ASSERT_OK(err, "destroy_lsm");
+	skel->links.lsm_run = NULL;
+}
+
+static void bpf_cgroup1_sleepable(struct test_cgroup1_hierarchy *skel)
+{
+	int err;
+
+	/* Attach LSM prog first */
+	skel->links.lsm_s_run = bpf_program__attach_lsm(skel->progs.lsm_s_run);
+	if (!ASSERT_OK_PTR(skel->links.lsm_s_run, "lsm_attach"))
+		return;
+
+	/* LSM prog will be triggered when attaching fentry */
+	skel->links.fentry_run = bpf_program__attach_trace(skel->progs.fentry_run);
+	ASSERT_NULL(skel->links.fentry_run, "fentry_attach_fail");
+
+	err = bpf_link__destroy(skel->links.lsm_s_run);
+	ASSERT_OK(err, "destroy_lsm");
+	skel->links.lsm_s_run = NULL;
+}
+
+static void bpf_cgroup1_invalid_id(struct test_cgroup1_hierarchy *skel)
+{
+	int err;
+
+	/* Attach LSM prog first */
+	skel->links.lsm_run = bpf_program__attach_lsm(skel->progs.lsm_run);
+	if (!ASSERT_OK_PTR(skel->links.lsm_run, "lsm_attach"))
+		return;
+
+	/* LSM prog will be triggered when attaching fentry */
+	skel->links.fentry_run = bpf_program__attach_trace(skel->progs.fentry_run);
+	if (!ASSERT_OK_PTR(skel->links.fentry_run, "fentry_attach_success"))
+		goto cleanup;
+
+	err = bpf_link__destroy(skel->links.lsm_run);
+	ASSERT_OK(err, "destroy_lsm");
+	skel->links.lsm_run = NULL;
+
+cleanup:
+	err = bpf_link__destroy(skel->links.fentry_run);
+	ASSERT_OK(err, "destroy_fentry");
+	skel->links.fentry_run = NULL;
+}
+
+void test_cgroup1_hierarchy(void)
+{
+	struct test_cgroup1_hierarchy *skel;
+	__u64 current_cgid;
+	int hid, err;
+
+	skel = test_cgroup1_hierarchy__open();
+	if (!ASSERT_OK_PTR(skel, "open"))
+		return;
+
+	skel->bss->target_pid = getpid();
+
+	err = bpf_program__set_attach_target(skel->progs.fentry_run, 0, "bpf_fentry_test1");
+	if (!ASSERT_OK(err, "fentry_set_target"))
+		goto destroy;
+
+	err = test_cgroup1_hierarchy__load(skel);
+	if (!ASSERT_OK(err, "load"))
+		goto destroy;
+
+	/* Setup cgroup1 hierarchy */
+	err = setup_classid_environment();
+	if (!ASSERT_OK(err, "setup_classid_environment"))
+		goto destroy;
+
+	err = join_classid();
+	if (!ASSERT_OK(err, "join_cgroup1"))
+		goto cleanup;
+
+	current_cgid = get_classid_cgroup_id();
+	if (!ASSERT_GE(current_cgid, 0, "cgroup1 id"))
+		goto cleanup;
+
+	hid = get_cgroup1_hierarchy_id("net_cls");
+	if (!ASSERT_GE(hid, 0, "cgroup1 id"))
+		goto cleanup;
+	skel->bss->target_hid = hid;
+
+	if (test__start_subtest("test_cgroup1_hierarchy")) {
+		skel->bss->target_ancestor_cgid = current_cgid;
+		bpf_cgroup1(skel);
+	}
+
+	if (test__start_subtest("test_root_cgid")) {
+		skel->bss->target_ancestor_cgid = 1;
+		skel->bss->target_ancestor_level = 0;
+		bpf_cgroup1(skel);
+	}
+
+	if (test__start_subtest("test_invalid_level")) {
+		skel->bss->target_ancestor_cgid = 1;
+		skel->bss->target_ancestor_level = 1;
+		bpf_cgroup1_invalid_id(skel);
+	}
+
+	if (test__start_subtest("test_invalid_cgid")) {
+		skel->bss->target_ancestor_cgid = 0;
+		bpf_cgroup1_invalid_id(skel);
+	}
+
+	if (test__start_subtest("test_invalid_hid")) {
+		skel->bss->target_ancestor_cgid = 1;
+		skel->bss->target_ancestor_level = 0;
+		skel->bss->target_hid = -1;
+		bpf_cgroup1_invalid_id(skel);
+	}
+
+	if (test__start_subtest("test_invalid_cgrp_name")) {
+		skel->bss->target_hid = get_cgroup1_hierarchy_id("net_cl");
+		skel->bss->target_ancestor_cgid = current_cgid;
+		bpf_cgroup1_invalid_id(skel);
+	}
+
+	if (test__start_subtest("test_invalid_cgrp_name2")) {
+		skel->bss->target_hid = get_cgroup1_hierarchy_id("net_cls,");
+		skel->bss->target_ancestor_cgid = current_cgid;
+		bpf_cgroup1_invalid_id(skel);
+	}
+
+	if (test__start_subtest("test_sleepable_prog")) {
+		skel->bss->target_hid = hid;
+		skel->bss->target_ancestor_cgid = current_cgid;
+		bpf_cgroup1_sleepable(skel);
+	}
+
+cleanup:
+	cleanup_classid_environment();
+destroy:
+	test_cgroup1_hierarchy__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_cgroup1_hierarchy.c b/tools/testing/selftests/bpf/progs/test_cgroup1_hierarchy.c
new file mode 100644
index 000000000000..95d9031892e4
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_cgroup1_hierarchy.c
@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: GPL-2.0
+//#endif
+/* Copyright (C) 2023 Yafang Shao <laoar.shao@gmail.com> */
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <bpf/bpf_core_read.h>
+
+__u32 target_ancestor_level;
+__u64 target_ancestor_cgid;
+int target_pid, target_hid;
+
+u64 bpf_task_cgroup1_id_within_hierarchy(struct task_struct *task, int hierarchy_id) __ksym;
+u64 bpf_task_ancestor_cgroup1_id_within_hierarchy(struct task_struct *task, int hierarchy_id,
+						 int ancestor_level) __ksym;
+
+static int bpf_link_create_verify(int cmd)
+{
+	__u64 cgid, ancestor_cgid;
+	struct task_struct *task;
+	int ret = 0;
+
+	if (cmd != BPF_LINK_CREATE)
+		return 0;
+
+	task = bpf_get_current_task_btf();
+	/* Then it can run in parallel */
+	if (task->pid != target_pid)
+		return 0;
+
+	/* Refuse it if its cgid or its ancestor's cgid is the target cgid */
+	cgid = bpf_task_cgroup1_id_within_hierarchy(task, target_hid);
+	if (cgid == target_ancestor_cgid)
+		ret = -1;
+
+	ancestor_cgid = bpf_task_ancestor_cgroup1_id_within_hierarchy(task, target_hid,
+								      target_ancestor_level);
+	if (ancestor_cgid == target_ancestor_cgid)
+		ret = -1;
+	return ret;
+}
+
+SEC("lsm/bpf")
+int BPF_PROG(lsm_run, int cmd, union bpf_attr *attr, unsigned int size)
+{
+	return bpf_link_create_verify(cmd);
+}
+
+SEC("lsm.s/bpf")
+int BPF_PROG(lsm_s_run, int cmd, union bpf_attr *attr, unsigned int size)
+{
+	return bpf_link_create_verify(cmd);
+}
+
+SEC("fentry")
+int BPF_PROG(fentry_run)
+{
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.30.1 (Apple Git-130)

Re: [RFC PATCH bpf-next 1/8] cgroup: Don't have to hold cgroup_mutex in task_cgroup_from_root()

[Tejun Heo]

On Sat, Oct 07, 2023 at 02:02:57PM +0000, Yafang Shao wrote:
> The task cannot modify cgroups if we have already acquired the
> css_set_lock, thus eliminating the need to hold the cgroup_mutex. Following
> this change, task_cgroup_from_root() can be employed in non-sleepable contexts.
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>

Maybe just drop lockdep_assert_held(&cgroup_mutex) from
cset_cgroup_from_root()?

Thanks.

-- 
tejun

Re: [RFC PATCH bpf-next 2/8] cgroup: Add new helpers for cgroup1 hierarchy

[Tejun Heo]

Hello,

On Sat, Oct 07, 2023 at 02:02:58PM +0000, Yafang Shao wrote:
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index b307013b9c6c..65bde6eb41ef 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -71,6 +71,8 @@ struct css_task_iter {
>  extern struct file_system_type cgroup_fs_type;
>  extern struct cgroup_root cgrp_dfl_root;
>  extern struct css_set init_css_set;
> +extern struct list_head cgroup_roots;
> +extern spinlock_t css_set_lock;

css_set_lock was already out here but why do we need to move cgrou_roots to
this header?

> +/**
> + * task_cgroup_id_within_hierarchy - Retrieves the associated cgroup ID from
> + * a task within a specific cgroup1 hierarchy.
> + * @task: The task to be tested
> + * @hierarchy_id: The hierarchy ID of a cgroup1
> + *
> + * We limit it to cgroup1 only.
> + */
> +u64 task_cgroup1_id_within_hierarchy(struct task_struct *tsk, int hierarchy_id)
> +{
...
> +}
> +
> +/**
> + * task_ancestor_cgroup_id_within_hierarchy - Retrieves the associated ancestor
> + * cgroup ID from a task within a specific cgroup1 hierarchy.
> + * @task: The task to be tested
> + * @hierarchy_id: The hierarchy ID of a cgroup1
> + * @ancestor_level: level of ancestor to find starting from root
> + *
> + * We limit it to cgroup1 only.
> + */
> +u64 task_ancestor_cgroup1_id_within_hierarchy(struct task_struct *tsk, int hierarchy_id,
> +					      int ancestor_level)
> +{
...
> +}

I'd much prefer to have `struct group *task_get_cgroup1_within_hierarchy()`
then the caller can do cgroup_ancestor() itself.

Thanks.

-- 
tejun

Re: [RFC PATCH bpf-next 3/8] bpf: Add kfuncs for cgroup1 hierarchy

[Tejun Heo]

On Sat, Oct 07, 2023 at 02:02:59PM +0000, Yafang Shao wrote:
> +
> +/**
> + * bpf_task_cgroup_id_within_hierarchy - Retrieves the associated cgroup ID of a
> + * task within a specific cgroup1 hierarchy.
> + * @task: The target task
> + * @hierarchy_id: The ID of a cgroup1 hierarchy
> + */
> +__bpf_kfunc u64 bpf_task_cgroup1_id_within_hierarchy(struct task_struct *task, int hierarchy_id)
> +{
> +	return task_cgroup1_id_within_hierarchy(task, hierarchy_id);
> +}
> +
> +/**
> + * bpf_task_ancestor_cgroup_id_within_hierarchy - Retrieves the associated
> + * ancestor cgroup ID of a task within a specific cgroup1 hierarchy.
> + * @task: The target task
> + * @hierarchy_id: The ID of a cgroup1 hierarchy
> + * @ancestor_level: The cgroup level of the ancestor in the cgroup1 hierarchy
> + */
> +__bpf_kfunc u64 bpf_task_ancestor_cgroup1_id_within_hierarchy(struct task_struct *task,
> +							      int hierarchy_id, int ancestor_level)
> +{
> +	return task_ancestor_cgroup1_id_within_hierarchy(task, hierarchy_id, ancestor_level);
> +}

The same here. Please make one helper that returns a kptr and then let the
user call bpf_cgroup_ancestor() if desired.

Thanks.

-- 
tejun

Re: [RFC PATCH bpf-next 1/8] cgroup: Don't have to hold cgroup_mutex in task_cgroup_from_root()

[Yafang Shao]

On Sat, Oct 7, 2023 at 11:50 PM Tejun Heo <tj@kernel.org> wrote:
>
> On Sat, Oct 07, 2023 at 02:02:57PM +0000, Yafang Shao wrote:
> > The task cannot modify cgroups if we have already acquired the
> > css_set_lock, thus eliminating the need to hold the cgroup_mutex. Following
> > this change, task_cgroup_from_root() can be employed in non-sleepable contexts.
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>
> Maybe just drop lockdep_assert_held(&cgroup_mutex) from
> cset_cgroup_from_root()?

It seems we can do it.
will do it in the next version.

-- 
Regards
Yafang

Re: [RFC PATCH bpf-next 2/8] cgroup: Add new helpers for cgroup1 hierarchy

[Yafang Shao]

On Sat, Oct 7, 2023 at 11:55 PM Tejun Heo <tj@kernel.org> wrote:
>
> Hello,
>
> On Sat, Oct 07, 2023 at 02:02:58PM +0000, Yafang Shao wrote:
> > diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> > index b307013b9c6c..65bde6eb41ef 100644
> > --- a/include/linux/cgroup.h
> > +++ b/include/linux/cgroup.h
> > @@ -71,6 +71,8 @@ struct css_task_iter {
> >  extern struct file_system_type cgroup_fs_type;
> >  extern struct cgroup_root cgrp_dfl_root;
> >  extern struct css_set init_css_set;
> > +extern struct list_head cgroup_roots;
> > +extern spinlock_t css_set_lock;
>
> css_set_lock was already out here but why do we need to move cgrou_roots to
> this header?

Ah, shouldn't export cgrou_roots. Thanks for pointing it out.

>
> > +/**
> > + * task_cgroup_id_within_hierarchy - Retrieves the associated cgroup ID from
> > + * a task within a specific cgroup1 hierarchy.
> > + * @task: The task to be tested
> > + * @hierarchy_id: The hierarchy ID of a cgroup1
> > + *
> > + * We limit it to cgroup1 only.
> > + */
> > +u64 task_cgroup1_id_within_hierarchy(struct task_struct *tsk, int hierarchy_id)
> > +{
> ...
> > +}
> > +
> > +/**
> > + * task_ancestor_cgroup_id_within_hierarchy - Retrieves the associated ancestor
> > + * cgroup ID from a task within a specific cgroup1 hierarchy.
> > + * @task: The task to be tested
> > + * @hierarchy_id: The hierarchy ID of a cgroup1
> > + * @ancestor_level: level of ancestor to find starting from root
> > + *
> > + * We limit it to cgroup1 only.
> > + */
> > +u64 task_ancestor_cgroup1_id_within_hierarchy(struct task_struct *tsk, int hierarchy_id,
> > +                                           int ancestor_level)
> > +{
> ...
> > +}
>
> I'd much prefer to have `struct group *task_get_cgroup1_within_hierarchy()`
> then the caller can do cgroup_ancestor() itself.

Good suggestion. will do it in the next version.

-- 
Regards
Yafang

Re: [RFC PATCH bpf-next 3/8] bpf: Add kfuncs for cgroup1 hierarchy

[Yafang Shao]

On Sat, Oct 7, 2023 at 11:57 PM Tejun Heo <tj@kernel.org> wrote:
>
> On Sat, Oct 07, 2023 at 02:02:59PM +0000, Yafang Shao wrote:
> > +
> > +/**
> > + * bpf_task_cgroup_id_within_hierarchy - Retrieves the associated cgroup ID of a
> > + * task within a specific cgroup1 hierarchy.
> > + * @task: The target task
> > + * @hierarchy_id: The ID of a cgroup1 hierarchy
> > + */
> > +__bpf_kfunc u64 bpf_task_cgroup1_id_within_hierarchy(struct task_struct *task, int hierarchy_id)
> > +{
> > +     return task_cgroup1_id_within_hierarchy(task, hierarchy_id);
> > +}
> > +
> > +/**
> > + * bpf_task_ancestor_cgroup_id_within_hierarchy - Retrieves the associated
> > + * ancestor cgroup ID of a task within a specific cgroup1 hierarchy.
> > + * @task: The target task
> > + * @hierarchy_id: The ID of a cgroup1 hierarchy
> > + * @ancestor_level: The cgroup level of the ancestor in the cgroup1 hierarchy
> > + */
> > +__bpf_kfunc u64 bpf_task_ancestor_cgroup1_id_within_hierarchy(struct task_struct *task,
> > +                                                           int hierarchy_id, int ancestor_level)
> > +{
> > +     return task_ancestor_cgroup1_id_within_hierarchy(task, hierarchy_id, ancestor_level);
> > +}
>
> The same here. Please make one helper that returns a kptr and then let the
> user call bpf_cgroup_ancestor() if desired.

Sure, will do it.

-- 
Regards
Yafang

Re: [RFC PATCH bpf-next 2/8] cgroup: Add new helpers for cgroup1 hierarchy

[Michal Koutný]

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

Hello.

On Sat, Oct 07, 2023 at 02:02:58PM +0000, Yafang Shao <laoar.shao@gmail.com> wrote:
> Two new helpers are added for cgroup1 hierarchy:
> 
> - task_cgroup1_id_within_hierarchy
>   Retrieves the associated cgroup ID of a task within a specific cgroup1
>   hierarchy. The cgroup1 hierarchy is identified by its hierarchy ID.
> - task_ancestor_cgroup1_id_within_hierarchy
>   Retrieves the associated ancestor cgroup ID of a task whithin a
>   specific cgroup1 hierarchy. The specific ancestor level is determined by
>   its ancestor level.
> 
> These helper functions have been added to facilitate the tracing of tasks
> within a particular container or cgroup in BPF programs. It's important to
> note that these helpers are designed specifically for cgroup1.

Are this helpers need for any 3rd party task?

I *think* operating on `current` would be simpler wrt assumptions needed
for object presense.


> +u64 task_cgroup1_id_within_hierarchy(struct task_struct *tsk, int hierarchy_id)
> +{
> +	struct cgroup_root *root;
> +	struct cgroup *cgrp;
> +	u64 cgid = 0;
> +
> +	spin_lock_irq(&css_set_lock);
> +	list_for_each_entry(root, &cgroup_roots, root_list) {

This should be for_each_root() macro for better uniform style.


Michal


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

Re: [RFC PATCH bpf-next 0/8] bpf, cgroup: Add BPF support for cgroup1 hierarchy

[Michal Koutný]

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

Hi.

On Sat, Oct 07, 2023 at 02:02:56PM +0000, Yafang Shao <laoar.shao@gmail.com> wrote:
> Given the widespread use of cgroup1 in container environments, this change
> would be beneficial to many users.

This is an unverifiable claim (and benefit applies only to subset of
those users who would use cgroup1 and BPF). So please don't use it in
this form.

Thanks,
Michal


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

Re: [RFC PATCH bpf-next 2/8] cgroup: Add new helpers for cgroup1 hierarchy

[Yafang Shao]

On Mon, Oct 9, 2023 at 7:32 PM Michal Koutný <mkoutny@suse.com> wrote:
>
> Hello.
>
> On Sat, Oct 07, 2023 at 02:02:58PM +0000, Yafang Shao <laoar.shao@gmail.com> wrote:
> > Two new helpers are added for cgroup1 hierarchy:
> >
> > - task_cgroup1_id_within_hierarchy
> >   Retrieves the associated cgroup ID of a task within a specific cgroup1
> >   hierarchy. The cgroup1 hierarchy is identified by its hierarchy ID.
> > - task_ancestor_cgroup1_id_within_hierarchy
> >   Retrieves the associated ancestor cgroup ID of a task whithin a
> >   specific cgroup1 hierarchy. The specific ancestor level is determined by
> >   its ancestor level.
> >
> > These helper functions have been added to facilitate the tracing of tasks
> > within a particular container or cgroup in BPF programs. It's important to
> > note that these helpers are designed specifically for cgroup1.
>
> Are this helpers need for any 3rd party task?

Yes, for example, we can check if the *next* task in sched_switch
belongs to a specific cgroup.

Hao also pointed out the use case of a 3rd partry task[1].

[1]. https://lore.kernel.org/bpf/CA+khW7hah317_beZ7QDA1R=sWi5q7TanRC+efMhivPKtWzbA4w@mail.gmail.com/

>
> I *think* operating on `current` would be simpler wrt assumptions needed
> for object presense.
>
>
> > +u64 task_cgroup1_id_within_hierarchy(struct task_struct *tsk, int hierarchy_id)
> > +{
> > +     struct cgroup_root *root;
> > +     struct cgroup *cgrp;
> > +     u64 cgid = 0;
> > +
> > +     spin_lock_irq(&css_set_lock);
> > +     list_for_each_entry(root, &cgroup_roots, root_list) {
>
> This should be for_each_root() macro for better uniform style.

will use it in the next version.

-- 
Regards
Yafang

Re: [RFC PATCH bpf-next 0/8] bpf, cgroup: Add BPF support for cgroup1 hierarchy

[Yafang Shao]

On Mon, Oct 9, 2023 at 7:46 PM Michal Koutný <mkoutny@suse.com> wrote:
>
> Hi.
>
> On Sat, Oct 07, 2023 at 02:02:56PM +0000, Yafang Shao <laoar.shao@gmail.com> wrote:
> > Given the widespread use of cgroup1 in container environments, this change
> > would be beneficial to many users.
>
> This is an unverifiable claim (and benefit applies only to subset of
> those users who would use cgroup1 and BPF). So please don't use it in
> this form.

Sure. will remove it.

-- 
Regards
Yafang

Re: [RFC PATCH bpf-next 1/8] cgroup: Don't have to hold cgroup_mutex in task_cgroup_from_root()

[Michal Koutný]

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

On Sat, Oct 07, 2023 at 02:02:57PM +0000, Yafang Shao <laoar.shao@gmail.com> wrote:
> The task cannot modify cgroups if we have already acquired the
> css_set_lock, thus eliminating the need to hold the cgroup_mutex. Following
> this change, task_cgroup_from_root() can be employed in non-sleepable contexts.

IIRC, cset_cgroup_from_root() needs cgroup_mutex to make sure the `root`
doesn't disappear under hands (synchronization with
cgroup_destroy_root().
However, as I look at it now, cgroup_mutex won't synchronize against
cgroup_kill_sb(), it may worked by accident(?) nowadays (i.e. callers
pinned the root implicitly in another way).

Still, it'd be good to have an annotation that ensures, the root is around
when using it. (RCU read lock might be fine but you'd need
cgroup_get_live() if passing it out of the RCU read section.)

Basically, the code must be made safe against cgroup v1 unmounts.


Michal

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

Re: [RFC PATCH bpf-next 2/8] cgroup: Add new helpers for cgroup1 hierarchy

[Michal Koutný]

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

On Mon, Oct 09, 2023 at 09:10:04PM +0800, Yafang Shao <laoar.shao@gmail.com> wrote:
> Hao also pointed out the use case of a 3rd partry task[1].

Sigh (I must have expulsed that mail from my mind).

Is the WARN_ON_ONCE(!cgrp); of any use when
__cset_cgroup_from_root() has
        BUG_ON(!res_cgroup);
?

Thanks,
Michal

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

Re: [RFC PATCH bpf-next 1/8] cgroup: Don't have to hold cgroup_mutex in task_cgroup_from_root()

[Yafang Shao]

On Mon, Oct 9, 2023 at 10:45 PM Michal Koutný <mkoutny@suse.com> wrote:
>
> On Sat, Oct 07, 2023 at 02:02:57PM +0000, Yafang Shao <laoar.shao@gmail.com> wrote:
> > The task cannot modify cgroups if we have already acquired the
> > css_set_lock, thus eliminating the need to hold the cgroup_mutex. Following
> > this change, task_cgroup_from_root() can be employed in non-sleepable contexts.
>
> IIRC, cset_cgroup_from_root() needs cgroup_mutex to make sure the `root`
> doesn't disappear under hands (synchronization with
> cgroup_destroy_root().

current_cgns_cgroup_from_root() doesn't hold the cgroup_mutext as
well. Could this potentially lead to issues, such as triggering the
BUG_ON() in __cset_cgroup_from_root(), if the root has already been
destroyed?

> However, as I look at it now, cgroup_mutex won't synchronize against
> cgroup_kill_sb(), it may worked by accident(?) nowadays (i.e. callers
> pinned the root implicitly in another way).
>
> Still, it'd be good to have an annotation that ensures, the root is around
> when using it. (RCU read lock might be fine but you'd need
> cgroup_get_live() if passing it out of the RCU read section.)
>
> Basically, the code must be made safe against cgroup v1 unmounts.

What we aim to protect against is changes to the `root_list`, which
occur exclusively during cgroup setup and destroy paths. Would it be
beneficial to introduce a dedicated root_list_lock specifically for
this purpose? This approach could potentially reduce the need for the
broader cgroup_mutex in other scenarios.

--
Regards
Yafang

Re: [RFC PATCH bpf-next 2/8] cgroup: Add new helpers for cgroup1 hierarchy

[Yafang Shao]

On Mon, Oct 9, 2023 at 10:48 PM Michal Koutný <mkoutny@suse.com> wrote:
>
> On Mon, Oct 09, 2023 at 09:10:04PM +0800, Yafang Shao <laoar.shao@gmail.com> wrote:
> > Hao also pointed out the use case of a 3rd partry task[1].
>
> Sigh (I must have expulsed that mail from my mind).
>
> Is the WARN_ON_ONCE(!cgrp); of any use when
> __cset_cgroup_from_root() has
>         BUG_ON(!res_cgroup);
> ?

It is useless. will drop it. Thank you for pointing it out.

-- 
Regards
Yafang

Re: [RFC PATCH bpf-next 1/8] cgroup: Don't have to hold cgroup_mutex in task_cgroup_from_root()

[Michal Koutný]

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

Hi Yafang.

On Tue, Oct 10, 2023 at 11:58:14AM +0800, Yafang Shao <laoar.shao@gmail.com> wrote:
> current_cgns_cgroup_from_root() doesn't hold the cgroup_mutext as
> well. Could this potentially lead to issues, such as triggering the
> BUG_ON() in __cset_cgroup_from_root(), if the root has already been
> destroyed?

current_cgns_cgroup_from_root() is a tricky one, see also
https://lore.kernel.org/r/20230502133847.14570-3-mkoutny@suse.com/

I argued there with RCU read lock but when I look at it now, it may not be
sufficient for the cgroup returned from current_cgns_cgroup_from_root().

The 2nd half still applies, umount synchronization is ensured via VFS
layer, so the cgroup_root nor its cgroup won't go away in the
only caller cgroup_show_path().


> Would it be beneficial to introduce a dedicated root_list_lock
> specifically for this purpose? This approach could potentially reduce
> the need for the broader cgroup_mutex in other scenarios.

It may be a convenience lock but v2 (cgrp_dfl_root could make do just
fine without it).

I'm keeping this dicussuion to illustrate the difficulties of adding the
BPF support for cgroup v1. That is a benefit I see ;-)

Michal


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

Re: [RFC PATCH bpf-next 1/8] cgroup: Don't have to hold cgroup_mutex in task_cgroup_from_root()

[Yafang Shao]

On Tue, Oct 10, 2023 at 4:29 PM Michal Koutný <mkoutny@suse.com> wrote:
>
> Hi Yafang.
>
> On Tue, Oct 10, 2023 at 11:58:14AM +0800, Yafang Shao <laoar.shao@gmail.com> wrote:
> > current_cgns_cgroup_from_root() doesn't hold the cgroup_mutext as
> > well. Could this potentially lead to issues, such as triggering the
> > BUG_ON() in __cset_cgroup_from_root(), if the root has already been
> > destroyed?
>
> current_cgns_cgroup_from_root() is a tricky one, see also
> https://lore.kernel.org/r/20230502133847.14570-3-mkoutny@suse.com/
>
> I argued there with RCU read lock but when I look at it now, it may not be
> sufficient for the cgroup returned from current_cgns_cgroup_from_root().
>
> The 2nd half still applies, umount synchronization is ensured via VFS
> layer, so the cgroup_root nor its cgroup won't go away in the
> only caller cgroup_show_path().

Thanks for your explanation.

>
>
> > Would it be beneficial to introduce a dedicated root_list_lock
> > specifically for this purpose? This approach could potentially reduce
> > the need for the broader cgroup_mutex in other scenarios.
>
> It may be a convenience lock but v2 (cgrp_dfl_root could make do just
> fine without it).

Right. cgroup2 doesn't require it.

>
> I'm keeping this dicussuion to illustrate the difficulties of adding the
> BPF support for cgroup v1. That is a benefit I see ;-)

A potentially more effective approach might be to employ RCU
protection for the cgroup_list. That said, use
list_for_each_entry_rcu/list_add_rcu/list_del_rcu instead.

--
Regards

Yafang