linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 bpf-next 0/5] Introduce bpf_cgroup_read_xattr
@ 2025-06-19 22:01 Song Liu
  2025-06-19 22:01 ` [PATCH v2 bpf-next 1/5] kernfs: remove iattr_mutex Song Liu
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Song Liu @ 2025-06-19 22:01 UTC (permalink / raw)
  To: bpf, linux-fsdevel, linux-kernel, linux-security-module
  Cc: kernel-team, andrii, eddyz87, ast, daniel, martin.lau, viro,
	brauner, jack, kpsingh, mattbobrowski, amir73il, gregkh, tj,
	daan.j.demeyer, Song Liu

Introduce a new kfunc bpf_cgroup_read_xattr, which can read xattr from
cgroupfs nodes. The primary users are LSMs, cgroup programs, and sched_ext.

---

Changes v1 => v2:
1. Replace 1/4 in v1 with Chritian's version (1/5 in v2).
2. Rename bpf_kernfs_read_xattr => bpf_cgroup_read_xattr, and limit access
   to cgroup only.
3. Add 5/5, which makes bpf_cgroup_read_xattr available to cgroup and
   struct_ops programs.

v1: https://lore.kernel.org/bpf/20250618233739.189106-1-song@kernel.org/

Christian Brauner (1):
  kernfs: remove iattr_mutex

Song Liu (4):
  bpf: Introduce bpf_cgroup_read_xattr to read xattr of cgroup's node
  bpf: Mark cgroup_subsys_state->cgroup RCU safe
  selftests/bpf: Add tests for bpf_cgroup_read_xattr
  bpf: Make bpf_cgroup_read_xattr available to cgroup and struct_ops
    progs

 fs/bpf_fs_kfuncs.c                            |  86 +++++++++-
 fs/kernfs/inode.c                             |  74 ++++----
 kernel/bpf/verifier.c                         |   5 +
 .../selftests/bpf/prog_tests/cgroup_xattr.c   | 145 ++++++++++++++++
 .../selftests/bpf/progs/cgroup_read_xattr.c   | 158 ++++++++++++++++++
 .../selftests/bpf/progs/read_cgroupfs_xattr.c |  60 +++++++
 6 files changed, 489 insertions(+), 39 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_xattr.c
 create mode 100644 tools/testing/selftests/bpf/progs/cgroup_read_xattr.c
 create mode 100644 tools/testing/selftests/bpf/progs/read_cgroupfs_xattr.c

--
2.47.1

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2 bpf-next 1/5] kernfs: remove iattr_mutex
  2025-06-19 22:01 [PATCH v2 bpf-next 0/5] Introduce bpf_cgroup_read_xattr Song Liu
@ 2025-06-19 22:01 ` Song Liu
  2025-06-19 22:01 ` [PATCH v2 bpf-next 2/5] bpf: Introduce bpf_cgroup_read_xattr to read xattr of cgroup's node Song Liu
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 14+ messages in thread
From: Song Liu @ 2025-06-19 22:01 UTC (permalink / raw)
  To: bpf, linux-fsdevel, linux-kernel, linux-security-module
  Cc: kernel-team, andrii, eddyz87, ast, daniel, martin.lau, viro,
	brauner, jack, kpsingh, mattbobrowski, amir73il, gregkh, tj,
	daan.j.demeyer, Song Liu

From: Christian Brauner <brauner@kernel.org>

All allocations of struct kernfs_iattrs are serialized through a global
mutex. Simply do a racy allocation and let the first one win. I bet most
callers are under inode->i_rwsem anyway and it wouldn't be needed but
let's not require that.

Signed-off-by: Christian Brauner <brauner@kernel.org>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Song Liu <song@kernel.org>
---
 fs/kernfs/inode.c | 74 +++++++++++++++++++++++++----------------------
 1 file changed, 40 insertions(+), 34 deletions(-)

diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
index b83054da68b3..f4b73b9482b7 100644
--- a/fs/kernfs/inode.c
+++ b/fs/kernfs/inode.c
@@ -24,45 +24,46 @@ static const struct inode_operations kernfs_iops = {
 	.listxattr	= kernfs_iop_listxattr,
 };
 
-static struct kernfs_iattrs *__kernfs_iattrs(struct kernfs_node *kn, int alloc)
+static struct kernfs_iattrs *__kernfs_iattrs(struct kernfs_node *kn, bool alloc)
 {
-	static DEFINE_MUTEX(iattr_mutex);
-	struct kernfs_iattrs *ret;
+	struct kernfs_iattrs *ret __free(kfree) = NULL;
+	struct kernfs_iattrs *attr;
 
-	mutex_lock(&iattr_mutex);
+	attr = READ_ONCE(kn->iattr);
+	if (attr || !alloc)
+		return attr;
 
-	if (kn->iattr || !alloc)
-		goto out_unlock;
-
-	kn->iattr = kmem_cache_zalloc(kernfs_iattrs_cache, GFP_KERNEL);
-	if (!kn->iattr)
-		goto out_unlock;
+	ret = kmem_cache_zalloc(kernfs_iattrs_cache, GFP_KERNEL);
+	if (!ret)
+		return NULL;
 
 	/* assign default attributes */
-	kn->iattr->ia_uid = GLOBAL_ROOT_UID;
-	kn->iattr->ia_gid = GLOBAL_ROOT_GID;
-
-	ktime_get_real_ts64(&kn->iattr->ia_atime);
-	kn->iattr->ia_mtime = kn->iattr->ia_atime;
-	kn->iattr->ia_ctime = kn->iattr->ia_atime;
-
-	simple_xattrs_init(&kn->iattr->xattrs);
-	atomic_set(&kn->iattr->nr_user_xattrs, 0);
-	atomic_set(&kn->iattr->user_xattr_size, 0);
-out_unlock:
-	ret = kn->iattr;
-	mutex_unlock(&iattr_mutex);
-	return ret;
+	ret->ia_uid = GLOBAL_ROOT_UID;
+	ret->ia_gid = GLOBAL_ROOT_GID;
+
+	ktime_get_real_ts64(&ret->ia_atime);
+	ret->ia_mtime = ret->ia_atime;
+	ret->ia_ctime = ret->ia_atime;
+
+	simple_xattrs_init(&ret->xattrs);
+	atomic_set(&ret->nr_user_xattrs, 0);
+	atomic_set(&ret->user_xattr_size, 0);
+
+	/* If someone raced us, recognize it. */
+	if (!try_cmpxchg(&kn->iattr, &attr, ret))
+		return READ_ONCE(kn->iattr);
+
+	return no_free_ptr(ret);
 }
 
 static struct kernfs_iattrs *kernfs_iattrs(struct kernfs_node *kn)
 {
-	return __kernfs_iattrs(kn, 1);
+	return __kernfs_iattrs(kn, true);
 }
 
 static struct kernfs_iattrs *kernfs_iattrs_noalloc(struct kernfs_node *kn)
 {
-	return __kernfs_iattrs(kn, 0);
+	return __kernfs_iattrs(kn, false);
 }
 
 int __kernfs_setattr(struct kernfs_node *kn, const struct iattr *iattr)
@@ -141,9 +142,9 @@ ssize_t kernfs_iop_listxattr(struct dentry *dentry, char *buf, size_t size)
 	struct kernfs_node *kn = kernfs_dentry_node(dentry);
 	struct kernfs_iattrs *attrs;
 
-	attrs = kernfs_iattrs(kn);
+	attrs = kernfs_iattrs_noalloc(kn);
 	if (!attrs)
-		return -ENOMEM;
+		return -ENODATA;
 
 	return simple_xattr_list(d_inode(dentry), &attrs->xattrs, buf, size);
 }
@@ -166,9 +167,10 @@ static inline void set_inode_attr(struct inode *inode,
 
 static void kernfs_refresh_inode(struct kernfs_node *kn, struct inode *inode)
 {
-	struct kernfs_iattrs *attrs = kn->iattr;
+	struct kernfs_iattrs *attrs;
 
 	inode->i_mode = kn->mode;
+	attrs = kernfs_iattrs_noalloc(kn);
 	if (attrs)
 		/*
 		 * kernfs_node has non-default attributes get them from
@@ -306,7 +308,9 @@ int kernfs_xattr_set(struct kernfs_node *kn, const char *name,
 		     const void *value, size_t size, int flags)
 {
 	struct simple_xattr *old_xattr;
-	struct kernfs_iattrs *attrs = kernfs_iattrs(kn);
+	struct kernfs_iattrs *attrs;
+
+	attrs = kernfs_iattrs(kn);
 	if (!attrs)
 		return -ENOMEM;
 
@@ -345,8 +349,9 @@ static int kernfs_vfs_user_xattr_add(struct kernfs_node *kn,
 				     struct simple_xattrs *xattrs,
 				     const void *value, size_t size, int flags)
 {
-	atomic_t *sz = &kn->iattr->user_xattr_size;
-	atomic_t *nr = &kn->iattr->nr_user_xattrs;
+	struct kernfs_iattrs *attr = kernfs_iattrs_noalloc(kn);
+	atomic_t *sz = &attr->user_xattr_size;
+	atomic_t *nr = &attr->nr_user_xattrs;
 	struct simple_xattr *old_xattr;
 	int ret;
 
@@ -384,8 +389,9 @@ static int kernfs_vfs_user_xattr_rm(struct kernfs_node *kn,
 				    struct simple_xattrs *xattrs,
 				    const void *value, size_t size, int flags)
 {
-	atomic_t *sz = &kn->iattr->user_xattr_size;
-	atomic_t *nr = &kn->iattr->nr_user_xattrs;
+	struct kernfs_iattrs *attr = kernfs_iattrs(kn);
+	atomic_t *sz = &attr->user_xattr_size;
+	atomic_t *nr = &attr->nr_user_xattrs;
 	struct simple_xattr *old_xattr;
 
 	old_xattr = simple_xattr_set(xattrs, full_name, value, size, flags);
-- 
2.47.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 bpf-next 2/5] bpf: Introduce bpf_cgroup_read_xattr to read xattr of cgroup's node
  2025-06-19 22:01 [PATCH v2 bpf-next 0/5] Introduce bpf_cgroup_read_xattr Song Liu
  2025-06-19 22:01 ` [PATCH v2 bpf-next 1/5] kernfs: remove iattr_mutex Song Liu
@ 2025-06-19 22:01 ` Song Liu
  2025-06-21  2:44   ` Tejun Heo
  2025-06-19 22:01 ` [PATCH v2 bpf-next 3/5] bpf: Mark cgroup_subsys_state->cgroup RCU safe Song Liu
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Song Liu @ 2025-06-19 22:01 UTC (permalink / raw)
  To: bpf, linux-fsdevel, linux-kernel, linux-security-module
  Cc: kernel-team, andrii, eddyz87, ast, daniel, martin.lau, viro,
	brauner, jack, kpsingh, mattbobrowski, amir73il, gregkh, tj,
	daan.j.demeyer, Song Liu

BPF programs, such as LSM and sched_ext, would benefit from tags on
cgroups. One common practice to apply such tags is to set xattrs on
cgroupfs folders.

Introduce kfunc bpf_cgroup_read_xattr, which allows reading cgroup's
xattr.

Note that, we already have bpf_get_[file|dentry]_xattr. However, these
two APIs are not ideal for reading cgroupfs xattrs, because:

  1) These two APIs only works in sleepable contexts;
  2) There is no kfunc that matches current cgroup to cgroupfs dentry.

Signed-off-by: Song Liu <song@kernel.org>
---
 fs/bpf_fs_kfuncs.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/fs/bpf_fs_kfuncs.c b/fs/bpf_fs_kfuncs.c
index 08412532db1b..9f3f9bd0f6f7 100644
--- a/fs/bpf_fs_kfuncs.c
+++ b/fs/bpf_fs_kfuncs.c
@@ -9,6 +9,7 @@
 #include <linux/fs.h>
 #include <linux/fsnotify.h>
 #include <linux/file.h>
+#include <linux/kernfs.h>
 #include <linux/mm.h>
 #include <linux/xattr.h>
 
@@ -322,6 +323,37 @@ __bpf_kfunc int bpf_remove_dentry_xattr(struct dentry *dentry, const char *name_
 	return ret;
 }
 
+/**
+ * bpf_cgroup_read_xattr - read xattr of a cgroup's node in cgroupfs
+ * @cgroup: cgroup to get xattr from
+ * @name__str: name of the xattr
+ * @value_p: output buffer of the xattr value
+ *
+ * Get xattr *name__str* of *cgroup* and store the output in *value_ptr*.
+ *
+ * For security reasons, only *name__str* with prefix "user." is allowed.
+ *
+ * Return: length of the xattr value on success, a negative value on error.
+ */
+__bpf_kfunc int bpf_cgroup_read_xattr(struct cgroup *cgroup, const char *name__str,
+					struct bpf_dynptr *value_p)
+{
+	struct bpf_dynptr_kern *value_ptr = (struct bpf_dynptr_kern *)value_p;
+	u32 value_len;
+	void *value;
+
+	/* Only allow reading "user.*" xattrs */
+	if (strncmp(name__str, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN))
+		return -EPERM;
+
+	value_len = __bpf_dynptr_size(value_ptr);
+	value = __bpf_dynptr_data_rw(value_ptr, value_len);
+	if (!value)
+		return -EINVAL;
+
+	return kernfs_xattr_get(cgroup->kn, name__str, value, value_len);
+}
+
 __bpf_kfunc_end_defs();
 
 BTF_KFUNCS_START(bpf_fs_kfunc_set_ids)
@@ -333,6 +365,7 @@ BTF_ID_FLAGS(func, bpf_get_dentry_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_set_dentry_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_remove_dentry_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_cgroup_read_xattr, KF_RCU)
 BTF_KFUNCS_END(bpf_fs_kfunc_set_ids)
 
 static int bpf_fs_kfuncs_filter(const struct bpf_prog *prog, u32 kfunc_id)
-- 
2.47.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 bpf-next 3/5] bpf: Mark cgroup_subsys_state->cgroup RCU safe
  2025-06-19 22:01 [PATCH v2 bpf-next 0/5] Introduce bpf_cgroup_read_xattr Song Liu
  2025-06-19 22:01 ` [PATCH v2 bpf-next 1/5] kernfs: remove iattr_mutex Song Liu
  2025-06-19 22:01 ` [PATCH v2 bpf-next 2/5] bpf: Introduce bpf_cgroup_read_xattr to read xattr of cgroup's node Song Liu
@ 2025-06-19 22:01 ` Song Liu
  2025-06-21  2:45   ` Tejun Heo
  2025-06-19 22:01 ` [PATCH v2 bpf-next 4/5] selftests/bpf: Add tests for bpf_cgroup_read_xattr Song Liu
  2025-06-19 22:01 ` [PATCH v2 bpf-next 5/5] bpf: Make bpf_cgroup_read_xattr available to cgroup and struct_ops progs Song Liu
  4 siblings, 1 reply; 14+ messages in thread
From: Song Liu @ 2025-06-19 22:01 UTC (permalink / raw)
  To: bpf, linux-fsdevel, linux-kernel, linux-security-module
  Cc: kernel-team, andrii, eddyz87, ast, daniel, martin.lau, viro,
	brauner, jack, kpsingh, mattbobrowski, amir73il, gregkh, tj,
	daan.j.demeyer, Song Liu

Mark struct cgroup_subsys_state->cgroup as safe under RCU read lock. This
will enable accessing css->cgroup from a bpf css iterator.

Signed-off-by: Song Liu <song@kernel.org>
---
 kernel/bpf/verifier.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 279a64933262..e2f53dc8766a 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7058,6 +7058,10 @@ BTF_TYPE_SAFE_RCU(struct css_set) {
 	struct cgroup *dfl_cgrp;
 };
 
+BTF_TYPE_SAFE_RCU(struct cgroup_subsys_state) {
+	struct cgroup *cgroup;
+};
+
 /* RCU trusted: these fields are trusted in RCU CS and can be NULL */
 BTF_TYPE_SAFE_RCU_OR_NULL(struct mm_struct) {
 	struct file __rcu *exe_file;
@@ -7108,6 +7112,7 @@ static bool type_is_rcu(struct bpf_verifier_env *env,
 	BTF_TYPE_EMIT(BTF_TYPE_SAFE_RCU(struct task_struct));
 	BTF_TYPE_EMIT(BTF_TYPE_SAFE_RCU(struct cgroup));
 	BTF_TYPE_EMIT(BTF_TYPE_SAFE_RCU(struct css_set));
+	BTF_TYPE_EMIT(BTF_TYPE_SAFE_RCU(struct cgroup_subsys_state));
 
 	return btf_nested_type_is_trusted(&env->log, reg, field_name, btf_id, "__safe_rcu");
 }
-- 
2.47.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 bpf-next 4/5] selftests/bpf: Add tests for bpf_cgroup_read_xattr
  2025-06-19 22:01 [PATCH v2 bpf-next 0/5] Introduce bpf_cgroup_read_xattr Song Liu
                   ` (2 preceding siblings ...)
  2025-06-19 22:01 ` [PATCH v2 bpf-next 3/5] bpf: Mark cgroup_subsys_state->cgroup RCU safe Song Liu
@ 2025-06-19 22:01 ` Song Liu
  2025-06-20 18:11   ` Alexei Starovoitov
  2025-06-19 22:01 ` [PATCH v2 bpf-next 5/5] bpf: Make bpf_cgroup_read_xattr available to cgroup and struct_ops progs Song Liu
  4 siblings, 1 reply; 14+ messages in thread
From: Song Liu @ 2025-06-19 22:01 UTC (permalink / raw)
  To: bpf, linux-fsdevel, linux-kernel, linux-security-module
  Cc: kernel-team, andrii, eddyz87, ast, daniel, martin.lau, viro,
	brauner, jack, kpsingh, mattbobrowski, amir73il, gregkh, tj,
	daan.j.demeyer, Song Liu

Add tests for different scenarios with bpf_cgroup_read_xattr:
1. Read cgroup xattr from bpf_cgroup_from_id;
2. Read cgroup xattr from bpf_cgroup_ancestor;
3. Read cgroup xattr from css_iter;
4. Use bpf_cgroup_read_xattr in LSM hook security_socket_connect.

Signed-off-by: Song Liu <song@kernel.org>
---
 .../selftests/bpf/prog_tests/cgroup_xattr.c   | 145 ++++++++++++++++++
 .../selftests/bpf/progs/cgroup_read_xattr.c   | 136 ++++++++++++++++
 .../selftests/bpf/progs/read_cgroupfs_xattr.c |  60 ++++++++
 3 files changed, 341 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_xattr.c
 create mode 100644 tools/testing/selftests/bpf/progs/cgroup_read_xattr.c
 create mode 100644 tools/testing/selftests/bpf/progs/read_cgroupfs_xattr.c

diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_xattr.c b/tools/testing/selftests/bpf/prog_tests/cgroup_xattr.c
new file mode 100644
index 000000000000..87978a0f7eb7
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/cgroup_xattr.c
@@ -0,0 +1,145 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (c) 2025 Meta Platforms, Inc. and affiliates. */
+
+#include <errno.h>
+#include <fcntl.h>
+#include <sys/stat.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/socket.h>
+#include <sys/xattr.h>
+
+#include <test_progs.h>
+
+#include "read_cgroupfs_xattr.skel.h"
+#include "cgroup_read_xattr.skel.h"
+
+#define CGROUP_FS_ROOT "/sys/fs/cgroup/"
+#define CGROUP_FS_PARENT CGROUP_FS_ROOT "foo/"
+#define CGROUP_FS_CHILD CGROUP_FS_PARENT "bar/"
+
+static int move_pid_to_cgroup(const char *cgroup_folder, pid_t pid)
+{
+	char filename[128];
+	char pid_str[64];
+	int procs_fd;
+	int ret;
+
+	snprintf(filename, sizeof(filename), "%scgroup.procs", cgroup_folder);
+	snprintf(pid_str, sizeof(pid_str), "%d", pid);
+
+	procs_fd = open(filename, O_WRONLY | O_APPEND);
+	if (!ASSERT_OK_FD(procs_fd, "open"))
+		return -1;
+
+	ret = write(procs_fd, pid_str, strlen(pid_str));
+	close(procs_fd);
+	if (!ASSERT_GT(ret, 0, "write cgroup.procs"))
+		return -1;
+	return 0;
+}
+
+static void reset_cgroups_and_lo(void)
+{
+	rmdir(CGROUP_FS_CHILD);
+	rmdir(CGROUP_FS_PARENT);
+	system("ip addr del 1.1.1.1/32 dev lo");
+	system("ip link set dev lo down");
+}
+
+static const char xattr_value_a[] = "bpf_selftest_value_a";
+static const char xattr_value_b[] = "bpf_selftest_value_b";
+static const char xattr_name[] = "user.bpf_test";
+
+static int setup_cgroups_and_lo(void)
+{
+	int err;
+
+	err = mkdir(CGROUP_FS_PARENT, 0755);
+	if (!ASSERT_OK(err, "mkdir 1"))
+		goto error;
+	err = mkdir(CGROUP_FS_CHILD, 0755);
+	if (!ASSERT_OK(err, "mkdir 2"))
+		goto error;
+
+	err = setxattr(CGROUP_FS_PARENT, xattr_name, xattr_value_a,
+		       strlen(xattr_value_a) + 1, 0);
+	if (!ASSERT_OK(err, "setxattr 1"))
+		goto error;
+
+	err = setxattr(CGROUP_FS_CHILD, xattr_name, xattr_value_b,
+		       strlen(xattr_value_b) + 1, 0);
+	if (!ASSERT_OK(err, "setxattr 2"))
+		goto error;
+
+	err = system("ip link set dev lo up");
+	if (!ASSERT_OK(err, "lo up"))
+		goto error;
+
+	err = system("ip addr add 1.1.1.1 dev lo");
+	if (!ASSERT_OK(err, "lo addr v4"))
+		goto error;
+
+	err = write_sysctl("/proc/sys/net/ipv4/ping_group_range", "0 0");
+	if (!ASSERT_OK(err, "write_sysctl"))
+		goto error;
+
+	return 0;
+error:
+	reset_cgroups_and_lo();
+	return err;
+}
+
+static void test_read_cgroup_xattr(void)
+{
+	struct sockaddr_in sa4 = {
+		.sin_family = AF_INET,
+		.sin_addr.s_addr = htonl(INADDR_LOOPBACK),
+	};
+	struct read_cgroupfs_xattr *skel = NULL;
+	pid_t pid = gettid();
+	int sock_fd = -1;
+	int connect_fd = -1;
+
+	if (!ASSERT_OK(setup_cgroups_and_lo(), "setup_cgroups_and_lo"))
+		return;
+	if (!ASSERT_OK(move_pid_to_cgroup(CGROUP_FS_CHILD, pid),
+		       "move_pid_to_cgroup"))
+		goto out;
+
+	skel = read_cgroupfs_xattr__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "read_cgroupfs_xattr__open_and_load"))
+		goto out;
+
+	skel->bss->target_pid = pid;
+
+	if (!ASSERT_OK(read_cgroupfs_xattr__attach(skel), "read_cgroupfs_xattr__attach"))
+		goto out;
+
+	sock_fd = socket(AF_INET, SOCK_DGRAM, IPPROTO_ICMP);
+	if (!ASSERT_OK_FD(sock_fd, "sock create"))
+		goto out;
+
+	connect_fd = connect(sock_fd, &sa4, sizeof(sa4));
+	if (!ASSERT_OK_FD(connect_fd, "connect 1"))
+		goto out;
+	close(connect_fd);
+
+	ASSERT_TRUE(skel->bss->found_value_a, "found_value_a");
+	ASSERT_TRUE(skel->bss->found_value_b, "found_value_b");
+
+out:
+	close(connect_fd);
+	close(sock_fd);
+	read_cgroupfs_xattr__destroy(skel);
+	move_pid_to_cgroup(CGROUP_FS_ROOT, pid);
+	reset_cgroups_and_lo();
+}
+
+void test_cgroup_xattr(void)
+{
+	RUN_TESTS(cgroup_read_xattr);
+
+	if (test__start_subtest("read_cgroupfs_xattr"))
+		test_read_cgroup_xattr();
+}
diff --git a/tools/testing/selftests/bpf/progs/cgroup_read_xattr.c b/tools/testing/selftests/bpf/progs/cgroup_read_xattr.c
new file mode 100644
index 000000000000..b50ccb3aebcf
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/cgroup_read_xattr.c
@@ -0,0 +1,136 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2025 Meta Platforms, Inc. and affiliates. */
+
+#include <vmlinux.h>
+#include <bpf/bpf_tracing.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_core_read.h>
+#include "bpf_experimental.h"
+#include "bpf_misc.h"
+
+char _license[] SEC("license") = "GPL";
+
+char value[16];
+
+__always_inline void read_xattr(struct cgroup *cgroup)
+{
+	struct bpf_dynptr value_ptr;
+
+	bpf_dynptr_from_mem(value, sizeof(value), 0, &value_ptr);
+	bpf_cgroup_read_xattr(cgroup, "user.bpf_test",
+			      &value_ptr);
+}
+
+SEC("lsm.s/socket_connect")
+__success
+int BPF_PROG(trusted_cgroup_ptr_sleepable)
+{
+	u64 cgrp_id = bpf_get_current_cgroup_id();
+	struct cgroup *cgrp;
+
+	cgrp = bpf_cgroup_from_id(cgrp_id);
+	if (!cgrp)
+		return 0;
+
+	read_xattr(cgrp);
+	bpf_cgroup_release(cgrp);
+	return 0;
+}
+
+SEC("lsm/socket_connect")
+__success
+int BPF_PROG(trusted_cgroup_ptr_non_sleepable)
+{
+	u64 cgrp_id = bpf_get_current_cgroup_id();
+	struct cgroup *cgrp;
+
+	cgrp = bpf_cgroup_from_id(cgrp_id);
+	if (!cgrp)
+		return 0;
+
+	read_xattr(cgrp);
+	bpf_cgroup_release(cgrp);
+	return 0;
+}
+
+SEC("lsm/socket_connect")
+__success
+int BPF_PROG(use_css_iter_non_sleepable)
+{
+	u64 cgrp_id = bpf_get_current_cgroup_id();
+	struct cgroup_subsys_state *css;
+	struct cgroup *cgrp;
+
+	cgrp = bpf_cgroup_from_id(cgrp_id);
+	if (!cgrp)
+		return 0;
+
+	bpf_for_each(css, css, &cgrp->self, BPF_CGROUP_ITER_ANCESTORS_UP)
+		read_xattr(css->cgroup);
+
+	bpf_cgroup_release(cgrp);
+	return 0;
+}
+
+SEC("lsm.s/socket_connect")
+__failure __msg("expected an RCU CS")
+int BPF_PROG(use_css_iter_sleepable_missing_rcu_lock)
+{
+	u64 cgrp_id = bpf_get_current_cgroup_id();
+	struct cgroup_subsys_state *css;
+	struct cgroup *cgrp;
+
+	cgrp = bpf_cgroup_from_id(cgrp_id);
+	if (!cgrp)
+		return 0;
+
+	bpf_for_each(css, css, &cgrp->self, BPF_CGROUP_ITER_ANCESTORS_UP)
+		read_xattr(css->cgroup);
+
+	bpf_cgroup_release(cgrp);
+	return 0;
+}
+
+SEC("lsm.s/socket_connect")
+__success
+int BPF_PROG(use_css_iter_sleepable_with_rcu_lock)
+{
+	u64 cgrp_id = bpf_get_current_cgroup_id();
+	struct cgroup_subsys_state *css;
+	struct cgroup *cgrp;
+
+	bpf_rcu_read_lock();
+	cgrp = bpf_cgroup_from_id(cgrp_id);
+	if (!cgrp)
+		goto out;
+
+	bpf_for_each(css, css, &cgrp->self, BPF_CGROUP_ITER_ANCESTORS_UP)
+		read_xattr(css->cgroup);
+
+	bpf_cgroup_release(cgrp);
+out:
+	bpf_rcu_read_unlock();
+	return 0;
+}
+
+SEC("lsm/socket_connect")
+__success
+int BPF_PROG(use_bpf_cgroup_ancestor)
+{
+	u64 cgrp_id = bpf_get_current_cgroup_id();
+	struct cgroup *cgrp, *ancestor;
+
+	cgrp = bpf_cgroup_from_id(cgrp_id);
+	if (!cgrp)
+		return 0;
+
+	ancestor = bpf_cgroup_ancestor(cgrp, 1);
+	if (!ancestor)
+		goto out;
+
+	read_xattr(cgrp);
+	bpf_cgroup_release(ancestor);
+out:
+	bpf_cgroup_release(cgrp);
+	return 0;
+}
diff --git a/tools/testing/selftests/bpf/progs/read_cgroupfs_xattr.c b/tools/testing/selftests/bpf/progs/read_cgroupfs_xattr.c
new file mode 100644
index 000000000000..855f85fc5522
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/read_cgroupfs_xattr.c
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2025 Meta Platforms, Inc. and affiliates. */
+
+#include <vmlinux.h>
+#include <bpf/bpf_tracing.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_core_read.h>
+#include "bpf_experimental.h"
+
+char _license[] SEC("license") = "GPL";
+
+pid_t target_pid = 0;
+
+char xattr_value[64];
+static const char expected_value_a[] = "bpf_selftest_value_a";
+static const char expected_value_b[] = "bpf_selftest_value_b";
+bool found_value_a;
+bool found_value_b;
+
+SEC("lsm.s/socket_connect")
+int BPF_PROG(test_socket_connect)
+{
+	u64 cgrp_id = bpf_get_current_cgroup_id();
+	struct cgroup_subsys_state *css, *tmp;
+	struct bpf_dynptr value_ptr;
+	struct cgroup *cgrp;
+
+	if ((bpf_get_current_pid_tgid() >> 32) != target_pid)
+		return 0;
+
+	bpf_rcu_read_lock();
+	cgrp = bpf_cgroup_from_id(cgrp_id);
+	if (!cgrp) {
+		bpf_rcu_read_unlock();
+		return 0;
+	}
+
+	css = &cgrp->self;
+	bpf_dynptr_from_mem(xattr_value, sizeof(xattr_value), 0, &value_ptr);
+	bpf_for_each(css, tmp, css, BPF_CGROUP_ITER_ANCESTORS_UP) {
+		int ret;
+
+		ret = bpf_cgroup_read_xattr(tmp->cgroup, "user.bpf_test",
+					    &value_ptr);
+		if (ret < 0)
+			continue;
+
+		if (ret == sizeof(expected_value_a) &&
+		    !bpf_strncmp(xattr_value, sizeof(expected_value_a), expected_value_a))
+			found_value_a = true;
+		if (ret == sizeof(expected_value_b) &&
+		    !bpf_strncmp(xattr_value, sizeof(expected_value_b), expected_value_b))
+			found_value_b = true;
+	}
+
+	bpf_rcu_read_unlock();
+	bpf_cgroup_release(cgrp);
+
+	return 0;
+}
-- 
2.47.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 bpf-next 5/5] bpf: Make bpf_cgroup_read_xattr available to cgroup and struct_ops progs
  2025-06-19 22:01 [PATCH v2 bpf-next 0/5] Introduce bpf_cgroup_read_xattr Song Liu
                   ` (3 preceding siblings ...)
  2025-06-19 22:01 ` [PATCH v2 bpf-next 4/5] selftests/bpf: Add tests for bpf_cgroup_read_xattr Song Liu
@ 2025-06-19 22:01 ` Song Liu
  2025-06-20 18:18   ` Alexei Starovoitov
  4 siblings, 1 reply; 14+ messages in thread
From: Song Liu @ 2025-06-19 22:01 UTC (permalink / raw)
  To: bpf, linux-fsdevel, linux-kernel, linux-security-module
  Cc: kernel-team, andrii, eddyz87, ast, daniel, martin.lau, viro,
	brauner, jack, kpsingh, mattbobrowski, amir73il, gregkh, tj,
	daan.j.demeyer, Song Liu

cgroup BPF programs and struct_ops BPF programs (such as sched_ext), need
bpf_cgroup_read_xattr. Make bpf_cgroup_read_xattr available to these prog
types.

Rename bpf_fs_kfunc_* variables as bpf_lsm_fs_kfunc_*, as these are only
available to BPF LSM programs. Then, reuse bpf_fs_kfunc_* name for cgroup
and struct_ops prog typs.

Also add a selftest with program of "cgroup/sendmsg4" type.

Signed-off-by: Song Liu <song@kernel.org>
---
 fs/bpf_fs_kfuncs.c                            | 53 +++++++++++++++++--
 .../selftests/bpf/progs/cgroup_read_xattr.c   | 22 ++++++++
 2 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/fs/bpf_fs_kfuncs.c b/fs/bpf_fs_kfuncs.c
index 9f3f9bd0f6f7..8e02e09e092e 100644
--- a/fs/bpf_fs_kfuncs.c
+++ b/fs/bpf_fs_kfuncs.c
@@ -356,7 +356,7 @@ __bpf_kfunc int bpf_cgroup_read_xattr(struct cgroup *cgroup, const char *name__s
 
 __bpf_kfunc_end_defs();
 
-BTF_KFUNCS_START(bpf_fs_kfunc_set_ids)
+BTF_KFUNCS_START(bpf_lsm_fs_kfunc_set_ids)
 BTF_ID_FLAGS(func, bpf_get_task_exe_file,
 	     KF_ACQUIRE | KF_TRUSTED_ARGS | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_put_file, KF_RELEASE)
@@ -366,11 +366,11 @@ BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_set_dentry_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_remove_dentry_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_cgroup_read_xattr, KF_RCU)
-BTF_KFUNCS_END(bpf_fs_kfunc_set_ids)
+BTF_KFUNCS_END(bpf_lsm_fs_kfunc_set_ids)
 
-static int bpf_fs_kfuncs_filter(const struct bpf_prog *prog, u32 kfunc_id)
+static int bpf_lsm_fs_kfuncs_filter(const struct bpf_prog *prog, u32 kfunc_id)
 {
-	if (!btf_id_set8_contains(&bpf_fs_kfunc_set_ids, kfunc_id) ||
+	if (!btf_id_set8_contains(&bpf_lsm_fs_kfunc_set_ids, kfunc_id) ||
 	    prog->type == BPF_PROG_TYPE_LSM)
 		return 0;
 	return -EACCES;
@@ -407,6 +407,40 @@ bool bpf_lsm_has_d_inode_locked(const struct bpf_prog *prog)
 	return btf_id_set_contains(&d_inode_locked_hooks, prog->aux->attach_btf_id);
 }
 
+static const struct btf_kfunc_id_set bpf_lsm_fs_kfunc_set = {
+	.owner = THIS_MODULE,
+	.set = &bpf_lsm_fs_kfunc_set_ids,
+	.filter = bpf_lsm_fs_kfuncs_filter,
+};
+
+/*
+ * This set contains kfuncs available to BPF programs of cgroup type and
+ * struct_ops type.
+ */
+BTF_KFUNCS_START(bpf_fs_kfunc_set_ids)
+BTF_ID_FLAGS(func, bpf_cgroup_read_xattr, KF_RCU)
+BTF_KFUNCS_END(bpf_fs_kfunc_set_ids)
+
+static int bpf_fs_kfuncs_filter(const struct bpf_prog *prog, u32 kfunc_id)
+{
+	if (!btf_id_set8_contains(&bpf_fs_kfunc_set_ids, kfunc_id))
+		return 0;
+	switch (prog->type) {
+	case BPF_PROG_TYPE_LSM:
+	case BPF_PROG_TYPE_STRUCT_OPS:
+	case BPF_PROG_TYPE_CGROUP_SKB:
+	case BPF_PROG_TYPE_CGROUP_SOCK:
+	case BPF_PROG_TYPE_CGROUP_DEVICE:
+	case BPF_PROG_TYPE_CGROUP_SOCK_ADDR:
+	case BPF_PROG_TYPE_CGROUP_SYSCTL:
+	case BPF_PROG_TYPE_CGROUP_SOCKOPT:
+		return 0;
+	default:
+		break;
+	}
+	return -EACCES;
+}
+
 static const struct btf_kfunc_id_set bpf_fs_kfunc_set = {
 	.owner = THIS_MODULE,
 	.set = &bpf_fs_kfunc_set_ids,
@@ -415,7 +449,16 @@ static const struct btf_kfunc_id_set bpf_fs_kfunc_set = {
 
 static int __init bpf_fs_kfuncs_init(void)
 {
-	return register_btf_kfunc_id_set(BPF_PROG_TYPE_LSM, &bpf_fs_kfunc_set);
+	int ret;
+
+	ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_LSM, &bpf_lsm_fs_kfunc_set);
+	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, &bpf_fs_kfunc_set);
+	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SKB, &bpf_fs_kfunc_set);
+	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCK, &bpf_fs_kfunc_set);
+	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_DEVICE, &bpf_fs_kfunc_set);
+	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCK_ADDR, &bpf_fs_kfunc_set);
+	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SYSCTL, &bpf_fs_kfunc_set);
+	return ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCKOPT, &bpf_fs_kfunc_set);
 }
 
 late_initcall(bpf_fs_kfuncs_init);
diff --git a/tools/testing/selftests/bpf/progs/cgroup_read_xattr.c b/tools/testing/selftests/bpf/progs/cgroup_read_xattr.c
index b50ccb3aebcf..0995fb2ac9ff 100644
--- a/tools/testing/selftests/bpf/progs/cgroup_read_xattr.c
+++ b/tools/testing/selftests/bpf/progs/cgroup_read_xattr.c
@@ -134,3 +134,25 @@ int BPF_PROG(use_bpf_cgroup_ancestor)
 	bpf_cgroup_release(cgrp);
 	return 0;
 }
+
+SEC("cgroup/sendmsg4")
+__success
+int BPF_PROG(cgroup_skb)
+{
+	u64 cgrp_id = bpf_get_current_cgroup_id();
+	struct cgroup *cgrp, *ancestor;
+
+	cgrp = bpf_cgroup_from_id(cgrp_id);
+	if (!cgrp)
+		return 0;
+
+	ancestor = bpf_cgroup_ancestor(cgrp, 1);
+	if (!ancestor)
+		goto out;
+
+	read_xattr(cgrp);
+	bpf_cgroup_release(ancestor);
+out:
+	bpf_cgroup_release(cgrp);
+	return 0;
+}
-- 
2.47.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 bpf-next 4/5] selftests/bpf: Add tests for bpf_cgroup_read_xattr
  2025-06-19 22:01 ` [PATCH v2 bpf-next 4/5] selftests/bpf: Add tests for bpf_cgroup_read_xattr Song Liu
@ 2025-06-20 18:11   ` Alexei Starovoitov
  2025-06-20 18:36     ` Eduard Zingerman
  0 siblings, 1 reply; 14+ messages in thread
From: Alexei Starovoitov @ 2025-06-20 18:11 UTC (permalink / raw)
  To: Song Liu, Jose E. Marchesi, Jose E. Marchesi
  Cc: bpf, Linux-Fsdevel, LKML, LSM List, Kernel Team, Andrii Nakryiko,
	Eduard, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau,
	Alexander Viro, Christian Brauner, Jan Kara, KP Singh,
	Matt Bobrowski, Amir Goldstein, Greg Kroah-Hartman, Tejun Heo,
	Daan De Meyer

On Thu, Jun 19, 2025 at 3:02 PM Song Liu <song@kernel.org> wrote:
> +       bpf_dynptr_from_mem(xattr_value, sizeof(xattr_value), 0, &value_ptr);

https://github.com/kernel-patches/bpf/actions/runs/15767046528/job/44445539248

progs/cgroup_read_xattr.c:19:9: error: ‘bpf_dynptr_from_mem’ is static
but used in inline function ‘read_xattr’ which is not static [-Werror]
19 | bpf_dynptr_from_mem(value, sizeof(value), 0, &value_ptr);
| ^~~~~~~~~~~~~~~~~~~


Jose,

Could you please help us understand this gcc-bpf error ?
What does it mean?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 bpf-next 5/5] bpf: Make bpf_cgroup_read_xattr available to cgroup and struct_ops progs
  2025-06-19 22:01 ` [PATCH v2 bpf-next 5/5] bpf: Make bpf_cgroup_read_xattr available to cgroup and struct_ops progs Song Liu
@ 2025-06-20 18:18   ` Alexei Starovoitov
  2025-06-20 20:48     ` Song Liu
  0 siblings, 1 reply; 14+ messages in thread
From: Alexei Starovoitov @ 2025-06-20 18:18 UTC (permalink / raw)
  To: Song Liu
  Cc: bpf, Linux-Fsdevel, LKML, LSM List, Kernel Team, Andrii Nakryiko,
	Eduard, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau,
	Alexander Viro, Christian Brauner, Jan Kara, KP Singh,
	Matt Bobrowski, Amir Goldstein, Greg Kroah-Hartman, Tejun Heo,
	Daan De Meyer

On Thu, Jun 19, 2025 at 3:02 PM Song Liu <song@kernel.org> wrote:
>
> cgroup BPF programs and struct_ops BPF programs (such as sched_ext), need
> bpf_cgroup_read_xattr. Make bpf_cgroup_read_xattr available to these prog
> types.

...

> +       ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_LSM, &bpf_lsm_fs_kfunc_set);
> +       ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, &bpf_fs_kfunc_set);
> +       ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SKB, &bpf_fs_kfunc_set);
> +       ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCK, &bpf_fs_kfunc_set);
> +       ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_DEVICE, &bpf_fs_kfunc_set);
> +       ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCK_ADDR, &bpf_fs_kfunc_set);
> +       ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SYSCTL, &bpf_fs_kfunc_set);
> +       return ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCKOPT, &bpf_fs_kfunc_set);

No need to artificially restrict it like this.
bpf_cgroup_read_xattr() is generic enough and the verifier will enforce
the safety due to KF_RCU.
Just add it to common_btf_ids.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 bpf-next 4/5] selftests/bpf: Add tests for bpf_cgroup_read_xattr
  2025-06-20 18:11   ` Alexei Starovoitov
@ 2025-06-20 18:36     ` Eduard Zingerman
  2025-06-20 19:09       ` Jose E. Marchesi
  0 siblings, 1 reply; 14+ messages in thread
From: Eduard Zingerman @ 2025-06-20 18:36 UTC (permalink / raw)
  To: Alexei Starovoitov, Song Liu, Jose E. Marchesi, Jose E. Marchesi
  Cc: bpf, Linux-Fsdevel, LKML, LSM List, Kernel Team, Andrii Nakryiko,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau,
	Alexander Viro, Christian Brauner, Jan Kara, KP Singh,
	Matt Bobrowski, Amir Goldstein, Greg Kroah-Hartman, Tejun Heo,
	Daan De Meyer

On Fri, 2025-06-20 at 11:11 -0700, Alexei Starovoitov wrote:
> On Thu, Jun 19, 2025 at 3:02 PM Song Liu <song@kernel.org> wrote:
> > +       bpf_dynptr_from_mem(xattr_value, sizeof(xattr_value), 0, &value_ptr);
> 
> https://github.com/kernel-patches/bpf/actions/runs/15767046528/job/44445539248
> 
> progs/cgroup_read_xattr.c:19:9: error: ‘bpf_dynptr_from_mem’ is static
> but used in inline function ‘read_xattr’ which is not static [-Werror]
> 19 | bpf_dynptr_from_mem(value, sizeof(value), 0, &value_ptr);
> > ^~~~~~~~~~~~~~~~~~~
> 
> 
> Jose,
> 
> Could you please help us understand this gcc-bpf error ?
> What does it mean?

Not Jose, but was curious.
Some googling lead to the following C99 wording [1]:

  > An inline definition of a function with external linkage shall not
  > contain a definition of a modifiable object with static storage
  > duration, and shall not contain a reference to an identifier with
  > internal linkage

[1] https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf
    6.7.4 Function specifiers, paragraph 3

The helper is defined as `static`:

  static long (* const bpf_dynptr_from_mem)(...) = (void *) 197;

While `read_xattr` has external linkage:

  __always_inline void read_xattr(struct cgroup *cgroup)
  {
	...
	bpf_dynptr_from_mem(value, sizeof(value), 0, &value_ptr);
	...
  }

I think that declaring `read_xattr` as `static` should help with gcc.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 bpf-next 4/5] selftests/bpf: Add tests for bpf_cgroup_read_xattr
  2025-06-20 18:36     ` Eduard Zingerman
@ 2025-06-20 19:09       ` Jose E. Marchesi
  0 siblings, 0 replies; 14+ messages in thread
From: Jose E. Marchesi @ 2025-06-20 19:09 UTC (permalink / raw)
  To: Eduard Zingerman
  Cc: Alexei Starovoitov, Song Liu, Jose E. Marchesi, bpf,
	Linux-Fsdevel, LKML, LSM List, Kernel Team, Andrii Nakryiko,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau,
	Alexander Viro, Christian Brauner, Jan Kara, KP Singh,
	Matt Bobrowski, Amir Goldstein, Greg Kroah-Hartman, Tejun Heo,
	Daan De Meyer



> On Fri, 2025-06-20 at 11:11 -0700, Alexei Starovoitov wrote:
>> On Thu, Jun 19, 2025 at 3:02 PM Song Liu <song@kernel.org> wrote:
>> > +       bpf_dynptr_from_mem(xattr_value, sizeof(xattr_value), 0, &value_ptr);
>> 
>> https://github.com/kernel-patches/bpf/actions/runs/15767046528/job/44445539248
>> 
>> progs/cgroup_read_xattr.c:19:9: error: ‘bpf_dynptr_from_mem’ is static
>> but used in inline function ‘read_xattr’ which is not static [-Werror]
>> 19 | bpf_dynptr_from_mem(value, sizeof(value), 0, &value_ptr);
>> > ^~~~~~~~~~~~~~~~~~~
>> 
>> 
>> Jose,
>> 
>> Could you please help us understand this gcc-bpf error ?
>> What does it mean?
>
> Not Jose, but was curious.
> Some googling lead to the following C99 wording [1]:
>
>   > An inline definition of a function with external linkage shall not
>   > contain a definition of a modifiable object with static storage
>   > duration, and shall not contain a reference to an identifier with
>   > internal linkage
>
> [1] https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf
>     6.7.4 Function specifiers, paragraph 3
>
> The helper is defined as `static`:
>
>   static long (* const bpf_dynptr_from_mem)(...) = (void *) 197;
>
> While `read_xattr` has external linkage:
>
>   __always_inline void read_xattr(struct cgroup *cgroup)
>   {
> 	...
> 	bpf_dynptr_from_mem(value, sizeof(value), 0, &value_ptr);
> 	...
>   }
>
> I think that declaring `read_xattr` as `static` should help with gcc.

Yes I agree that is the issue.  It is a restriction introduced by C99.
I wasn't aware of it so I had to look it up.

If you need read_xattr to be accessible from other compilation units and
inlinable, you may declare it as static inline and put it in a header
file.  It will then use the copies of the helper pointers in the other
compilation units.

Alternatively, with GCC you could try to add a direct declaration for
the helper function to progs/cgroup_read_xattr.c, that doesnt need to
use any static intermediate pointer:

  #if COMPILING_WITH_GCC
  long _bpf_dynptr_from_mem (...) __attribute__ ((kernel_helper (197)));
  #define bpf_dynptr_from_mem _bpf_dynptr_from_mem
  #endif

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 bpf-next 5/5] bpf: Make bpf_cgroup_read_xattr available to cgroup and struct_ops progs
  2025-06-20 18:18   ` Alexei Starovoitov
@ 2025-06-20 20:48     ` Song Liu
  0 siblings, 0 replies; 14+ messages in thread
From: Song Liu @ 2025-06-20 20:48 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Song Liu, bpf, Linux-Fsdevel, LKML, LSM List, Kernel Team,
	Andrii Nakryiko, Eduard, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Alexander Viro, Christian Brauner, Jan Kara,
	KP Singh, Matt Bobrowski, Amir Goldstein, Greg Kroah-Hartman,
	Tejun Heo, Daan De Meyer



> On Jun 20, 2025, at 11:18 AM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> 
> On Thu, Jun 19, 2025 at 3:02 PM Song Liu <song@kernel.org> wrote:
>> 
>> cgroup BPF programs and struct_ops BPF programs (such as sched_ext), need
>> bpf_cgroup_read_xattr. Make bpf_cgroup_read_xattr available to these prog
>> types.
> 
> ...
> 
>> +       ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_LSM, &bpf_lsm_fs_kfunc_set);
>> +       ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, &bpf_fs_kfunc_set);
>> +       ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SKB, &bpf_fs_kfunc_set);
>> +       ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCK, &bpf_fs_kfunc_set);
>> +       ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_DEVICE, &bpf_fs_kfunc_set);
>> +       ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCK_ADDR, &bpf_fs_kfunc_set);
>> +       ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SYSCTL, &bpf_fs_kfunc_set);
>> +       return ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCKOPT, &bpf_fs_kfunc_set);
> 
> No need to artificially restrict it like this.
> bpf_cgroup_read_xattr() is generic enough and the verifier will enforce
> the safety due to KF_RCU.
> Just add it to common_btf_ids.

Makes sense. I will add it to common_btf_ids in v3. 

Thanks,
Song


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 bpf-next 2/5] bpf: Introduce bpf_cgroup_read_xattr to read xattr of cgroup's node
  2025-06-19 22:01 ` [PATCH v2 bpf-next 2/5] bpf: Introduce bpf_cgroup_read_xattr to read xattr of cgroup's node Song Liu
@ 2025-06-21  2:44   ` Tejun Heo
  2025-06-21  3:50     ` Song Liu
  0 siblings, 1 reply; 14+ messages in thread
From: Tejun Heo @ 2025-06-21  2:44 UTC (permalink / raw)
  To: Song Liu
  Cc: bpf, linux-fsdevel, linux-kernel, linux-security-module,
	kernel-team, andrii, eddyz87, ast, daniel, martin.lau, viro,
	brauner, jack, kpsingh, mattbobrowski, amir73il, gregkh,
	daan.j.demeyer

On Thu, Jun 19, 2025 at 03:01:11PM -0700, Song Liu wrote:
> BPF programs, such as LSM and sched_ext, would benefit from tags on
> cgroups. One common practice to apply such tags is to set xattrs on
> cgroupfs folders.
> 
> Introduce kfunc bpf_cgroup_read_xattr, which allows reading cgroup's
> xattr.
> 
> Note that, we already have bpf_get_[file|dentry]_xattr. However, these
> two APIs are not ideal for reading cgroupfs xattrs, because:
> 
>   1) These two APIs only works in sleepable contexts;
>   2) There is no kfunc that matches current cgroup to cgroupfs dentry.
> 
> Signed-off-by: Song Liu <song@kernel.org>
...
> +__bpf_kfunc int bpf_cgroup_read_xattr(struct cgroup *cgroup, const char *name__str,
> +					struct bpf_dynptr *value_p)
> +{
> +	struct bpf_dynptr_kern *value_ptr = (struct bpf_dynptr_kern *)value_p;
> +	u32 value_len;
> +	void *value;
> +
> +	/* Only allow reading "user.*" xattrs */
> +	if (strncmp(name__str, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN))
> +		return -EPERM;

Just out of curiosity, what security holes are there if we allow BPF
programs to read other xattrs? Given how priviledged BPF programs already
are, does this make meaningful difference?

From cgroup POV:

 Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 bpf-next 3/5] bpf: Mark cgroup_subsys_state->cgroup RCU safe
  2025-06-19 22:01 ` [PATCH v2 bpf-next 3/5] bpf: Mark cgroup_subsys_state->cgroup RCU safe Song Liu
@ 2025-06-21  2:45   ` Tejun Heo
  0 siblings, 0 replies; 14+ messages in thread
From: Tejun Heo @ 2025-06-21  2:45 UTC (permalink / raw)
  To: Song Liu
  Cc: bpf, linux-fsdevel, linux-kernel, linux-security-module,
	kernel-team, andrii, eddyz87, ast, daniel, martin.lau, viro,
	brauner, jack, kpsingh, mattbobrowski, amir73il, gregkh,
	daan.j.demeyer

On Thu, Jun 19, 2025 at 03:01:12PM -0700, Song Liu wrote:
> Mark struct cgroup_subsys_state->cgroup as safe under RCU read lock. This
> will enable accessing css->cgroup from a bpf css iterator.
> 
> Signed-off-by: Song Liu <song@kernel.org>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 bpf-next 2/5] bpf: Introduce bpf_cgroup_read_xattr to read xattr of cgroup's node
  2025-06-21  2:44   ` Tejun Heo
@ 2025-06-21  3:50     ` Song Liu
  0 siblings, 0 replies; 14+ messages in thread
From: Song Liu @ 2025-06-21  3:50 UTC (permalink / raw)
  To: Tejun Heo
  Cc: Song Liu, bpf@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-security-module@vger.kernel.org, Kernel Team,
	andrii@kernel.org, eddyz87@gmail.com, ast@kernel.org,
	daniel@iogearbox.net, martin.lau@linux.dev,
	viro@zeniv.linux.org.uk, brauner@kernel.org, jack@suse.cz,
	kpsingh@kernel.org, mattbobrowski@google.com, amir73il@gmail.com,
	gregkh@linuxfoundation.org, daan.j.demeyer@gmail.com



> On Jun 20, 2025, at 7:44 PM, Tejun Heo <tj@kernel.org> wrote:
> 
> On Thu, Jun 19, 2025 at 03:01:11PM -0700, Song Liu wrote:
>> BPF programs, such as LSM and sched_ext, would benefit from tags on
>> cgroups. One common practice to apply such tags is to set xattrs on
>> cgroupfs folders.
>> 
>> Introduce kfunc bpf_cgroup_read_xattr, which allows reading cgroup's
>> xattr.
>> 
>> Note that, we already have bpf_get_[file|dentry]_xattr. However, these
>> two APIs are not ideal for reading cgroupfs xattrs, because:
>> 
>>  1) These two APIs only works in sleepable contexts;
>>  2) There is no kfunc that matches current cgroup to cgroupfs dentry.
>> 
>> Signed-off-by: Song Liu <song@kernel.org>
> ...
>> +__bpf_kfunc int bpf_cgroup_read_xattr(struct cgroup *cgroup, const char *name__str,
>> + struct bpf_dynptr *value_p)
>> +{
>> + struct bpf_dynptr_kern *value_ptr = (struct bpf_dynptr_kern *)value_p;
>> + u32 value_len;
>> + void *value;
>> +
>> + /* Only allow reading "user.*" xattrs */
>> + if (strncmp(name__str, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN))
>> + return -EPERM;
> 
> Just out of curiosity, what security holes are there if we allow BPF
> programs to read other xattrs? Given how priviledged BPF programs already
> are, does this make meaningful difference?

There are some xatters that we shouldn’t read, for example, other 
security.* xattrs (security.selinux etc.). 

We can probably allow BPF LSM programs to read security.bpf.* xattrs, 
on cgroup nodes, just like bpf_get_[file|dentry]_xattr. But that 
requires some extra logic. 

Thanks,
Song


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2025-06-21  3:50 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-19 22:01 [PATCH v2 bpf-next 0/5] Introduce bpf_cgroup_read_xattr Song Liu
2025-06-19 22:01 ` [PATCH v2 bpf-next 1/5] kernfs: remove iattr_mutex Song Liu
2025-06-19 22:01 ` [PATCH v2 bpf-next 2/5] bpf: Introduce bpf_cgroup_read_xattr to read xattr of cgroup's node Song Liu
2025-06-21  2:44   ` Tejun Heo
2025-06-21  3:50     ` Song Liu
2025-06-19 22:01 ` [PATCH v2 bpf-next 3/5] bpf: Mark cgroup_subsys_state->cgroup RCU safe Song Liu
2025-06-21  2:45   ` Tejun Heo
2025-06-19 22:01 ` [PATCH v2 bpf-next 4/5] selftests/bpf: Add tests for bpf_cgroup_read_xattr Song Liu
2025-06-20 18:11   ` Alexei Starovoitov
2025-06-20 18:36     ` Eduard Zingerman
2025-06-20 19:09       ` Jose E. Marchesi
2025-06-19 22:01 ` [PATCH v2 bpf-next 5/5] bpf: Make bpf_cgroup_read_xattr available to cgroup and struct_ops progs Song Liu
2025-06-20 18:18   ` Alexei Starovoitov
2025-06-20 20:48     ` Song Liu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).