Merge branch 'bpf-fix-setting-retval-to-eperm-for-cgroup-hooks-not-returning-errno'

Xu Kuohai says:

====================
bpf: Fix setting retval to -EPERM for cgroup hooks not returning errno

This series fixes the issue reported by sashiko in [1]. The issue is that,
when a cgroup BPF program exits with 0, bpf_prog_run_array_cg() sets
the hook return value to -EPERM if it is not a valid errno. This is
correct for errno-based hooks, which return 0 on success and negative
errno on failure, but wrong for void and boolean LSM hooks. Boolean
LSM hooks should only return true or false, and void LSM hooks have
no return value at all.

Fix it by skipping setting -EPERM for hooks not returning errno.

[1] https://lore.kernel.org/bpf/20260605144232.95A141F00893@smtp.kernel.org/
====================

Link: https://patch.msgid.link/20260610201724.733943-1-xukuohai@huaweicloud.com
Signed-off-by: Alexei Starovoitov <ast@kernel.org>

include/linux/bpf_lsm.h

@@ -52,6 +52,7 @@ int bpf_set_dentry_xattr_locked(struct dentry *dentry, const char *name__str,
 				const struct bpf_dynptr *value_p, int flags);
 int bpf_remove_dentry_xattr_locked(struct dentry *dentry, const char *name__str);
 bool bpf_lsm_has_d_inode_locked(const struct bpf_prog *prog);
+bool bpf_lsm_hook_returns_errno(u32 btf_id);
 
 #else /* !CONFIG_BPF_LSM */
 
@@ -104,6 +105,11 @@ static inline bool bpf_lsm_has_d_inode_locked(const struct bpf_prog *prog)
 {
 	return false;
 }
+
+static inline bool bpf_lsm_hook_returns_errno(u32 btf_id)
+{
+	return true;
+}
 #endif /* CONFIG_BPF_LSM */
 
 #endif /* _LINUX_BPF_LSM_H */

kernel/bpf/bpf_lsm.c

@@ -427,6 +427,26 @@ BTF_ID(func, bpf_lsm_audit_rule_known)
 BTF_ID(func, bpf_lsm_inode_xattr_skipcap)
 BTF_SET_END(bool_lsm_hooks)
 
+/* hooks returning void */
+#define LSM_HOOK_void(DEFAULT, NAME, ...) BTF_ID(func, bpf_lsm_##NAME)
+#define LSM_HOOK_int(DEFAULT, NAME, ...)  /* nothing */
+#define LSM_HOOK(RET, DEFAULT, NAME, ...) LSM_HOOK_##RET(DEFAULT, NAME, __VA_ARGS__)
+BTF_SET_START(void_lsm_hooks)
+#include <linux/lsm_hook_defs.h>
+#undef LSM_HOOK
+#undef LSM_HOOK_void
+#undef LSM_HOOK_int
+BTF_SET_END(void_lsm_hooks)
+
+bool bpf_lsm_hook_returns_errno(u32 btf_id)
+{
+	if (btf_id_set_contains(&bool_lsm_hooks, btf_id))
+		return false;
+	if (btf_id_set_contains(&void_lsm_hooks, btf_id))
+		return false;
+	return true;
+}
+
 int bpf_lsm_get_retval_range(const struct bpf_prog *prog,
 			     struct bpf_retval_range *retval_range)
 {

kernel/bpf/cgroup.c

@@ -55,6 +55,28 @@ void __init cgroup_bpf_lifetime_notifier_init(void)
 						&cgroup_bpf_lifetime_nb));
 }
 
+#ifdef CONFIG_BPF_LSM
+struct cgroup_lsm_atype {
+	u32 attach_btf_id;
+	int refcnt;
+	bool returns_errno;
+};
+
+static struct cgroup_lsm_atype cgroup_lsm_atype[CGROUP_LSM_NUM];
+
+static bool cgroup_bpf_hook_returns_errno(enum cgroup_bpf_attach_type atype)
+{
+	if (atype >= CGROUP_LSM_START && atype <= CGROUP_LSM_END)
+		return READ_ONCE(cgroup_lsm_atype[atype - CGROUP_LSM_START].returns_errno);
+	return true;
+}
+#else
+static bool cgroup_bpf_hook_returns_errno(enum cgroup_bpf_attach_type atype)
+{
+	return true;
+}
+#endif
+
 /* __always_inline is necessary to prevent indirect call through run_prog
  * function pointer.
  */
@@ -83,7 +105,8 @@ bpf_prog_run_array_cg(const struct cgroup_bpf *cgrp,
 			*(ret_flags) |= (func_ret >> 1);
 			func_ret &= 1;
 		}
-		if (!func_ret && !IS_ERR_VALUE((long)run_ctx.retval))
+		if (!func_ret && cgroup_bpf_hook_returns_errno(atype) &&
+		    !IS_ERR_VALUE((long)run_ctx.retval))
 			run_ctx.retval = -EPERM;
 		item++;
 	}
@@ -156,13 +179,6 @@ unsigned int __cgroup_bpf_run_lsm_current(const void *ctx,
 }
 
 #ifdef CONFIG_BPF_LSM
-struct cgroup_lsm_atype {
-	u32 attach_btf_id;
-	int refcnt;
-};
-
-static struct cgroup_lsm_atype cgroup_lsm_atype[CGROUP_LSM_NUM];
-
 static enum cgroup_bpf_attach_type
 bpf_cgroup_atype_find(enum bpf_attach_type attach_type, u32 attach_btf_id)
 {
@@ -191,10 +207,13 @@ void bpf_cgroup_atype_get(u32 attach_btf_id, int cgroup_atype)
 
 	lockdep_assert_held(&cgroup_mutex);
 
-	WARN_ON_ONCE(cgroup_lsm_atype[i].attach_btf_id &&
-		     cgroup_lsm_atype[i].attach_btf_id != attach_btf_id);
-
-	cgroup_lsm_atype[i].attach_btf_id = attach_btf_id;
+	if (!cgroup_lsm_atype[i].attach_btf_id) {
+		cgroup_lsm_atype[i].attach_btf_id = attach_btf_id;
+		WRITE_ONCE(cgroup_lsm_atype[i].returns_errno,
+			   bpf_lsm_hook_returns_errno(attach_btf_id));
+	} else {
+		WARN_ON_ONCE(cgroup_lsm_atype[i].attach_btf_id != attach_btf_id);
+	}
 	cgroup_lsm_atype[i].refcnt++;
 }
 
@@ -203,8 +222,10 @@ void bpf_cgroup_atype_put(int cgroup_atype)
 	int i = cgroup_atype - CGROUP_LSM_START;
 
 	cgroup_lock();
-	if (--cgroup_lsm_atype[i].refcnt <= 0)
+	if (--cgroup_lsm_atype[i].refcnt <= 0) {
+		WRITE_ONCE(cgroup_lsm_atype[i].returns_errno, true);
 		cgroup_lsm_atype[i].attach_btf_id = 0;
+	}
 	WARN_ON_ONCE(cgroup_lsm_atype[i].refcnt < 0);
 	cgroup_unlock();
 }

tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c

@@ -2,6 +2,7 @@
 
 #include <sys/types.h>
 #include <sys/socket.h>
+#include <sys/xattr.h>
 #include <test_progs.h>
 #include <bpf/btf.h>
 
@@ -309,11 +310,89 @@ static void test_lsm_cgroup_nonvoid(void)
 	lsm_cgroup_nonvoid__destroy(skel);
 }
 
+static void test_lsm_cgroup_retval(void)
+{
+	struct lsm_cgroup *skel = NULL;
+	int skipcap_prog_fd1, skipcap_prog_fd2, socket_prog_fd1, socket_prog_fd2;
+	int cgroup_fd = -1;
+	int err, fd;
+	char tmpfile[] = "/tmp/test_lsm_cgroup_retval.XXXXXX";
+
+	fd = mkstemp(tmpfile);
+	if (!ASSERT_OK_FD(fd, "mkstemp"))
+		return;
+	close(fd);
+
+	cgroup_fd = test__join_cgroup("/default_retval");
+	if (!ASSERT_OK_FD(cgroup_fd, "join_cgroup"))
+		goto cleanup_tmpfile;
+
+	skel = lsm_cgroup__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "open_and_load"))
+		goto cleanup_cgroup;
+
+	skipcap_prog_fd1 = bpf_program__fd(skel->progs.skipcap_first);
+	skipcap_prog_fd2 = bpf_program__fd(skel->progs.skipcap_second);
+	socket_prog_fd1 = bpf_program__fd(skel->progs.socket_first);
+	socket_prog_fd2 = bpf_program__fd(skel->progs.socket_second);
+
+	err = bpf_prog_attach(skipcap_prog_fd1, cgroup_fd, BPF_LSM_CGROUP, BPF_F_ALLOW_MULTI);
+	if (err == -ENOTSUPP) {
+		test__skip();
+		goto cleanup_skeleton;
+	}
+	if (!ASSERT_OK(err, "attach first skipcap prog"))
+		goto cleanup_skeleton;
+
+	err = bpf_prog_attach(skipcap_prog_fd2, cgroup_fd, BPF_LSM_CGROUP, BPF_F_ALLOW_MULTI);
+	if (!ASSERT_OK(err, "attach second skipcap prog"))
+		goto cleanup_skipcap1;
+
+	err = bpf_prog_attach(socket_prog_fd1, cgroup_fd, BPF_LSM_CGROUP, BPF_F_ALLOW_MULTI);
+	if (!ASSERT_OK(err, "attach first sock_create prog"))
+		goto cleanup_skipcap2;
+
+	err = bpf_prog_attach(socket_prog_fd2, cgroup_fd, BPF_LSM_CGROUP, BPF_F_ALLOW_MULTI);
+	if (!ASSERT_OK(err, "attach second sock_create prog"))
+		goto cleanup_sock_create1;
+
+	/* trigger the bool hook by setxattr */
+	err = setxattr(tmpfile, "user.test", "value", 5, 0);
+	if (!ASSERT_OK(err, "setxattr"))
+		goto cleanup_sock_create2;
+
+	/* trigger the errno hook by creating a socket */
+	fd = socket(AF_INET, SOCK_STREAM, 0);
+	if (!ASSERT_OK_FD(fd, "socket"))
+		goto cleanup_sock_create2;
+	close(fd);
+
+	ASSERT_EQ(skel->data->skipcap_retval, 0, "bool_hook_retval_should_be_0");
+	ASSERT_EQ(skel->data->socket_retval, -EPERM, "errno_hook_retval_should_be_EPERM");
+
+cleanup_sock_create2:
+	bpf_prog_detach2(socket_prog_fd2, cgroup_fd, BPF_LSM_CGROUP);
+cleanup_sock_create1:
+	bpf_prog_detach2(socket_prog_fd1, cgroup_fd, BPF_LSM_CGROUP);
+cleanup_skipcap2:
+	bpf_prog_detach2(skipcap_prog_fd2, cgroup_fd, BPF_LSM_CGROUP);
+cleanup_skipcap1:
+	bpf_prog_detach2(skipcap_prog_fd1, cgroup_fd, BPF_LSM_CGROUP);
+cleanup_skeleton:
+	lsm_cgroup__destroy(skel);
+cleanup_cgroup:
+	close(cgroup_fd);
+cleanup_tmpfile:
+	unlink(tmpfile);
+}
+
 void test_lsm_cgroup(void)
 {
 	if (test__start_subtest("functional"))
 		test_lsm_cgroup_functional();
 	if (test__start_subtest("nonvoid"))
 		test_lsm_cgroup_nonvoid();
+	if (test__start_subtest("retval"))
+		test_lsm_cgroup_retval();
 	btf__free(btf);
 }

tools/testing/selftests/bpf/progs/lsm_cgroup.c

@@ -35,6 +35,8 @@ int called_socket_bind;
 int called_socket_bind2;
 int called_socket_alloc;
 int called_socket_clone;
+int skipcap_retval = -4095;
+int socket_retval = -4095;
 
 static __always_inline int test_local_storage(void)
 {
@@ -190,3 +192,31 @@ int BPF_PROG(socket_clone, struct sock *newsk, const struct request_sock *req)
 
 	return 1;
 }
+
+SEC("lsm_cgroup/inode_xattr_skipcap")
+int BPF_PROG(skipcap_first, const char *name)
+{
+	return 0;
+}
+
+SEC("lsm_cgroup/inode_xattr_skipcap")
+int BPF_PROG(skipcap_second, const char *name)
+{
+	skipcap_retval = bpf_get_retval();
+	bpf_set_retval(0);
+	return 1;
+}
+
+SEC("lsm_cgroup/socket_create")
+int BPF_PROG(socket_first, int family, int type, int protocol, int kern)
+{
+	return 0;
+}
+
+SEC("lsm_cgroup/socket_create")
+int BPF_PROG(socket_second, int family, int type, int protocol, int kern)
+{
+	socket_retval = bpf_get_retval();
+	bpf_set_retval(0);
+	return 1;
+}