* [PATCH bpf] bpf: handle return value of BPF_PROG_TYPE_STRUCT_OPS prog
@ 2021-09-01 8:53 Hou Tao
2021-09-07 11:13 ` Hou Tao
2021-09-08 6:06 ` Martin KaFai Lau
0 siblings, 2 replies; 8+ messages in thread
From: Hou Tao @ 2021-09-01 8:53 UTC (permalink / raw)
To: bpf, Alexei Starovoitov, Daniel Borkmann
Cc: Martin KaFai Lau, Andrii Nakryiko, KP Singh, houtao1
Currently if a function ptr in struct_ops has a return value, its
caller will get a random return value from it, because the return
value of related BPF_PROG_TYPE_STRUCT_OPS prog is just dropped.
So adding a new flag BPF_TRAMP_F_RET_FENTRY_RET to tell bpf trampoline
to save and return the return value of struct_ops prog if ret_size of
the function ptr is greater than 0. Also restricting the flag to be
used alone.
Fixes: 85d33df357b6 ("bpf: Introduce BPF_MAP_TYPE_STRUCT_OPS")
Signed-off-by: Hou Tao <houtao1@huawei.com>
---
arch/x86/net/bpf_jit_comp.c | 53 ++++++++++++++++++++++++++++---------
include/linux/bpf.h | 2 ++
kernel/bpf/bpf_struct_ops.c | 7 +++--
3 files changed, 47 insertions(+), 15 deletions(-)
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 16d76f814e9b..47780844598a 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1744,7 +1744,7 @@ static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
}
static int invoke_bpf_prog(const struct btf_func_model *m, u8 **pprog,
- struct bpf_prog *p, int stack_size, bool mod_ret)
+ struct bpf_prog *p, int stack_size, bool save_ret)
{
u8 *prog = *pprog;
u8 *jmp_insn;
@@ -1777,11 +1777,15 @@ static int invoke_bpf_prog(const struct btf_func_model *m, u8 **pprog,
if (emit_call(&prog, p->bpf_func, prog))
return -EINVAL;
- /* BPF_TRAMP_MODIFY_RETURN trampolines can modify the return
+ /*
+ * BPF_TRAMP_MODIFY_RETURN trampolines can modify the return
* of the previous call which is then passed on the stack to
* the next BPF program.
+ *
+ * BPF_TRAMP_FENTRY trampoline may need to return the return
+ * value of BPF_PROG_TYPE_STRUCT_OPS prog.
*/
- if (mod_ret)
+ if (save_ret)
emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8);
/* replace 2 nops with JE insn, since jmp target is known */
@@ -1828,13 +1832,15 @@ static int emit_cond_near_jump(u8 **pprog, void *func, void *ip, u8 jmp_cond)
}
static int invoke_bpf(const struct btf_func_model *m, u8 **pprog,
- struct bpf_tramp_progs *tp, int stack_size)
+ struct bpf_tramp_progs *tp, int stack_size,
+ bool save_ret)
{
int i;
u8 *prog = *pprog;
for (i = 0; i < tp->nr_progs; i++) {
- if (invoke_bpf_prog(m, &prog, tp->progs[i], stack_size, false))
+ if (invoke_bpf_prog(m, &prog, tp->progs[i], stack_size,
+ save_ret))
return -EINVAL;
}
*pprog = prog;
@@ -1877,6 +1883,23 @@ static int invoke_bpf_mod_ret(const struct btf_func_model *m, u8 **pprog,
return 0;
}
+static bool is_valid_bpf_tramp_flags(unsigned int flags)
+{
+ if ((flags & BPF_TRAMP_F_RESTORE_REGS) &&
+ (flags & BPF_TRAMP_F_SKIP_FRAME))
+ return false;
+
+ /*
+ * BPF_TRAMP_F_RET_FENTRY_RET is only used by bpf_struct_ops,
+ * and it must be used alone.
+ */
+ if ((flags & BPF_TRAMP_F_RET_FENTRY_RET) &&
+ (flags & ~BPF_TRAMP_F_RET_FENTRY_RET))
+ return false;
+
+ return true;
+}
+
/* Example:
* __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev);
* its 'struct btf_func_model' will be nr_args=2
@@ -1949,17 +1972,19 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
struct bpf_tramp_progs *fmod_ret = &tprogs[BPF_TRAMP_MODIFY_RETURN];
u8 **branches = NULL;
u8 *prog;
+ bool save_ret;
/* x86-64 supports up to 6 arguments. 7+ can be added in the future */
if (nr_args > 6)
return -ENOTSUPP;
- if ((flags & BPF_TRAMP_F_RESTORE_REGS) &&
- (flags & BPF_TRAMP_F_SKIP_FRAME))
+ if (!is_valid_bpf_tramp_flags(flags))
return -EINVAL;
- if (flags & BPF_TRAMP_F_CALL_ORIG)
- stack_size += 8; /* room for return value of orig_call */
+ /* room for return value of orig_call or fentry prog */
+ save_ret = flags & (BPF_TRAMP_F_CALL_ORIG | BPF_TRAMP_F_RET_FENTRY_RET);
+ if (save_ret)
+ stack_size += 8;
if (flags & BPF_TRAMP_F_SKIP_FRAME)
/* skip patched call instruction and point orig_call to actual
@@ -1986,7 +2011,8 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
}
if (fentry->nr_progs)
- if (invoke_bpf(m, &prog, fentry, stack_size))
+ if (invoke_bpf(m, &prog, fentry, stack_size,
+ flags & BPF_TRAMP_F_RET_FENTRY_RET))
return -EINVAL;
if (fmod_ret->nr_progs) {
@@ -2033,7 +2059,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
}
if (fexit->nr_progs)
- if (invoke_bpf(m, &prog, fexit, stack_size)) {
+ if (invoke_bpf(m, &prog, fexit, stack_size, false)) {
ret = -EINVAL;
goto cleanup;
}
@@ -2053,9 +2079,10 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
ret = -EINVAL;
goto cleanup;
}
- /* restore original return value back into RAX */
- emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, -8);
}
+ /* restore return value of orig_call or fentry prog back into RAX */
+ if (save_ret)
+ emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, -8);
EMIT1(0x5B); /* pop rbx */
EMIT1(0xC9); /* leave */
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index e8e2b0393ca9..85413eb368de 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -553,6 +553,8 @@ struct btf_func_model {
* programs only. Should not be used with normal calls and indirect calls.
*/
#define BPF_TRAMP_F_SKIP_FRAME BIT(2)
+/* Return the return value of fentry prog. Only used by bpf_struct_ops. */
+#define BPF_TRAMP_F_RET_FENTRY_RET BIT(3)
/* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50
* bytes on x86. Pick a number to fit into BPF_IMAGE_SIZE / 2
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 70f6fd4fa305..2ce17447fb76 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -367,6 +367,7 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
const struct btf_type *mtype, *ptype;
struct bpf_prog *prog;
u32 moff;
+ u32 flags;
moff = btf_member_bit_offset(t, member) / 8;
ptype = btf_type_resolve_ptr(btf_vmlinux, member->type, NULL);
@@ -430,10 +431,12 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
tprogs[BPF_TRAMP_FENTRY].progs[0] = prog;
tprogs[BPF_TRAMP_FENTRY].nr_progs = 1;
+ flags = st_ops->func_models[i].ret_size > 0 ?
+ BPF_TRAMP_F_RET_FENTRY_RET : 0;
err = arch_prepare_bpf_trampoline(NULL, image,
st_map->image + PAGE_SIZE,
- &st_ops->func_models[i], 0,
- tprogs, NULL);
+ &st_ops->func_models[i],
+ flags, tprogs, NULL);
if (err < 0)
goto reset_unlock;
--
2.29.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH bpf] bpf: handle return value of BPF_PROG_TYPE_STRUCT_OPS prog
2021-09-01 8:53 [PATCH bpf] bpf: handle return value of BPF_PROG_TYPE_STRUCT_OPS prog Hou Tao
@ 2021-09-07 11:13 ` Hou Tao
2021-09-08 6:06 ` Martin KaFai Lau
1 sibling, 0 replies; 8+ messages in thread
From: Hou Tao @ 2021-09-07 11:13 UTC (permalink / raw)
To: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, netdev
Cc: Martin KaFai Lau, KP Singh
ping ?
+cc netdev
On 9/1/2021 4:53 PM, Hou Tao wrote:
> Currently if a function ptr in struct_ops has a return value, its
> caller will get a random return value from it, because the return
> value of related BPF_PROG_TYPE_STRUCT_OPS prog is just dropped.
>
> So adding a new flag BPF_TRAMP_F_RET_FENTRY_RET to tell bpf trampoline
> to save and return the return value of struct_ops prog if ret_size of
> the function ptr is greater than 0. Also restricting the flag to be
> used alone.
>
> Fixes: 85d33df357b6 ("bpf: Introduce BPF_MAP_TYPE_STRUCT_OPS")
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
> arch/x86/net/bpf_jit_comp.c | 53 ++++++++++++++++++++++++++++---------
> include/linux/bpf.h | 2 ++
> kernel/bpf/bpf_struct_ops.c | 7 +++--
> 3 files changed, 47 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 16d76f814e9b..47780844598a 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1744,7 +1744,7 @@ static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
> }
>
> static int invoke_bpf_prog(const struct btf_func_model *m, u8 **pprog,
> - struct bpf_prog *p, int stack_size, bool mod_ret)
> + struct bpf_prog *p, int stack_size, bool save_ret)
> {
> u8 *prog = *pprog;
> u8 *jmp_insn;
> @@ -1777,11 +1777,15 @@ static int invoke_bpf_prog(const struct btf_func_model *m, u8 **pprog,
> if (emit_call(&prog, p->bpf_func, prog))
> return -EINVAL;
>
> - /* BPF_TRAMP_MODIFY_RETURN trampolines can modify the return
> + /*
> + * BPF_TRAMP_MODIFY_RETURN trampolines can modify the return
> * of the previous call which is then passed on the stack to
> * the next BPF program.
> + *
> + * BPF_TRAMP_FENTRY trampoline may need to return the return
> + * value of BPF_PROG_TYPE_STRUCT_OPS prog.
> */
> - if (mod_ret)
> + if (save_ret)
> emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8);
>
> /* replace 2 nops with JE insn, since jmp target is known */
> @@ -1828,13 +1832,15 @@ static int emit_cond_near_jump(u8 **pprog, void *func, void *ip, u8 jmp_cond)
> }
>
> static int invoke_bpf(const struct btf_func_model *m, u8 **pprog,
> - struct bpf_tramp_progs *tp, int stack_size)
> + struct bpf_tramp_progs *tp, int stack_size,
> + bool save_ret)
> {
> int i;
> u8 *prog = *pprog;
>
> for (i = 0; i < tp->nr_progs; i++) {
> - if (invoke_bpf_prog(m, &prog, tp->progs[i], stack_size, false))
> + if (invoke_bpf_prog(m, &prog, tp->progs[i], stack_size,
> + save_ret))
> return -EINVAL;
> }
> *pprog = prog;
> @@ -1877,6 +1883,23 @@ static int invoke_bpf_mod_ret(const struct btf_func_model *m, u8 **pprog,
> return 0;
> }
>
> +static bool is_valid_bpf_tramp_flags(unsigned int flags)
> +{
> + if ((flags & BPF_TRAMP_F_RESTORE_REGS) &&
> + (flags & BPF_TRAMP_F_SKIP_FRAME))
> + return false;
> +
> + /*
> + * BPF_TRAMP_F_RET_FENTRY_RET is only used by bpf_struct_ops,
> + * and it must be used alone.
> + */
> + if ((flags & BPF_TRAMP_F_RET_FENTRY_RET) &&
> + (flags & ~BPF_TRAMP_F_RET_FENTRY_RET))
> + return false;
> +
> + return true;
> +}
> +
> /* Example:
> * __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev);
> * its 'struct btf_func_model' will be nr_args=2
> @@ -1949,17 +1972,19 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
> struct bpf_tramp_progs *fmod_ret = &tprogs[BPF_TRAMP_MODIFY_RETURN];
> u8 **branches = NULL;
> u8 *prog;
> + bool save_ret;
>
> /* x86-64 supports up to 6 arguments. 7+ can be added in the future */
> if (nr_args > 6)
> return -ENOTSUPP;
>
> - if ((flags & BPF_TRAMP_F_RESTORE_REGS) &&
> - (flags & BPF_TRAMP_F_SKIP_FRAME))
> + if (!is_valid_bpf_tramp_flags(flags))
> return -EINVAL;
>
> - if (flags & BPF_TRAMP_F_CALL_ORIG)
> - stack_size += 8; /* room for return value of orig_call */
> + /* room for return value of orig_call or fentry prog */
> + save_ret = flags & (BPF_TRAMP_F_CALL_ORIG | BPF_TRAMP_F_RET_FENTRY_RET);
> + if (save_ret)
> + stack_size += 8;
>
> if (flags & BPF_TRAMP_F_SKIP_FRAME)
> /* skip patched call instruction and point orig_call to actual
> @@ -1986,7 +2011,8 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
> }
>
> if (fentry->nr_progs)
> - if (invoke_bpf(m, &prog, fentry, stack_size))
> + if (invoke_bpf(m, &prog, fentry, stack_size,
> + flags & BPF_TRAMP_F_RET_FENTRY_RET))
> return -EINVAL;
>
> if (fmod_ret->nr_progs) {
> @@ -2033,7 +2059,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
> }
>
> if (fexit->nr_progs)
> - if (invoke_bpf(m, &prog, fexit, stack_size)) {
> + if (invoke_bpf(m, &prog, fexit, stack_size, false)) {
> ret = -EINVAL;
> goto cleanup;
> }
> @@ -2053,9 +2079,10 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
> ret = -EINVAL;
> goto cleanup;
> }
> - /* restore original return value back into RAX */
> - emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, -8);
> }
> + /* restore return value of orig_call or fentry prog back into RAX */
> + if (save_ret)
> + emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, -8);
>
> EMIT1(0x5B); /* pop rbx */
> EMIT1(0xC9); /* leave */
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index e8e2b0393ca9..85413eb368de 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -553,6 +553,8 @@ struct btf_func_model {
> * programs only. Should not be used with normal calls and indirect calls.
> */
> #define BPF_TRAMP_F_SKIP_FRAME BIT(2)
> +/* Return the return value of fentry prog. Only used by bpf_struct_ops. */
> +#define BPF_TRAMP_F_RET_FENTRY_RET BIT(3)
>
> /* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50
> * bytes on x86. Pick a number to fit into BPF_IMAGE_SIZE / 2
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 70f6fd4fa305..2ce17447fb76 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -367,6 +367,7 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
> const struct btf_type *mtype, *ptype;
> struct bpf_prog *prog;
> u32 moff;
> + u32 flags;
>
> moff = btf_member_bit_offset(t, member) / 8;
> ptype = btf_type_resolve_ptr(btf_vmlinux, member->type, NULL);
> @@ -430,10 +431,12 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
>
> tprogs[BPF_TRAMP_FENTRY].progs[0] = prog;
> tprogs[BPF_TRAMP_FENTRY].nr_progs = 1;
> + flags = st_ops->func_models[i].ret_size > 0 ?
> + BPF_TRAMP_F_RET_FENTRY_RET : 0;
> err = arch_prepare_bpf_trampoline(NULL, image,
> st_map->image + PAGE_SIZE,
> - &st_ops->func_models[i], 0,
> - tprogs, NULL);
> + &st_ops->func_models[i],
> + flags, tprogs, NULL);
> if (err < 0)
> goto reset_unlock;
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf] bpf: handle return value of BPF_PROG_TYPE_STRUCT_OPS prog
2021-09-01 8:53 [PATCH bpf] bpf: handle return value of BPF_PROG_TYPE_STRUCT_OPS prog Hou Tao
2021-09-07 11:13 ` Hou Tao
@ 2021-09-08 6:06 ` Martin KaFai Lau
2021-09-08 13:31 ` Hou Tao
1 sibling, 1 reply; 8+ messages in thread
From: Martin KaFai Lau @ 2021-09-08 6:06 UTC (permalink / raw)
To: Hou Tao
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, KP Singh
On Wed, Sep 01, 2021 at 04:53:44PM +0800, Hou Tao wrote:
> Currently if a function ptr in struct_ops has a return value, its
> caller will get a random return value from it, because the return
> value of related BPF_PROG_TYPE_STRUCT_OPS prog is just dropped.
>
> So adding a new flag BPF_TRAMP_F_RET_FENTRY_RET to tell bpf trampoline
> to save and return the return value of struct_ops prog if ret_size of
> the function ptr is greater than 0. Also restricting the flag to be
> used alone.
Thanks for the report and fix! Sorry for the late reply.
This bug is missed because the tcp-cc func is not always called.
A better test needs to be created to force exercising these funcs
in bpf_test_run(), which can be a follow-up patch in the bpf-next.
Could you help to create this test as a follow up?
The patch lgtm. However, it does not apply cleanly on bpf,
so please rebase and repost. I applied it manually and
tested it by hard coding to call the ->ssthresh() and
observes the return value.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf] bpf: handle return value of BPF_PROG_TYPE_STRUCT_OPS prog
2021-09-08 6:06 ` Martin KaFai Lau
@ 2021-09-08 13:31 ` Hou Tao
2021-09-08 17:19 ` Martin KaFai Lau
0 siblings, 1 reply; 8+ messages in thread
From: Hou Tao @ 2021-09-08 13:31 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, KP Singh
Hi,
On 9/8/2021 2:06 PM, Martin KaFai Lau wrote:
> On Wed, Sep 01, 2021 at 04:53:44PM +0800, Hou Tao wrote:
>> Currently if a function ptr in struct_ops has a return value, its
>> caller will get a random return value from it, because the return
>> value of related BPF_PROG_TYPE_STRUCT_OPS prog is just dropped.
>>
>> So adding a new flag BPF_TRAMP_F_RET_FENTRY_RET to tell bpf trampoline
>> to save and return the return value of struct_ops prog if ret_size of
>> the function ptr is greater than 0. Also restricting the flag to be
>> used alone.
> Thanks for the report and fix! Sorry for the late reply.
>
> This bug is missed because the tcp-cc func is not always called.
> A better test needs to be created to force exercising these funcs
> in bpf_test_run(), which can be a follow-up patch in the bpf-next.
> Could you help to create this test as a follow up?
Yes, will do. The first thought comes into my mind is implementing .get_info hook
in a bpf tcp_congestion_ops and checking its return value in userspace by
getsockopt(fd, TCP_CC_INFO). I also consider to add a new BPF struct_ops
for testing purpose, but it may be a little overkill.
> The patch lgtm. However, it does not apply cleanly on bpf,
> so please rebase and repost. I applied it manually and
> tested it by hard coding to call the ->ssthresh() and
> observes the return value.
I just check that it can be applied both on bpf and bpf-next, do you
have other commits in your tree ?
> .
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf] bpf: handle return value of BPF_PROG_TYPE_STRUCT_OPS prog
2021-09-08 13:31 ` Hou Tao
@ 2021-09-08 17:19 ` Martin KaFai Lau
2021-09-08 17:27 ` Martin KaFai Lau
2021-09-09 1:45 ` Hou Tao
0 siblings, 2 replies; 8+ messages in thread
From: Martin KaFai Lau @ 2021-09-08 17:19 UTC (permalink / raw)
To: Hou Tao
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, KP Singh
On Wed, Sep 08, 2021 at 09:31:55PM +0800, Hou Tao wrote:
> Hi,
>
> On 9/8/2021 2:06 PM, Martin KaFai Lau wrote:
> > On Wed, Sep 01, 2021 at 04:53:44PM +0800, Hou Tao wrote:
> >> Currently if a function ptr in struct_ops has a return value, its
> >> caller will get a random return value from it, because the return
> >> value of related BPF_PROG_TYPE_STRUCT_OPS prog is just dropped.
> >>
> >> So adding a new flag BPF_TRAMP_F_RET_FENTRY_RET to tell bpf trampoline
> >> to save and return the return value of struct_ops prog if ret_size of
> >> the function ptr is greater than 0. Also restricting the flag to be
> >> used alone.
> > Thanks for the report and fix! Sorry for the late reply.
> >
> > This bug is missed because the tcp-cc func is not always called.
> > A better test needs to be created to force exercising these funcs
> > in bpf_test_run(), which can be a follow-up patch in the bpf-next.
> > Could you help to create this test as a follow up?
>
> Yes, will do. The first thought comes into my mind is implementing .get_info hook
> in a bpf tcp_congestion_ops and checking its return value in userspace by
> getsockopt(fd, TCP_CC_INFO).
The bpf-tcp-cc's struct_ops currently does not support ".get_info".
It will be a good addition also.
Different bpf-tcp-cc implementations have different infos, so it cannot be
bounded by a fixed struct like 'union tcp_cc_info'. The format should be
a btf_id followed by the actual info-data. The kernel should be able to
learn the size of the info-data from the btf_id. The ".get_info" is
also used by inet_diag for tools (ss) like iproute2. libbpf can pretty-print
the btf described data and libbpf support is added to iproute2, so pieces
should be in-place for iproute2's tools to handle data described by btf.
For ".get_info" in getsockopt(TCP_CC_INFO), not sure how the application
may use them but I think it will at least enable the application log
them as other kernel's tcp-cc do. The implementation details may
need some more thoughts but should not be a big issue.
> I also consider to add a new BPF struct_ops
> for testing purpose, but it may be a little overkill.
A dummy struct_ops for testing makes sense. It probably should
be the one done first for testing purpose. Although "get_info"
is a good add, having a separate testing struct_ops will be easier
to test other interesting cases in the future.
> I just check that it can be applied both on bpf and bpf-next, do you
> have other commits in your tree ?
There is no local commit.
From a quick look, the patch is created from a pretty old tree and it
is missing the BPF_TRAMP_F_SKIP_FRAME. It is introduced in
commit 7e6f3cd89f04 ("bpf, x86: Store caller's ip in trampoline stack")
on Jul 15 2021 which is pretty old.
I am only able to apply with the --3way merge like "git am --3way".
Andrii, is it fine to land the patch like this?
> @@ -1949,17 +1972,19 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
> struct bpf_tramp_progs *fmod_ret = &tprogs[BPF_TRAMP_MODIFY_RETURN];
> u8 **branches = NULL;
> u8 *prog;
> + bool save_ret;
>
> /* x86-64 supports up to 6 arguments. 7+ can be added in the future */
> if (nr_args > 6)
> return -ENOTSUPP;
>
> - if ((flags & BPF_TRAMP_F_RESTORE_REGS) &&
> - (flags & BPF_TRAMP_F_SKIP_FRAME))
> + if (!is_valid_bpf_tramp_flags(flags))
> return -EINVAL;
>
> - if (flags & BPF_TRAMP_F_CALL_ORIG)
> - stack_size += 8; /* room for return value of orig_call */
> + /* room for return value of orig_call or fentry prog */
> + save_ret = flags & (BPF_TRAMP_F_CALL_ORIG | BPF_TRAMP_F_RET_FENTRY_RET);
> + if (save_ret)
> + stack_size += 8;
>
> if (flags & BPF_TRAMP_F_SKIP_FRAME)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> /* skip patched call instruction and point orig_call to actual
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf] bpf: handle return value of BPF_PROG_TYPE_STRUCT_OPS prog
2021-09-08 17:19 ` Martin KaFai Lau
@ 2021-09-08 17:27 ` Martin KaFai Lau
2021-09-09 1:45 ` Hou Tao
1 sibling, 0 replies; 8+ messages in thread
From: Martin KaFai Lau @ 2021-09-08 17:27 UTC (permalink / raw)
To: Hou Tao
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, KP Singh
On Wed, Sep 08, 2021 at 10:19:39AM -0700, Martin KaFai Lau wrote:
> On Wed, Sep 08, 2021 at 09:31:55PM +0800, Hou Tao wrote:
> > Hi,
> >
> > On 9/8/2021 2:06 PM, Martin KaFai Lau wrote:
> > > On Wed, Sep 01, 2021 at 04:53:44PM +0800, Hou Tao wrote:
> > >> Currently if a function ptr in struct_ops has a return value, its
> > >> caller will get a random return value from it, because the return
> > >> value of related BPF_PROG_TYPE_STRUCT_OPS prog is just dropped.
> > >>
> > >> So adding a new flag BPF_TRAMP_F_RET_FENTRY_RET to tell bpf trampoline
> > >> to save and return the return value of struct_ops prog if ret_size of
> > >> the function ptr is greater than 0. Also restricting the flag to be
> > >> used alone.
> > > Thanks for the report and fix! Sorry for the late reply.
> > >
> > > This bug is missed because the tcp-cc func is not always called.
> > > A better test needs to be created to force exercising these funcs
> > > in bpf_test_run(), which can be a follow-up patch in the bpf-next.
> > > Could you help to create this test as a follow up?
> >
> > Yes, will do. The first thought comes into my mind is implementing .get_info hook
> > in a bpf tcp_congestion_ops and checking its return value in userspace by
> > getsockopt(fd, TCP_CC_INFO).
> The bpf-tcp-cc's struct_ops currently does not support ".get_info".
> It will be a good addition also.
>
> Different bpf-tcp-cc implementations have different infos, so it cannot be
> bounded by a fixed struct like 'union tcp_cc_info'. The format should be
> a btf_id followed by the actual info-data. The kernel should be able to
> learn the size of the info-data from the btf_id. The ".get_info" is
> also used by inet_diag for tools (ss) like iproute2. libbpf can pretty-print
> the btf described data and libbpf support is added to iproute2, so pieces
> should be in-place for iproute2's tools to handle data described by btf.
>
> For ".get_info" in getsockopt(TCP_CC_INFO), not sure how the application
> may use them but I think it will at least enable the application log
> them as other kernel's tcp-cc do. The implementation details may
> need some more thoughts but should not be a big issue.
>
> > I also consider to add a new BPF struct_ops
> > for testing purpose, but it may be a little overkill.
> A dummy struct_ops for testing makes sense. It probably should
> be the one done first for testing purpose. Although "get_info"
> is a good add, having a separate testing struct_ops will be easier
> to test other interesting cases in the future.
>
> > I just check that it can be applied both on bpf and bpf-next, do you
> > have other commits in your tree ?
> There is no local commit.
>
> From a quick look, the patch is created from a pretty old tree and it
> is missing the BPF_TRAMP_F_SKIP_FRAME. It is introduced in
typo. I meant missing the BPF_TRAMP_F_IP_ARG.
> commit 7e6f3cd89f04 ("bpf, x86: Store caller's ip in trampoline stack")
> on Jul 15 2021 which is pretty old.
>
> I am only able to apply with the --3way merge like "git am --3way".
> Andrii, is it fine to land the patch like this?
>
> > @@ -1949,17 +1972,19 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
> > struct bpf_tramp_progs *fmod_ret = &tprogs[BPF_TRAMP_MODIFY_RETURN];
> > u8 **branches = NULL;
> > u8 *prog;
> > + bool save_ret;
> >
> > /* x86-64 supports up to 6 arguments. 7+ can be added in the future */
> > if (nr_args > 6)
> > return -ENOTSUPP;
> >
> > - if ((flags & BPF_TRAMP_F_RESTORE_REGS) &&
> > - (flags & BPF_TRAMP_F_SKIP_FRAME))
> > + if (!is_valid_bpf_tramp_flags(flags))
> > return -EINVAL;
> >
> > - if (flags & BPF_TRAMP_F_CALL_ORIG)
> > - stack_size += 8; /* room for return value of orig_call */
> > + /* room for return value of orig_call or fentry prog */
> > + save_ret = flags & (BPF_TRAMP_F_CALL_ORIG | BPF_TRAMP_F_RET_FENTRY_RET);
> > + if (save_ret)
> > + stack_size += 8;
> >
> > if (flags & BPF_TRAMP_F_SKIP_FRAME)
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> > /* skip patched call instruction and point orig_call to actual
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf] bpf: handle return value of BPF_PROG_TYPE_STRUCT_OPS prog
2021-09-08 17:19 ` Martin KaFai Lau
2021-09-08 17:27 ` Martin KaFai Lau
@ 2021-09-09 1:45 ` Hou Tao
2021-09-13 20:37 ` Martin KaFai Lau
1 sibling, 1 reply; 8+ messages in thread
From: Hou Tao @ 2021-09-09 1:45 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, KP Singh
Hi,
On 9/9/2021 1:19 AM, Martin KaFai Lau wrote:
> On Wed, Sep 08, 2021 at 09:31:55PM +0800, Hou Tao wrote:
>> Hi,
>>
>> On 9/8/2021 2:06 PM, Martin KaFai Lau wrote:
>>> On Wed, Sep 01, 2021 at 04:53:44PM +0800, Hou Tao wrote:
>>>> Currently if a function ptr in struct_ops has a return value, its
>>>> caller will get a random return value from it, because the return
>>>> value of related BPF_PROG_TYPE_STRUCT_OPS prog is just dropped.
>>>>
>>>> So adding a new flag BPF_TRAMP_F_RET_FENTRY_RET to tell bpf trampoline
>>>> to save and return the return value of struct_ops prog if ret_size of
>>>> the function ptr is greater than 0. Also restricting the flag to be
>>>> used alone.
>>> Thanks for the report and fix! Sorry for the late reply.
>>>
>>> This bug is missed because the tcp-cc func is not always called.
>>> A better test needs to be created to force exercising these funcs
>>> in bpf_test_run(), which can be a follow-up patch in the bpf-next.
>>> Could you help to create this test as a follow up?
>> Yes, will do. The first thought comes into my mind is implementing .get_info hook
>> in a bpf tcp_congestion_ops and checking its return value in userspace by
>> getsockopt(fd, TCP_CC_INFO).
> The bpf-tcp-cc's struct_ops currently does not support ".get_info".
Yes. I just find it in unsupported_ops[].
> It will be a good addition also.
> Different bpf-tcp-cc implementations have different infos, so it cannot be
> bounded by a fixed struct like 'union tcp_cc_info'. The format should be
> a btf_id followed by the actual info-data. The kernel should be able to
> learn the size of the info-data from the btf_id. The ".get_info" is
> also used by inet_diag for tools (ss) like iproute2. libbpf can pretty-print
> the btf described data and libbpf support is added to iproute2, so pieces
> should be in-place for iproute2's tools to handle data described by btf.
>
> For ".get_info" in getsockopt(TCP_CC_INFO), not sure how the application
> may use them but I think it will at least enable the application log
> them as other kernel's tcp-cc do. The implementation details may
> need some more thoughts but should not be a big issue.
getsockopt(TCP_CC_INFO) also calls .get_info although it bounds the max size of
info as sizoef(union_tcp_cc_info), so userspace can check the value of returned optlen.
>> I also consider to add a new BPF struct_ops
>> for testing purpose, but it may be a little overkill.
> A dummy struct_ops for testing makes sense. It probably should
> be the one done first for testing purpose. Although "get_info"
> is a good add, having a separate testing struct_ops will be easier
> to test other interesting cases in the future.
So I will start by adding a dummy struct_ops for testing purpose, because it will
be much simpler and leaving adding support for .get_info for bpf-tcp-cc as another
patch.
>> I just check that it can be applied both on bpf and bpf-next, do you
>> have other commits in your tree ?
> There is no local commit.
>
> >From a quick look, the patch is created from a pretty old tree and it
> is missing the BPF_TRAMP_F_SKIP_FRAME. It is introduced in
> commit 7e6f3cd89f04 ("bpf, x86: Store caller's ip in trampoline stack")
> on Jul 15 2021 which is pretty old.
>
> I am only able to apply with the --3way merge like "git am --3way".
> Andrii, is it fine to land the patch like this?
I cannot apply it cleanly if using "git am --reject xx.patch", and it's OK if
using "git cherry-pick commit_id", so I will rebase and repost it.
>> @@ -1949,17 +1972,19 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>> struct bpf_tramp_progs *fmod_ret = &tprogs[BPF_TRAMP_MODIFY_RETURN];
>> u8 **branches = NULL;
>> u8 *prog;
>> + bool save_ret;
>>
>> /* x86-64 supports up to 6 arguments. 7+ can be added in the future */
>> if (nr_args > 6)
>> return -ENOTSUPP;
>>
>> - if ((flags & BPF_TRAMP_F_RESTORE_REGS) &&
>> - (flags & BPF_TRAMP_F_SKIP_FRAME))
>> + if (!is_valid_bpf_tramp_flags(flags))
>> return -EINVAL;
>>
>> - if (flags & BPF_TRAMP_F_CALL_ORIG)
>> - stack_size += 8; /* room for return value of orig_call */
>> + /* room for return value of orig_call or fentry prog */
>> + save_ret = flags & (BPF_TRAMP_F_CALL_ORIG | BPF_TRAMP_F_RET_FENTRY_RET);
>> + if (save_ret)
>> + stack_size += 8;
>>
>> if (flags & BPF_TRAMP_F_SKIP_FRAME)
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
>> /* skip patched call instruction and point orig_call to actual
> .
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf] bpf: handle return value of BPF_PROG_TYPE_STRUCT_OPS prog
2021-09-09 1:45 ` Hou Tao
@ 2021-09-13 20:37 ` Martin KaFai Lau
0 siblings, 0 replies; 8+ messages in thread
From: Martin KaFai Lau @ 2021-09-13 20:37 UTC (permalink / raw)
To: Hou Tao
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, KP Singh
On Thu, Sep 09, 2021 at 09:45:03AM +0800, Hou Tao wrote:
> > >From a quick look, the patch is created from a pretty old tree and it
> > is missing the BPF_TRAMP_F_SKIP_FRAME. It is introduced in
> > commit 7e6f3cd89f04 ("bpf, x86: Store caller's ip in trampoline stack")
> > on Jul 15 2021 which is pretty old.
> >
> > I am only able to apply with the --3way merge like "git am --3way".
> > Andrii, is it fine to land the patch like this?
>
> I cannot apply it cleanly if using "git am --reject xx.patch", and it's OK if
> using "git cherry-pick commit_id", so I will rebase and repost it.
Hi Hou, could you repost the patch after rebasing? The dummy testing
struct_ops can be a separate patch targeting the bpf-next tree isntead.
Thanks!
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-09-13 20:38 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-01 8:53 [PATCH bpf] bpf: handle return value of BPF_PROG_TYPE_STRUCT_OPS prog Hou Tao
2021-09-07 11:13 ` Hou Tao
2021-09-08 6:06 ` Martin KaFai Lau
2021-09-08 13:31 ` Hou Tao
2021-09-08 17:19 ` Martin KaFai Lau
2021-09-08 17:27 ` Martin KaFai Lau
2021-09-09 1:45 ` Hou Tao
2021-09-13 20:37 ` Martin KaFai Lau
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).