bpf: expose bpf_{g,s}et_retval to more cgroup hooks

include/linux/bpf.h +6 -0

index a627a02cf8ab..c302d2de073a 100644
@@ -1948,6 +1948,10 @@ struct bpf_prog *bpf_prog_by_id(u32 id);
 struct bpf_link *bpf_link_by_id(u32 id);
 
 const struct bpf_func_proto *bpf_base_func_proto(enum bpf_func_id func_id);
+const struct bpf_func_proto *
+cgroup_common_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog);
+const struct bpf_func_proto *
+cgroup_current_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog);
 void bpf_task_storage_free(struct task_struct *task);
 bool bpf_prog_has_kfunc_call(const struct bpf_prog *prog);
 const struct btf_func_model *

@@ -2154,6 +2158,18 @@ bpf_base_func_proto(enum bpf_func_id func_id)
 	return NULL;
 }
 
+static inline const struct bpf_func_proto *
+cgroup_common_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)

include/linux/bpf.h +0 -109

-#define BPF_STRTOX_BASE_MASK 0x1F
-
-static int __bpf_strtoull(const char *buf, size_t buf_len, u64 flags,
-			  unsigned long long *res, bool *is_negative)
-{
-	unsigned int base = flags & BPF_STRTOX_BASE_MASK;
-	const char *cur_buf = buf;
-	size_t cur_len = buf_len;
-	unsigned int consumed;
-	size_t val_len;
-	char str[64];
-
-	if (!buf || !buf_len || !res || !is_negative)
-		return -EINVAL;
-
-	if (base != 0 && base != 8 && base != 10 && base != 16)
-		return -EINVAL;
-
-	if (flags & ~BPF_STRTOX_BASE_MASK)
-		return -EINVAL;
-
-	while (cur_buf < buf + buf_len && isspace(*cur_buf))
-		++cur_buf;
-
-	*is_negative = (cur_buf < buf + buf_len && *cur_buf == '-');
-	if (*is_negative)
-		++cur_buf;
-
-	consumed = cur_buf - buf;
-	cur_len -= consumed;
-	if (!cur_len)
-		return -EINVAL;
-
-	cur_len = min(cur_len, sizeof(str) - 1);
-	memcpy(str, cur_buf, cur_len);
-	str[cur_len] = '\0';
-	cur_buf = str;
-
-	cur_buf = _parse_integer_fixup_radix(cur_buf, &base);
-	val_len = _parse_integer(cur_buf, base, res);
-
-	if (val_len & KSTRTOX_OVERFLOW)
-		return -ERANGE;
-
-	if (val_len == 0)
-		return -EINVAL;
-
-	cur_buf += val_len;
-	consumed += cur_buf - str;
-
-	return consumed;
-}
-
-static int __bpf_strtoll(const char *buf, size_t buf_len, u64 flags,
-			 long long *res)
-{
-	unsigned long long _res;
-	bool is_negative;
-	int err;
-
-	err = __bpf_strtoull(buf, buf_len, flags, &_res, &is_negative);
-	if (err < 0)
-		return err;
-	if (is_negative) {
-		if ((long long)-_res > 0)
-			return -ERANGE;
-		*res = -_res;
-	} else {
-		if ((long long)_res < 0)
-			return -ERANGE;
-		*res = _res;
-	}
-	return err;
-}
-
-BPF_CALL_4(bpf_strtol, const char *, buf, size_t, buf_len, u64, flags,
-	   long *, res)
-{
-	long long _res;
-	int err;
-
-	err = __bpf_strtoll(buf, buf_len, flags, &_res);
-	if (err < 0)
-		return err;
-	if (_res != (long)_res)
-		return -ERANGE;
-	*res = _res;
-	return err;
-}
-
-const struct bpf_func_proto bpf_strtol_proto = {
-	.func		= bpf_strtol,
-	.gpl_only	= false,
-	.ret_type	= RET_INTEGER,
-	.arg1_type	= ARG_PTR_TO_MEM | MEM_RDONLY,
-	.arg2_type	= ARG_CONST_SIZE,
-	.arg3_type	= ARG_ANYTHING,
-	.arg4_type	= ARG_PTR_TO_LONG,
-};
-
-BPF_CALL_4(bpf_strtoul, const char *, buf, size_t, buf_len, u64, flags,
-	   unsigned long *, res)
-{
-	unsigned long long _res;
-	bool is_negative;
-	int err;
-
-	err = __bpf_strtoull(buf, buf_len, flags, &_res, &is_negative);
-	if (err < 0)

include/linux/bpf.h +0 -9

-	.func		= bpf_strtoul,
-	.gpl_only	= false,
-	.ret_type	= RET_INTEGER,
-	.arg1_type	= ARG_PTR_TO_MEM | MEM_RDONLY,
-	.arg2_type	= ARG_CONST_SIZE,
-	.arg3_type	= ARG_ANYTHING,
-	.arg4_type	= ARG_PTR_TO_LONG,
-};
-#endif

Quoted 19 lines · Stanislav Fomichev

include/linux/bpf.h +4 -0

index a627a02cf8ab..c302d2de073a 100644
@@ -1948,6 +1948,10 @@ struct bpf_prog *bpf_prog_by_id(u32 id);
 struct bpf_link *bpf_link_by_id(u32 id);
 
 const struct bpf_func_proto *bpf_base_func_proto(enum bpf_func_id func_id);
+const struct bpf_func_proto *
+cgroup_common_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog);
+const struct bpf_func_proto *
+cgroup_current_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog);
 void bpf_task_storage_free(struct task_struct *task);
 bool bpf_prog_has_kfunc_call(const struct bpf_prog *prog);
 const struct btf_func_model *

@@ -2154,6 +2158,18 @@ bpf_base_func_proto(enum bpf_func_id func_id)
      return NULL;
 }
 

include/linux/bpf.h bpf_base_func_proto(enum bpf_func_id func_id) · line 2172

Quoted 12 lines

include/linux/bpf.h +12 -0

+static inline const struct bpf_func_proto *
+cgroup_common_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+{
+     return NULL;
+}
+
+static inline const struct bpf_func_proto *
+cgroup_current_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+{
+     return NULL;
+}
+

There two new functions are implemented in cgroup.c which only compiles with CONFIG_CGROUP_BPF. I think the change in bpf.h here should be done in bpf-cgroup.h instead. Otherwise, the changes in filter.c in the next patch will have issue in resolving these functions when CONFIG_CGROUP_BPF is not set.

Quoted 115 lines

include/linux/bpf.h +0 -115

-#define BPF_STRTOX_BASE_MASK 0x1F
-
-static int __bpf_strtoull(const char *buf, size_t buf_len, u64 flags,
-                       unsigned long long *res, bool *is_negative)
-{
-     unsigned int base = flags & BPF_STRTOX_BASE_MASK;
-     const char *cur_buf = buf;
-     size_t cur_len = buf_len;
-     unsigned int consumed;
-     size_t val_len;
-     char str[64];
-
-     if (!buf || !buf_len || !res || !is_negative)
-             return -EINVAL;
-
-     if (base != 0 && base != 8 && base != 10 && base != 16)
-             return -EINVAL;
-
-     if (flags & ~BPF_STRTOX_BASE_MASK)
-             return -EINVAL;
-
-     while (cur_buf < buf + buf_len && isspace(*cur_buf))
-             ++cur_buf;
-
-     *is_negative = (cur_buf < buf + buf_len && *cur_buf == '-');
-     if (*is_negative)
-             ++cur_buf;
-
-     consumed = cur_buf - buf;
-     cur_len -= consumed;
-     if (!cur_len)
-             return -EINVAL;
-
-     cur_len = min(cur_len, sizeof(str) - 1);
-     memcpy(str, cur_buf, cur_len);
-     str[cur_len] = '\0';
-     cur_buf = str;
-
-     cur_buf = _parse_integer_fixup_radix(cur_buf, &base);
-     val_len = _parse_integer(cur_buf, base, res);
-
-     if (val_len & KSTRTOX_OVERFLOW)
-             return -ERANGE;
-
-     if (val_len == 0)
-             return -EINVAL;
-
-     cur_buf += val_len;
-     consumed += cur_buf - str;
-
-     return consumed;
-}
-
-static int __bpf_strtoll(const char *buf, size_t buf_len, u64 flags,
-                      long long *res)
-{
-     unsigned long long _res;
-     bool is_negative;
-     int err;
-
-     err = __bpf_strtoull(buf, buf_len, flags, &_res, &is_negative);
-     if (err < 0)
-             return err;
-     if (is_negative) {
-             if ((long long)-_res > 0)
-                     return -ERANGE;
-             *res = -_res;
-     } else {
-             if ((long long)_res < 0)
-                     return -ERANGE;
-             *res = _res;
-     }
-     return err;
-}
-
-BPF_CALL_4(bpf_strtol, const char *, buf, size_t, buf_len, u64, flags,
-        long *, res)
-{
-     long long _res;
-     int err;
-
-     err = __bpf_strtoll(buf, buf_len, flags, &_res);
-     if (err < 0)
-             return err;
-     if (_res != (long)_res)
-             return -ERANGE;
-     *res = _res;
-     return err;
-}
-
-const struct bpf_func_proto bpf_strtol_proto = {
-     .func           = bpf_strtol,
-     .gpl_only       = false,
-     .ret_type       = RET_INTEGER,
-     .arg1_type      = ARG_PTR_TO_MEM | MEM_RDONLY,
-     .arg2_type      = ARG_CONST_SIZE,
-     .arg3_type      = ARG_ANYTHING,
-     .arg4_type      = ARG_PTR_TO_LONG,
-};
-
-BPF_CALL_4(bpf_strtoul, const char *, buf, size_t, buf_len, u64, flags,
-        unsigned long *, res)
-{
-     unsigned long long _res;
-     bool is_negative;
-     int err;
-
-     err = __bpf_strtoull(buf, buf_len, flags, &_res, &is_negative);
-     if (err < 0)
-             return err;
-     if (is_negative)
-             return -EINVAL;
-     if (_res != (unsigned long)_res)
-             return -ERANGE;
-     *res = _res;

include/linux/bpf.h

Quoted 4 lines

include/linux/bpf.h +0 -4

-     return err;
-}
-
-const struct bpf_func_proto bpf_strtoul_proto = {

This should be useful in general other than cgroup bpf. It may end up moving back to helpers.c soon.
How about take this chance to add it to bpf_base_func_proto() which already has another string helper bpf_strncmp_proto?

Quoted 9 lines

include/linux/bpf.h +0 -9

-     .func           = bpf_strtoul,
-     .gpl_only       = false,
-     .ret_type       = RET_INTEGER,
-     .arg1_type      = ARG_PTR_TO_MEM | MEM_RDONLY,
-     .arg2_type      = ARG_CONST_SIZE,
-     .arg3_type      = ARG_ANYTHING,
-     .arg4_type      = ARG_PTR_TO_LONG,
-};
-#endif

Apparently, only a small subset of cgroup hooks actually falls back to cgroup_base_func_proto. This leads to unexpected result where not all cgroup helpers have bpf_{g,s}et_retval.

It's getting harder and harder to manage which helpers are exported to which hooks. We now have the following call chains:

- cg_skb_func_proto
- sk_filter_func_proto
- bpf_sk_base_func_proto
- bpf_base_func_proto

Quoted 7 lines · Stanislav Fomichev

Apparently, only a small subset of cgroup hooks actually falls back to cgroup_base_func_proto. This leads to unexpected result where not all cgroup helpers have bpf_{g,s}et_retval.

It's getting harder and harder to manage which helpers are exported to which hooks. We now have the following call chains:

Quoted 4 lines

- cg_skb_func_proto
- sk_filter_func_proto
- bpf_sk_base_func_proto
- bpf_base_func_proto

Could you explain how bpf_set_retval() will work with cgroup prog that is not syscall and can return flags in the higher bit (e.g. cg_skb egress). It will be a useful doc to add to the uapi bpf.h for
the bpf_set_retval() helper.

Quoted 13 lines · Martin KaFai Lau

Quoted 11 lines · Stanislav Fomichev

Apparently, only a small subset of cgroup hooks actually falls back to cgroup_base_func_proto. This leads to unexpected result where not all cgroup helpers have bpf_{g,s}et_retval.

It's getting harder and harder to manage which helpers are exported to which hooks. We now have the following call chains:

- cg_skb_func_proto
- sk_filter_func_proto
- bpf_sk_base_func_proto
- bpf_base_func_proto

Quoted 4 lines

Could you explain how bpf_set_retval() will work with cgroup prog that is not syscall and can return flags in the higher bit (e.g. cg_skb egress). It will be a useful doc to add to the uapi bpf.h for
the bpf_set_retval() helper.

I think it's the same case as the case without bpf_set_retval? I don't think the flags can be exported via bpf_set_retval, it just lets the users override EPERM.

Let me verify and I can add a note to bpf_set_retval uapi definition to mention that it just overrides EPERM. bpf_set_retval should probably not talk about userspace/syscall and instead use the words like "caller".

Quoted 14 lines · Stanislav Fomichev

Quoted 13 lines · Martin KaFai Lau

Quoted 11 lines · Stanislav Fomichev

Apparently, only a small subset of cgroup hooks actually falls back to cgroup_base_func_proto. This leads to unexpected result where not all cgroup helpers have bpf_{g,s}et_retval.

It's getting harder and harder to manage which helpers are exported to which hooks. We now have the following call chains:

- cg_skb_func_proto
- sk_filter_func_proto
- bpf_sk_base_func_proto
- bpf_base_func_proto

Quoted 8 lines

Quoted 4 lines

Could you explain how bpf_set_retval() will work with cgroup prog that is not syscall and can return flags in the higher bit (e.g. cg_skb egress). It will be a useful doc to add to the uapi bpf.h for
the bpf_set_retval() helper.

I think it's the same case as the case without bpf_set_retval? I don't think the flags can be exported via bpf_set_retval, it just lets the users override EPERM.

eg. Before, a cg_skb@egress prog returns 3 to mean NET_XMIT_CN. What if the prog now returns 3 and also bpf_set_retval(-Exxxx). If I read how __cgroup_bpf_run_filter_skb() uses bpf_prog_run_array_cg() correctly, __cgroup_bpf_run_filter_skb() will return NET_XMIT_DROP instead of the -Exxxx. The -Exxxx is probably what the bpf prog is expecting after calling bpf_set_retval(-Exxxx) ?
Thinking more about it, should __cgroup_bpf_run_filter_skb() always return -Exxxx whenever a -ve retval is set in bpf_set_retval() ?

Quoted 4 lines

Let me verify and I can add a note to bpf_set_retval uapi definition to mention that it just overrides EPERM. bpf_set_retval should probably not talk about userspace/syscall and instead use the words like "caller".

yeah, it is no longer syscall return value only now.

Quoted 24 lines · Martin KaFai Lau

Quoted 14 lines · Stanislav Fomichev

Quoted 13 lines · Martin KaFai Lau

Quoted 11 lines · Stanislav Fomichev

Apparently, only a small subset of cgroup hooks actually falls back to cgroup_base_func_proto. This leads to unexpected result where not all cgroup helpers have bpf_{g,s}et_retval.

It's getting harder and harder to manage which helpers are exported to which hooks. We now have the following call chains:

- cg_skb_func_proto
- sk_filter_func_proto
- bpf_sk_base_func_proto
- bpf_base_func_proto

Quoted 8 lines

Quoted 4 lines

Could you explain how bpf_set_retval() will work with cgroup prog that is not syscall and can return flags in the higher bit (e.g. cg_skb egress). It will be a useful doc to add to the uapi bpf.h for
the bpf_set_retval() helper.

I think it's the same case as the case without bpf_set_retval? I don't think the flags can be exported via bpf_set_retval, it just lets the users override EPERM.

Quoted 8 lines

eg. Before, a cg_skb@egress prog returns 3 to mean NET_XMIT_CN. What if the prog now returns 3 and also bpf_set_retval(-Exxxx). If I read how __cgroup_bpf_run_filter_skb() uses bpf_prog_run_array_cg() correctly, __cgroup_bpf_run_filter_skb() will return NET_XMIT_DROP instead of the -Exxxx. The -Exxxx is probably what the bpf prog is expecting after calling bpf_set_retval(-Exxxx) ?
Thinking more about it, should __cgroup_bpf_run_filter_skb() always return -Exxxx whenever a -ve retval is set in bpf_set_retval() ?

I think we used to have "return 0/1/2/3" to indicate different conditions but then switched to "return 1/0" + flags.

So, technically, "return 3 + bpf_set_retval" is still fundamentally a "return 3" api-wise.

I guess we can make bpf_set_retval override that but let me start by trying to document what we currently have.

If it turns out to be super ugly, we can try to fix it. (not sure how much of a uapi that is)

Quoted 8 lines

Quoted 7 lines

Let me verify and I can add a note to bpf_set_retval uapi definition to mention that it just overrides EPERM. bpf_set_retval should probably not talk about userspace/syscall and instead use the words like "caller".

yeah, it is no longer syscall return value only now.

Quoted 34 lines · Stanislav Fomichev

Quoted 32 lines · Martin KaFai Lau

Quoted 14 lines · Stanislav Fomichev

Quoted 13 lines · Martin KaFai Lau

Quoted 11 lines · Stanislav Fomichev

Apparently, only a small subset of cgroup hooks actually falls back to cgroup_base_func_proto. This leads to unexpected result where not all cgroup helpers have bpf_{g,s}et_retval.

It's getting harder and harder to manage which helpers are exported to which hooks. We now have the following call chains:

- cg_skb_func_proto
- sk_filter_func_proto
- bpf_sk_base_func_proto
- bpf_base_func_proto

Quoted 8 lines

Quoted 4 lines

Could you explain how bpf_set_retval() will work with cgroup prog that is not syscall and can return flags in the higher bit (e.g. cg_skb egress). It will be a useful doc to add to the uapi bpf.h for
the bpf_set_retval() helper.

I think it's the same case as the case without bpf_set_retval? I don't think the flags can be exported via bpf_set_retval, it just lets the users override EPERM.

eg. Before, a cg_skb@egress prog returns 3 to mean NET_XMIT_CN. What if the prog now returns 3 and also bpf_set_retval(-Exxxx). If I read how __cgroup_bpf_run_filter_skb() uses bpf_prog_run_array_cg() correctly, __cgroup_bpf_run_filter_skb() will return NET_XMIT_DROP instead of the -Exxxx. The -Exxxx is probably what the bpf prog is expecting after calling bpf_set_retval(-Exxxx) ?
Thinking more about it, should __cgroup_bpf_run_filter_skb() always return -Exxxx whenever a -ve retval is set in bpf_set_retval() ?

Quoted 2 lines

I think we used to have "return 0/1/2/3" to indicate different conditions but then switched to "return 1/0" + flags.

For 'int bpf_prog_run_array_cg(..., u32 *ret_flags)'? I think it is more like return "0 (OK)/-Exxxx" + ret_flags now.

Quoted 2 lines

So, technically, "return 3 + bpf_set_retval" is still fundamentally a "return 3" api-wise.

hm....for the exisiting usecase (eg. CGROUP_SETSOCKOPT), what does "bpf-prog-return 1 + bpf_set_retval(-EPERM)" mean?

Quoted 2 lines

I guess we can make bpf_set_retval override that but let me start by trying to document what we currently have.

To be clear, for cg_skb case, I meant to clear the ret_flags only if run_ctx.retval is set.

Quoted 2 lines

If it turns out to be super ugly, we can try to fix it. (not sure how much of a uapi that is)

sgtm.

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Let me verify and I can add a note to bpf_set_retval uapi definition to mention that it just overrides EPERM. bpf_set_retval should probably not talk about userspace/syscall and instead use the words like "caller".

yeah, it is no longer syscall return value only now.

Quoted 36 lines · Martin KaFai Lau

Quoted 34 lines · Stanislav Fomichev

Quoted 32 lines · Martin KaFai Lau

Quoted 14 lines · Stanislav Fomichev

Quoted 13 lines · Martin KaFai Lau

Quoted 11 lines · Stanislav Fomichev

Apparently, only a small subset of cgroup hooks actually falls back to cgroup_base_func_proto. This leads to unexpected result where not all cgroup helpers have bpf_{g,s}et_retval.

It's getting harder and harder to manage which helpers are exported to which hooks. We now have the following call chains:

- cg_skb_func_proto
- sk_filter_func_proto
- bpf_sk_base_func_proto
- bpf_base_func_proto

Quoted 8 lines

Quoted 4 lines

Could you explain how bpf_set_retval() will work with cgroup prog that is not syscall and can return flags in the higher bit (e.g. cg_skb egress). It will be a useful doc to add to the uapi bpf.h for
the bpf_set_retval() helper.

I think it's the same case as the case without bpf_set_retval? I don't think the flags can be exported via bpf_set_retval, it just lets the users override EPERM.

eg. Before, a cg_skb@egress prog returns 3 to mean NET_XMIT_CN. What if the prog now returns 3 and also bpf_set_retval(-Exxxx). If I read how __cgroup_bpf_run_filter_skb() uses bpf_prog_run_array_cg() correctly, __cgroup_bpf_run_filter_skb() will return NET_XMIT_DROP instead of the -Exxxx. The -Exxxx is probably what the bpf prog is expecting after calling bpf_set_retval(-Exxxx) ?
Thinking more about it, should __cgroup_bpf_run_filter_skb() always return -Exxxx whenever a -ve retval is set in bpf_set_retval() ?

Quoted 4 lines

Quoted 2 lines

I think we used to have "return 0/1/2/3" to indicate different conditions but then switched to "return 1/0" + flags.

For 'int bpf_prog_run_array_cg(..., u32 *ret_flags)'? I think it is more like return "0 (OK)/-Exxxx" + ret_flags now.

Yes, right now that's that case. What I meant to say is that for the BPF program itself, the api is still "return a set of predefined values". We don't advertise the flags to the bpf programs. 'return 2' is a perfectly valid return for cgroup/egress that will tx the packet with a cn. (where bpf_prog_run_array_cg sees it as a 'return 0 + (1 << 1)')

Quoted 4 lines

Quoted 2 lines

So, technically, "return 3 + bpf_set_retval" is still fundamentally a "return 3" api-wise.

hm....for the exisiting usecase (eg. CGROUP_SETSOCKOPT), what does "bpf-prog-return 1 + bpf_set_retval(-EPERM)" mean?

I think bpf_set_retval takes precedence and in this case bpf_prog_run wrapper will return -EPERM to the caller.
Will try to document that as well.

Quoted 4 lines

Quoted 2 lines

I guess we can make bpf_set_retval override that but let me start by trying to document what we currently have.

To be clear, for cg_skb case, I meant to clear the ret_flags only if run_ctx.retval is set.

Are you suggesting something like the following?

kernel/bpf/cgroup.c +2 -0

index fd113bd2f79c..c110cbe52001 100644
@@ -61,6 +61,8 @@ bpf_prog_run_array_cg(const struct cgroup_bpf *cgrp,
        bpf_reset_run_ctx(old_run_ctx);
        rcu_read_unlock();
        migrate_enable();
+       if (IS_ERR_VALUE((long)run_ctx.retval))
+               *ret_flags = 0;
        return run_ctx.retval;
 }
 

I think this will break the 'return 2' case? But is it worth it doing it more carefully like this? LMKWYT.

kernel/bpf/cgroup.c +6 -1

index fd113bd2f79c..dcd25561f8d4 100644
@@ -39,6 +39,7 @@ bpf_prog_run_array_cg(const struct cgroup_bpf *cgrp,
        const struct bpf_prog_array *array;
        struct bpf_run_ctx *old_run_ctx;
        struct bpf_cg_run_ctx run_ctx;
+       bool seen_return0 = false;
        u32 func_ret;
 
        run_ctx.retval = retval;

@@ -54,13 +55,17 @@ 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 && !IS_ERR_VALUE((long)run_ctx.retval)) {
                        run_ctx.retval = -EPERM;
+                       seen_return0 = true;
+               }
                item++;
        }
        bpf_reset_run_ctx(old_run_ctx);
        rcu_read_unlock();
        migrate_enable();
+       if (IS_ERR_VALUE((long)run_ctx.retval) && !seen_return0)
+               *ret_flags = 0;
        return run_ctx.retval;
 }
 

Quoted 12 lines

Quoted 2 lines

If it turns out to be super ugly, we can try to fix it. (not sure how much of a uapi that is)

sgtm.

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Let me verify and I can add a note to bpf_set_retval uapi definition to mention that it just overrides EPERM. bpf_set_retval should probably not talk about userspace/syscall and instead use the words like "caller".

yeah, it is no longer syscall return value only now.

Quoted 78 lines · Stanislav Fomichev

Quoted 36 lines · Martin KaFai Lau

Quoted 34 lines · Stanislav Fomichev

Quoted 32 lines · Martin KaFai Lau

Quoted 14 lines · Stanislav Fomichev

Quoted 13 lines · Martin KaFai Lau

Quoted 11 lines · Stanislav Fomichev

Apparently, only a small subset of cgroup hooks actually falls back to cgroup_base_func_proto. This leads to unexpected result where not all cgroup helpers have bpf_{g,s}et_retval.

It's getting harder and harder to manage which helpers are exported to which hooks. We now have the following call chains:

- cg_skb_func_proto
- sk_filter_func_proto
- bpf_sk_base_func_proto
- bpf_base_func_proto

Quoted 8 lines

Quoted 4 lines

Could you explain how bpf_set_retval() will work with cgroup prog that is not syscall and can return flags in the higher bit (e.g. cg_skb egress). It will be a useful doc to add to the uapi bpf.h for
the bpf_set_retval() helper.

I think it's the same case as the case without bpf_set_retval? I don't think the flags can be exported via bpf_set_retval, it just lets the users override EPERM.

eg. Before, a cg_skb@egress prog returns 3 to mean NET_XMIT_CN. What if the prog now returns 3 and also bpf_set_retval(-Exxxx). If I read how __cgroup_bpf_run_filter_skb() uses bpf_prog_run_array_cg() correctly, __cgroup_bpf_run_filter_skb() will return NET_XMIT_DROP instead of the -Exxxx. The -Exxxx is probably what the bpf prog is expecting after calling bpf_set_retval(-Exxxx) ?
Thinking more about it, should __cgroup_bpf_run_filter_skb() always return -Exxxx whenever a -ve retval is set in bpf_set_retval() ?

Quoted 4 lines

Quoted 2 lines

I think we used to have "return 0/1/2/3" to indicate different conditions but then switched to "return 1/0" + flags.

For 'int bpf_prog_run_array_cg(..., u32 *ret_flags)'? I think it is more like return "0 (OK)/-Exxxx" + ret_flags now.

Yes, right now that's that case. What I meant to say is that for the BPF program itself, the api is still "return a set of predefined values". We don't advertise the flags to the bpf programs. 'return 2' is a perfectly valid return for cgroup/egress that will tx the packet with a cn. (where bpf_prog_run_array_cg sees it as a 'return 0 + (1 << 1)')

Quoted 4 lines

Quoted 2 lines

So, technically, "return 3 + bpf_set_retval" is still fundamentally a "return 3" api-wise.

hm....for the exisiting usecase (eg. CGROUP_SETSOCKOPT), what does "bpf-prog-return 1 + bpf_set_retval(-EPERM)" mean?

I think bpf_set_retval takes precedence and in this case bpf_prog_run wrapper will return -EPERM to the caller.
Will try to document that as well.

Quoted 4 lines

Quoted 2 lines

I guess we can make bpf_set_retval override that but let me start by trying to document what we currently have.

To be clear, for cg_skb case, I meant to clear the ret_flags only if run_ctx.retval is set.

Are you suggesting something like the following?

kernel/bpf/cgroup.c +2 -0

index fd113bd2f79c..c110cbe52001 100644
@@ -61,6 +61,8 @@ bpf_prog_run_array_cg(const struct cgroup_bpf *cgrp,
        bpf_reset_run_ctx(old_run_ctx);
        rcu_read_unlock();
        migrate_enable();
+       if (IS_ERR_VALUE((long)run_ctx.retval))
+               *ret_flags = 0;
        return run_ctx.retval;
 }
 

Quoted 2 lines

I think this will break the 'return 2' case? But is it worth it doing it more carefully like this? LMKWYT.

The below should work. Not sure it is worth it
but before doing this...

During this discussion, I think I am not clear what is the use case on bpf_{g,s}et_retval() for cg_skb. Could you describe how it will be used in your use case? Is it for another tracing program to get a different return value from (eg.) sk_filter_trim_cap or ip[6]_output?

Not meaning the helper should not be exposed. It is easier to think with some examples.

Quoted 90 lines · Martin KaFai Lau

Quoted 78 lines · Stanislav Fomichev

Quoted 36 lines · Martin KaFai Lau

Quoted 34 lines · Stanislav Fomichev

Quoted 32 lines · Martin KaFai Lau

Quoted 14 lines · Stanislav Fomichev

Quoted 13 lines · Martin KaFai Lau

Quoted 11 lines · Stanislav Fomichev

Apparently, only a small subset of cgroup hooks actually falls back to cgroup_base_func_proto. This leads to unexpected result where not all cgroup helpers have bpf_{g,s}et_retval.

It's getting harder and harder to manage which helpers are exported to which hooks. We now have the following call chains:

- cg_skb_func_proto
- sk_filter_func_proto
- bpf_sk_base_func_proto
- bpf_base_func_proto

Quoted 8 lines

Quoted 4 lines

Could you explain how bpf_set_retval() will work with cgroup prog that is not syscall and can return flags in the higher bit (e.g. cg_skb egress). It will be a useful doc to add to the uapi bpf.h for
the bpf_set_retval() helper.

I think it's the same case as the case without bpf_set_retval? I don't think the flags can be exported via bpf_set_retval, it just lets the users override EPERM.

eg. Before, a cg_skb@egress prog returns 3 to mean NET_XMIT_CN. What if the prog now returns 3 and also bpf_set_retval(-Exxxx). If I read how __cgroup_bpf_run_filter_skb() uses bpf_prog_run_array_cg() correctly, __cgroup_bpf_run_filter_skb() will return NET_XMIT_DROP instead of the -Exxxx. The -Exxxx is probably what the bpf prog is expecting after calling bpf_set_retval(-Exxxx) ?
Thinking more about it, should __cgroup_bpf_run_filter_skb() always return -Exxxx whenever a -ve retval is set in bpf_set_retval() ?

Quoted 4 lines

Quoted 2 lines

I think we used to have "return 0/1/2/3" to indicate different conditions but then switched to "return 1/0" + flags.

For 'int bpf_prog_run_array_cg(..., u32 *ret_flags)'? I think it is more like return "0 (OK)/-Exxxx" + ret_flags now.

Yes, right now that's that case. What I meant to say is that for the BPF program itself, the api is still "return a set of predefined values". We don't advertise the flags to the bpf programs. 'return 2' is a perfectly valid return for cgroup/egress that will tx the packet with a cn. (where bpf_prog_run_array_cg sees it as a 'return 0 + (1 << 1)')

Quoted 4 lines

Quoted 2 lines

So, technically, "return 3 + bpf_set_retval" is still fundamentally a "return 3" api-wise.

hm....for the exisiting usecase (eg. CGROUP_SETSOCKOPT), what does "bpf-prog-return 1 + bpf_set_retval(-EPERM)" mean?

I think bpf_set_retval takes precedence and in this case bpf_prog_run wrapper will return -EPERM to the caller.
Will try to document that as well.

Quoted 4 lines

Quoted 2 lines

I guess we can make bpf_set_retval override that but let me start by trying to document what we currently have.

To be clear, for cg_skb case, I meant to clear the ret_flags only if run_ctx.retval is set.

Are you suggesting something like the following?

kernel/bpf/cgroup.c +2 -0

index fd113bd2f79c..c110cbe52001 100644
@@ -61,6 +61,8 @@ bpf_prog_run_array_cg(const struct cgroup_bpf *cgrp,
        bpf_reset_run_ctx(old_run_ctx);
        rcu_read_unlock();
        migrate_enable();
+       if (IS_ERR_VALUE((long)run_ctx.retval))
+               *ret_flags = 0;
        return run_ctx.retval;
 }
 

Quoted 2 lines

I think this will break the 'return 2' case? But is it worth it doing it more carefully like this? LMKWYT.

The below should work. Not sure it is worth it
but before doing this...

During this discussion, I think I am not clear what is the use case on bpf_{g,s}et_retval() for cg_skb. Could you describe how it will be used in your use case? Is it for another tracing program to get a different return value from (eg.) sk_filter_trim_cap or ip[6]_output?

Quoted 2 lines

Not meaning the helper should not be exposed. It is easier to think with some examples.

I don't really need them in cg_skb, I want them in cg_sock so I can return a custom errno from socket() syscall.

You're probably right and it doesn't make sense to support them in cg_skb. Most of the BPF_CGROUP_RUN_PROG_INET_INGRESS/BPF_CGROUP_RUN_PROG_INET_EGRESS callers don't seem to care about returned error code? (from my brief grepping)
Let's maybe err on the safe side and special case cg_skb for now (in cgroup_common_func_proto) and not expose retval helpers?