Handle possible NULL trusted raw_tp arguments
Subsystems (5)
Kumar Kartikeya Dwivedi
maintainercover Handle possible NULL trusted raw_tp arguments
More context is available in [0], but the TLDR; is that the verifier incorrectly assumes that any raw tracepoint argument will always be non-NULL. This means that even when users correctly check possible NULL arguments, the verifier can remove the NULL check due to incorrect knowledge of the NULL-ness of the pointer. Secondly, kernel helpers or kfuncs taking these trusted tracepoint arguments incorrectly assume that all arguments will always be valid non-NULL.
In this set, we mark raw_tp arguments as PTR_MAYBE_NULL on top of PTR_TRUSTED, but special case their behavior when dereferencing them or pointer arithmetic over them is involved. When passing trusted args to helpers or kfuncs, raw_tp programs are permitted to pass possibly NULL pointers in such cases.
Any loads into such maybe NULL trusted PTR_TO_BTF_ID is promoted to a PROBE_MEM load to handle emanating page faults. The verifier will ensure NULL checks on such pointers are preserved and do not lead to dead code elimination.
This new behavior is not applied when ref_obj_id is non-zero, as those pointers do not belong to raw_tp arguments, but instead acquired objects.
Since helpers and kfuncs already require attention for PTR_TO_BTF_ID (non-trusted) pointers, we do not implement any protection for such cases in this patch set, and leave it as future work for an upcoming series.
A selftest is included with this patch set to verify the new behavior, and it crashes the kernel without the first patch.
[0]: https://lore.kernel.org/bpf/CAADnVQLMPPavJQR6JFsi3dtaaLHB816JN4HCV_TFWohJ61D+wQ@mail.gmail.com
Kumar Kartikeya Dwivedi (2):
bpf: Mark raw_tp arguments with PTR_MAYBE_NULL selftests/bpf: Add tests for raw_tp null handling
include/linux/bpf.h | 6 ++ kernel/bpf/btf.c | 5 +- kernel/bpf/verifier.c | 75 +++++++++++++++++-- .../bpf/bpf_testmod/bpf_testmod-events.h | 8 ++ .../selftests/bpf/bpf_testmod/bpf_testmod.c | 2 + .../selftests/bpf/prog_tests/raw_tp_null.c | 25 +++++++ .../testing/selftests/bpf/progs/raw_tp_null.c | 27 +++++++ .../bpf/progs/test_tp_btf_nullable.c | 6 +- 8 files changed, 145 insertions(+), 9 deletions(-) create mode 100644 tools/testing/selftests/bpf/prog_tests/raw_tp_null.c create mode 100644 tools/testing/selftests/bpf/progs/raw_tp_null.c
base-commit: e626a13f6fbb4697f8734333432dca577628d09a
Signature · 1 line
2.43.5
Kumar Kartikeya Dwivedi
maintainerPATCH 1/2 bpf: Mark raw_tp arguments with PTR_MAYBE_NULL
Arguments to a raw tracepoint are tagged as trusted, which carries the semantics that the pointer will be non-NULL. However, in certain cases, a raw tracepoint argument may end up being NULL More context about this issue is available in [0].
Thus, there is a discrepancy between the reality, that raw_tp arguments can actually be NULL, and the verifier's knowledge, that they are never NULL, causing explicit NULL checks to be deleted, and accesses to such pointers potentially crashing the kernel.
To fix this, mark raw_tp arguments as PTR_MAYBE_NULL, and then special case the dereference and pointer arithmetic to permit it, and allow passing them into helpers/kfuncs; these exceptions are made for raw_tp programs only. Ensure that we don't do this when ref_obj_id > 0, as in that case this is an acquired object and doesn't need such adjustment.
The reason we do mask_raw_tp_trusted_reg logic is because other will recheck in places whether the register is a trusted_reg, and then consider our register as untrusted when detecting the presence of the PTR_MAYBE_NULL flag.
To allow safe dereference, we enable PROBE_MEM marking when we see loads into trusted pointers with PTR_MAYBE_NULL.
While trusted raw_tp arguments can also be passed into helpers or kfuncs where such broken assumption may cause issues, a future patch set will tackle their case separately, as PTR_TO_BTF_ID (without PTR_TRUSTED) can already be passed into helpers and causes similar problems. Thus, they are left alone for now.
It is possible that these checks also permit passing non-raw_tp args that are trusted PTR_TO_BTF_ID with null marking. In such a case, allowing dereference when pointer is NULL expands allowed behavior, so won't regress existing programs, and the case of passing these into helpers is the same as above and will be dealt with later.
Also update the failure case in tp_btf_nullable selftest to capture the new behavior, as the verifier will no longer cause an error when directly dereference a raw tracepoint argument marked as __nullable.
[0]: https://lore.kernel.org/bpf/[email protected]
--- include/linux/bpf.h | 6 ++ kernel/bpf/btf.c | 5 +- kernel/bpf/verifier.c | 75 +++++++++++++++++-- .../bpf/progs/test_tp_btf_nullable.c | 6 +- 4 files changed, 83 insertions(+), 9 deletions(-)
include/linux/bpf.h +6 -0
@@ -3495,4 +3495,10 @@ static inline bool bpf_is_subprog(const struct bpf_prog *prog) return prog->aux->func_idx != 0; } +static inline bool bpf_prog_is_raw_tp (const struct bpf_prog *prog) +{ +return prog->type ==BPF_PROG_TYPE_TRACING && + prog->expected_attach_type ==BPF_TRACE_RAW_TP ; +} +#endif /* _LINUX_BPF_H */
kernel/bpf/btf.c +4 -1
@@ -6588,7 +6588,10 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,if (prog_args_trusted (prog)) info->reg_type |=PTR_TRUSTED ; -if (btf_param_match_suffix (btf, &args[arg],"__nullable" )) +/* Raw tracepoint arguments always get marked as maybe NULL */ +if (bpf_prog_is_raw_tp (prog)) + info->reg_type |=PTR_MAYBE_NULL ; +else if (btf_param_match_suffix (btf, &args[arg],"__nullable" )) info->reg_type |=PTR_MAYBE_NULL ;if (tgt_prog) {
kernel/bpf/verifier.c +68 -7
219 lines not on this page · raw message
tools/testing/selftests/bpf/progs/test_tp_btf_nullable.c +5 -1
@@ -7,7 +7,11 @@#include "bpf_misc.h" SEC ("tp_btf/bpf_testmod_test_nullable_bare" ) -__failure __msg("R1 invalid mem access 'trusted_ptr_or_null_'") +/* This used to be a failure test, but raw_tp nullable arguments can now +* directly be dereferenced, whether they have nullable annotation or not, +* and don't need to be explicitly checked. + */ +__success int BPF_PROG(handle_tp_btf_nullable_bare1, struct bpf_testmod_test_read_ctx *nullable_ctx) {return nullable_ctx->len;
Signature · 1 line
2.43.5
Re: [PATCH bpf-next v1 1/2] bpf: Mark raw_tp arguments with PTR_MAYBE_NULL
Quoted 53 lines · Kumar Kartikeya Dwivedi
Arguments to a raw tracepoint are tagged as trusted, which carries the semantics that the pointer will be non-NULL. However, in certain cases, a raw tracepoint argument may end up being NULL More context about this issue is available in [0].
Thus, there is a discrepancy between the reality, that
raw_tparguments can actually be NULL, and the verifier's knowledge, that they are never NULL, causing explicit NULL checks to be deleted, and accesses to such pointers potentially crashing the kernel.To fix this, mark
raw_tparguments asPTR_MAYBE_NULL, and then special case the dereference and pointer arithmetic to permit it, and allow passing them into helpers/kfuncs; these exceptions are made forraw_tpprograms only. Ensure that we don't do this whenref_obj_id> 0, as in that case this is an acquired object and doesn't need such adjustment.The reason we do
mask_raw_tp_trusted_reglogic is because other will recheck in places whether the register is atrusted_reg, and then consider our register as untrusted when detecting the presence of thePTR_MAYBE_NULLflag.To allow safe dereference, we enable
PROBE_MEMmarking when we see loads into trusted pointers withPTR_MAYBE_NULL.While trusted
raw_tparguments can also be passed into helpers or kfuncs where such broken assumption may cause issues, a future patch set will tackle their case separately, asPTR_TO_BTF_ID(withoutPTR_TRUSTED) can already be passed into helpers and causes similar problems. Thus, they are left alone for now.It is possible that these checks also permit passing non-raw_tp args that are trusted
PTR_TO_BTF_IDwith null marking. In such a case, allowing dereference when pointer is NULL expands allowed behavior, so won't regress existing programs, and the case of passing these into helpers is the same as above and will be dealt with later.Also update the failure case in
tp_btf_nullableselftest to capture the new behavior, as the verifier will no longer cause an error when directly dereference a raw tracepoint argument marked as__nullable.[0]: https://lore.kernel.org/bpf/[email protected]
Reported-by Juri Lelli <[email protected]>Fixes 3f00c5239344 ("bpf: Allow trusted pointers to be passed to KF_TRUSTED_ARGS kfuncs")Signed-off-by Kumar Kartikeya Dwivedi <[email protected]>--- include/linux/bpf.h | 6 ++ kernel/bpf/btf.c | 5 +- kernel/bpf/verifier.c | 75 +++++++++++++++++-- .../bpf/progs/test_tp_btf_nullable.c | 6 +- 4 files changed, 83 insertions(+), 9 deletions(-)
[...]
Quoted 9 lines
kernel/bpf/verifier.c +4
-1@@ -6693,7 +6709,21 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,if (ret <0 )return ret; - +/* For raw_tp progs, we allow dereference of PTR_MAYBE_NULL +* trusted PTR_TO_BTF_ID, these are the ones that are possibly +* arguments to the raw_tp. Since internal checks in for trusted +* reg in check_ptr_to_btf_access would consider PTR_MAYBE_NULL
kernel/bpf/verifier.c static int check_ptr_to_btf_access(struct bpf_verifier_env *env, · line 6725
Quoted 10 lines
kernel/bpf/verifier.c +10
-0+* modifier as problematic, mask it out temporarily for the +* check. Don't apply this to pointers with ref_obj_id > 0, as +* those won't be raw_tp args. +* +* We may end up applying this relaxation to other trusted +* PTR_TO_BTF_ID with maybe null flag, since we cannot +* distinguish PTR_MAYBE_NULL tagged for arguments vs normal +* tagging, but that should expand allowed behavior, and not +* cause regression for existing behavior. +*/
Yeah, I'm not sure why this has to be raw tp-specific?.. What's wrong with the same behavior for BPF iterator programs, for example?
It seems nicer if we can avoid this temporary masking and instead support this as a generic functionality? Or are there complications?
Quoted 19 lines
kernel/bpf/verifier.c +7
-1+ mask =mask_raw_tp_reg (env, reg);if (ret !=PTR_TO_BTF_ID ) {/* just mark; */ @@ -6754,8 +6784,13 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,clear_trusted_flags (&flag); } -if (atype ==BPF_READ && value_regno >=0 ) +if (atype ==BPF_READ && value_regno >=0 ) {mark_btf_ld_reg (env, regs, value_regno, ret, reg->btf, btf_id, flag); +/* We've assigned a new type to regno, so don't undo masking. */ +if (regno == value_regno) + mask = false; + } +unmask_raw_tp_reg (reg, mask);return 0 ; }
[...]
Re: [PATCH bpf-next v1 1/2] bpf: Mark raw_tp arguments with PTR_MAYBE_NULL
Quoted 35 lines · Andrii Nakryiko
Quoted 10 lines
(quoted diff) +4
-1@@ -6693,7 +6709,21 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, if (ret < 0) return ret; - + /* For raw_tp progs, we allow dereference of PTR_MAYBE_NULL + * trusted PTR_TO_BTF_ID, these are the ones that are possibly + * arguments to the raw_tp. Since internal checks in for trusted + * reg in check_ptr_to_btf_access would consider PTR_MAYBE_NULLReview static int check_ptr_to_btf_access(struct bpf_verifier_env *env, · line 6725
Quoted 10 lines
(quoted diff) +10
-0+ * modifier as problematic, mask it out temporarily for the + * check. Don't apply this to pointers with ref_obj_id > 0, as + * those won't be raw_tp args. + * + * We may end up applying this relaxation to other trusted + * PTR_TO_BTF_ID with maybe null flag, since we cannot + * distinguish PTR_MAYBE_NULL tagged for arguments vs normal + * tagging, but that should expand allowed behavior, and not + * cause regression for existing behavior. + */Yeah, I'm not sure why this has to be raw tp-specific?.. What's wrong with the same behavior for BPF iterator programs, for example?
It seems nicer if we can avoid this temporary masking and instead support this as a generic functionality? Or are there complications?
Quoted 8 lines
(quoted diff) +1
-0+ mask = mask_raw_tp_reg(env, reg); if (ret != PTR_TO_BTF_ID) { /* just mark; */@@ -6754,8 +6784,13 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, clear_trusted_flags(&flag); }
Quoted 8 lines
Review static int check_ptr_to_btf_access(struct bpf_verifier_env *env, · line 6793
Quoted 8 lines
(quoted diff) +6
-1- if (atype == BPF_READ && value_regno >= 0) + if (atype == BPF_READ && value_regno >= 0) { mark_btf_ld_reg(env, regs, value_regno, ret, reg->btf, btf_id, flag); + /* We've assigned a new type to regno, so don't undo masking. */ + if (regno == value_regno) + mask = false; + } + unmask_raw_tp_reg(reg, mask);
Kumar,
I chatted with Andrii offline. All other cases of mask/unmask should probably stay raw_tp specific, but it seems we can make this particular case to be generic.
Something like the following:
kernel/bpf/verifier.c +0 -0
@@ -6703,7 +6703,11 @@ static int check_ptr_to_btf_access(struct
bpf_verifier_env *env,
*/
flag = PTR_UNTRUSTED;+ } else if (reg->type == (PTR_TO_BTF_ID | PTR_TRUSTED |PTR_MAYBE_NULL)) {
+ flag |= PTR_MAYBE_NULL;
+ goto trusted;
} else if (is_trusted_reg(reg) || is_rcu_reg(reg)) {+trusted:
With the idea that trusted_or_null stays that way for all prog types and bpf_iter__task->task deref stays trusted_or_null instead of being downgraded to ptr_to_btf_id without any flags. So progs can do few less != null checks.
Need to think it through.
Kumar Kartikeya Dwivedi
maintainerRe: [PATCH bpf-next v1 1/2] bpf: Mark raw_tp arguments with PTR_MAYBE_NULL
Quoted 27 lines · Alexei Starovoitov
Quoted 24 lines · Andrii Nakryiko
Quoted 10 lines
(quoted diff) +4
-1@@ -6693,7 +6709,21 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, if (ret < 0) return ret; - + /* For raw_tp progs, we allow dereference of PTR_MAYBE_NULL + * trusted PTR_TO_BTF_ID, these are the ones that are possibly + * arguments to the raw_tp. Since internal checks in for trusted + * reg in check_ptr_to_btf_access would consider PTR_MAYBE_NULLReview static int check_ptr_to_btf_access(struct bpf_verifier_env *env, · line 6725
Quoted 10 lines
(quoted diff) +10
-0+ * modifier as problematic, mask it out temporarily for the + * check. Don't apply this to pointers with ref_obj_id > 0, as + * those won't be raw_tp args. + * + * We may end up applying this relaxation to other trusted + * PTR_TO_BTF_ID with maybe null flag, since we cannot + * distinguish PTR_MAYBE_NULL tagged for arguments vs normal + * tagging, but that should expand allowed behavior, and not + * cause regression for existing behavior. + */Yeah, I'm not sure why this has to be raw tp-specific?.. What's wrong with the same behavior for BPF iterator programs, for example?
Quoted 3 lines
Quoted 3 lines
It seems nicer if we can avoid this temporary masking and instead support this as a generic functionality? Or are there complications?
We _can_ do this for all programs. The thought process here was to leave existing raw_tp programs unbroken if possible if we're marking their arguments as PTR_MAYBE_NULL, since most of them won't be performing any NULL checks at all.
Quoted 40 lines
Quoted 8 lines
Quoted 8 lines
(quoted diff) +1
-0+ mask = mask_raw_tp_reg(env, reg); if (ret != PTR_TO_BTF_ID) { /* just mark; */@@ -6754,8 +6784,13 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, clear_trusted_flags(&flag); }Quoted 8 lines
Review static int check_ptr_to_btf_access(struct bpf_verifier_env *env, · line 6793
Quoted 8 lines
(quoted diff) +6
-1- if (atype == BPF_READ && value_regno >= 0) + if (atype == BPF_READ && value_regno >= 0) { mark_btf_ld_reg(env, regs, value_regno, ret, reg->btf, btf_id, flag); + /* We've assigned a new type to regno, so don't undo masking. */ + if (regno == value_regno) + mask = false; + } + unmask_raw_tp_reg(reg, mask);Kumar,
I chatted with Andrii offline. All other cases of mask/unmask should probably stay
raw_tpspecific, but it seems we can make this particular case to be generic.
Something like the following:kernel/bpf/verifier.c +4
-0@@ -6703,7 +6703,11 @@ static int check_ptr_to_btf_access(structbpf_verifier_env *env, */ flag =PTR_UNTRUSTED ; + }else if (reg->type == (PTR_TO_BTF_ID |PTR_TRUSTED |PTR_MAYBE_NULL )) { + flag |=PTR_MAYBE_NULL ; + goto trusted; }else if (is_trusted_reg (reg) ||is_rcu_reg (reg)) { +trusted:
Quoted 5 lines
With the idea that
trusted_or_nullstays that way for all prog types andbpf_iter__task->taskderef staystrusted_or_nullinstead of being downgraded toptr_to_btf_idwithout any flags. So progs can do few less != null checks.
Need to think it through.
Ok. But don't allow passing such pointers into helpers, right?
We do that for raw_tp to preserve compat, but it would just exacerbate the issue if we start doing it everywhere.
So it's just that dereferencing a _or_null pointer becomes an ok thing to do? Let me mull over this for a bit.
I'm not sure whether not doing the NULL check is better or worse though. On one hand everything will work without checking for NULL, on the other hand people may also assume the verifier isn't complaining because the pointer is valid, and then they read data from the pointer which always ends up being zero, meaning different things for different kinds of fields.
Just thinking out loud, but one of the other concerns would be that we're encouraging people not to do these NULL checks, which means a potential page fault penalty everytime that pointer _is_ NULL, instead of a simple branch, which would certainly be a bit expensive. If this becomes the common case, I think the prog execution latency penalty will be big. It is something to consider.
Kumar Kartikeya Dwivedi
maintainerRe: [PATCH bpf-next v1 1/2] bpf: Mark raw_tp arguments with PTR_MAYBE_NULL
Quoted 98 lines · Kumar Kartikeya Dwivedi
Quoted 27 lines · Alexei Starovoitov
Quoted 24 lines · Andrii Nakryiko
Quoted 10 lines
(quoted diff) +4
-1@@ -6693,7 +6709,21 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, if (ret < 0) return ret; - + /* For raw_tp progs, we allow dereference of PTR_MAYBE_NULL + * trusted PTR_TO_BTF_ID, these are the ones that are possibly + * arguments to the raw_tp. Since internal checks in for trusted + * reg in check_ptr_to_btf_access would consider PTR_MAYBE_NULLReview static int check_ptr_to_btf_access(struct bpf_verifier_env *env, · line 6725
Quoted 10 lines
(quoted diff) +10
-0+ * modifier as problematic, mask it out temporarily for the + * check. Don't apply this to pointers with ref_obj_id > 0, as + * those won't be raw_tp args. + * + * We may end up applying this relaxation to other trusted + * PTR_TO_BTF_ID with maybe null flag, since we cannot + * distinguish PTR_MAYBE_NULL tagged for arguments vs normal + * tagging, but that should expand allowed behavior, and not + * cause regression for existing behavior. + */Yeah, I'm not sure why this has to be raw tp-specific?.. What's wrong with the same behavior for BPF iterator programs, for example?
Quoted 3 lines
Quoted 3 lines
It seems nicer if we can avoid this temporary masking and instead support this as a generic functionality? Or are there complications?
We _can_ do this for all programs. The thought process here was to leave existing
raw_tpprograms unbroken if possible if we're marking their arguments asPTR_MAYBE_NULL, since most of them won't be performing any NULL checks at all.Quoted 40 lines
Quoted 8 lines
Quoted 8 lines
(quoted diff) +1
-0+ mask = mask_raw_tp_reg(env, reg); if (ret != PTR_TO_BTF_ID) { /* just mark; */@@ -6754,8 +6784,13 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, clear_trusted_flags(&flag); }Quoted 8 lines
Review static int check_ptr_to_btf_access(struct bpf_verifier_env *env, · line 6793
Quoted 8 lines
(quoted diff) +6
-1- if (atype == BPF_READ && value_regno >= 0) + if (atype == BPF_READ && value_regno >= 0) { mark_btf_ld_reg(env, regs, value_regno, ret, reg->btf, btf_id, flag); + /* We've assigned a new type to regno, so don't undo masking. */ + if (regno == value_regno) + mask = false; + } + unmask_raw_tp_reg(reg, mask);Kumar,
I chatted with Andrii offline. All other cases of mask/unmask should probably stay
raw_tpspecific, but it seems we can make this particular case to be generic.
Something like the following:kernel/bpf/verifier.c +4
-0@@ -6703,7 +6703,11 @@ static int check_ptr_to_btf_access(structbpf_verifier_env *env, */ flag =PTR_UNTRUSTED ; + }else if (reg->type == (PTR_TO_BTF_ID |PTR_TRUSTED |PTR_MAYBE_NULL )) { + flag |=PTR_MAYBE_NULL ; + goto trusted; }else if (is_trusted_reg (reg) ||is_rcu_reg (reg)) { +trusted:Quoted 5 lines
With the idea that
trusted_or_nullstays that way for all prog types andbpf_iter__task->taskderef staystrusted_or_nullinstead of being downgraded toptr_to_btf_idwithout any flags. So progs can do few less != null checks.
Need to think it through.Ok. But don't allow passing such pointers into helpers, right?
We do that forraw_tpto preserve compat, but it would just exacerbate the issue if we start doing it everywhere.
So it's just that dereferencing a_or_nullpointer becomes an ok thing to do? Let me mull over this for a bit.I'm not sure whether not doing the NULL check is better or worse though. On one hand everything will work without checking for NULL, on the other hand people may also assume the verifier isn't complaining because the pointer is valid, and then they read data from the pointer which always ends up being zero, meaning different things for different kinds of fields.
Quoted 6 lines
Just thinking out loud, but one of the other concerns would be that we're encouraging people not to do these NULL checks, which means a potential page fault penalty everytime that pointer _is_ NULL, instead of a simple branch, which would certainly be a bit expensive. If this becomes the common case, I think the prog execution latency penalty will be big. It is something to consider.
Ah, no, my bad, this won't be a problem now, as the JIT does emit a branch to check for kernel addresses, but it probably will be if
https://lore.kernel.org/bpf/[email protected]/
gets accepted.
Kumar Kartikeya Dwivedi
maintainerRe: [PATCH bpf-next v1 1/2] bpf: Mark raw_tp arguments with PTR_MAYBE_NULL
Quoted 107 lines · Kumar Kartikeya Dwivedi
Quoted 98 lines · Kumar Kartikeya Dwivedi
Quoted 27 lines · Alexei Starovoitov
Quoted 24 lines · Andrii Nakryiko
Quoted 10 lines
(quoted diff) +4
-1@@ -6693,7 +6709,21 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, if (ret < 0) return ret; - + /* For raw_tp progs, we allow dereference of PTR_MAYBE_NULL + * trusted PTR_TO_BTF_ID, these are the ones that are possibly + * arguments to the raw_tp. Since internal checks in for trusted + * reg in check_ptr_to_btf_access would consider PTR_MAYBE_NULLReview static int check_ptr_to_btf_access(struct bpf_verifier_env *env, · line 6725
Quoted 10 lines
(quoted diff) +10
-0+ * modifier as problematic, mask it out temporarily for the + * check. Don't apply this to pointers with ref_obj_id > 0, as + * those won't be raw_tp args. + * + * We may end up applying this relaxation to other trusted + * PTR_TO_BTF_ID with maybe null flag, since we cannot + * distinguish PTR_MAYBE_NULL tagged for arguments vs normal + * tagging, but that should expand allowed behavior, and not + * cause regression for existing behavior. + */Yeah, I'm not sure why this has to be raw tp-specific?.. What's wrong with the same behavior for BPF iterator programs, for example?
Quoted 3 lines
Quoted 3 lines
It seems nicer if we can avoid this temporary masking and instead support this as a generic functionality? Or are there complications?
We _can_ do this for all programs. The thought process here was to leave existing
raw_tpprograms unbroken if possible if we're marking their arguments asPTR_MAYBE_NULL, since most of them won't be performing any NULL checks at all.Quoted 40 lines
Quoted 8 lines
Quoted 8 lines
(quoted diff) +1
-0+ mask = mask_raw_tp_reg(env, reg); if (ret != PTR_TO_BTF_ID) { /* just mark; */@@ -6754,8 +6784,13 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, clear_trusted_flags(&flag); }Quoted 8 lines
Review static int check_ptr_to_btf_access(struct bpf_verifier_env *env, · line 6793
Quoted 8 lines
(quoted diff) +6
-1- if (atype == BPF_READ && value_regno >= 0) + if (atype == BPF_READ && value_regno >= 0) { mark_btf_ld_reg(env, regs, value_regno, ret, reg->btf, btf_id, flag); + /* We've assigned a new type to regno, so don't undo masking. */ + if (regno == value_regno) + mask = false; + } + unmask_raw_tp_reg(reg, mask);Kumar,
I chatted with Andrii offline. All other cases of mask/unmask should probably stay
raw_tpspecific, but it seems we can make this particular case to be generic.
Something like the following:kernel/bpf/verifier.c +4
-0@@ -6703,7 +6703,11 @@ static int check_ptr_to_btf_access(structbpf_verifier_env *env, */ flag =PTR_UNTRUSTED ; + }else if (reg->type == (PTR_TO_BTF_ID |PTR_TRUSTED |PTR_MAYBE_NULL )) { + flag |=PTR_MAYBE_NULL ; + goto trusted; }else if (is_trusted_reg (reg) ||is_rcu_reg (reg)) { +trusted:Quoted 5 lines
With the idea that
trusted_or_nullstays that way for all prog types andbpf_iter__task->taskderef staystrusted_or_nullinstead of being downgraded toptr_to_btf_idwithout any flags. So progs can do few less != null checks.
Need to think it through.Ok. But don't allow passing such pointers into helpers, right?
We do that forraw_tpto preserve compat, but it would just exacerbate the issue if we start doing it everywhere.
So it's just that dereferencing a_or_nullpointer becomes an ok thing to do? Let me mull over this for a bit.I'm not sure whether not doing the NULL check is better or worse though. On one hand everything will work without checking for NULL, on the other hand people may also assume the verifier isn't complaining because the pointer is valid, and then they read data from the pointer which always ends up being zero, meaning different things for different kinds of fields.
Quoted 6 lines
Just thinking out loud, but one of the other concerns would be that we're encouraging people not to do these NULL checks, which means a potential page fault penalty everytime that pointer _is_ NULL, instead of a simple branch, which would certainly be a bit expensive. If this becomes the common case, I think the prog execution latency penalty will be big. It is something to consider.
Quoted 4 lines
Ah, no, my bad, this won't be a problem now, as the JIT does emit a branch to check for kernel addresses, but it probably will be if
https://lore.kernel.org/bpf/[email protected]/
gets accepted.
I applied this and tried to find out the time it takes to dereference a NULL pointer using bpf_ktime_get_ns
With the patch above: time=3345 ns
Without (bpf-next right now): time=170 ns
So I guess that means I should probably drop the patch above if we decide to allow dereferencing NULL for all programs.
Re: [PATCH bpf-next v1 1/2] bpf: Mark raw_tp arguments with PTR_MAYBE_NULL
Quoted 114 lines · Kumar Kartikeya Dwivedi
Quoted 107 lines · Kumar Kartikeya Dwivedi
Quoted 98 lines · Kumar Kartikeya Dwivedi
Quoted 27 lines · Alexei Starovoitov
Quoted 24 lines · Andrii Nakryiko
Quoted 10 lines
(quoted diff) +4
-1@@ -6693,7 +6709,21 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, if (ret < 0) return ret; - + /* For raw_tp progs, we allow dereference of PTR_MAYBE_NULL + * trusted PTR_TO_BTF_ID, these are the ones that are possibly + * arguments to the raw_tp. Since internal checks in for trusted + * reg in check_ptr_to_btf_access would consider PTR_MAYBE_NULLReview static int check_ptr_to_btf_access(struct bpf_verifier_env *env, · line 6725
Quoted 10 lines
(quoted diff) +10
-0+ * modifier as problematic, mask it out temporarily for the + * check. Don't apply this to pointers with ref_obj_id > 0, as + * those won't be raw_tp args. + * + * We may end up applying this relaxation to other trusted + * PTR_TO_BTF_ID with maybe null flag, since we cannot + * distinguish PTR_MAYBE_NULL tagged for arguments vs normal + * tagging, but that should expand allowed behavior, and not + * cause regression for existing behavior. + */Yeah, I'm not sure why this has to be raw tp-specific?.. What's wrong with the same behavior for BPF iterator programs, for example?
Quoted 3 lines
Quoted 3 lines
It seems nicer if we can avoid this temporary masking and instead support this as a generic functionality? Or are there complications?
We _can_ do this for all programs. The thought process here was to leave existing
raw_tpprograms unbroken if possible if we're marking their arguments asPTR_MAYBE_NULL, since most of them won't be performing any NULL checks at all.Quoted 40 lines
Quoted 8 lines
Quoted 8 lines
(quoted diff) +1
-0+ mask = mask_raw_tp_reg(env, reg); if (ret != PTR_TO_BTF_ID) { /* just mark; */@@ -6754,8 +6784,13 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, clear_trusted_flags(&flag); }Quoted 8 lines
Review static int check_ptr_to_btf_access(struct bpf_verifier_env *env, · line 6793
Quoted 8 lines
(quoted diff) +6
-1- if (atype == BPF_READ && value_regno >= 0) + if (atype == BPF_READ && value_regno >= 0) { mark_btf_ld_reg(env, regs, value_regno, ret, reg->btf, btf_id, flag); + /* We've assigned a new type to regno, so don't undo masking. */ + if (regno == value_regno) + mask = false; + } + unmask_raw_tp_reg(reg, mask);Kumar,
I chatted with Andrii offline. All other cases of mask/unmask should probably stay
raw_tpspecific, but it seems we can make this particular case to be generic.
Something like the following:kernel/bpf/verifier.c +4
-0@@ -6703,7 +6703,11 @@ static int check_ptr_to_btf_access(structbpf_verifier_env *env, */ flag =PTR_UNTRUSTED ; + }else if (reg->type == (PTR_TO_BTF_ID |PTR_TRUSTED |PTR_MAYBE_NULL )) { + flag |=PTR_MAYBE_NULL ; + goto trusted; }else if (is_trusted_reg (reg) ||is_rcu_reg (reg)) { +trusted:Quoted 5 lines
With the idea that
trusted_or_nullstays that way for all prog types andbpf_iter__task->taskderef staystrusted_or_nullinstead of being downgraded toptr_to_btf_idwithout any flags. So progs can do few less != null checks.
Need to think it through.Ok. But don't allow passing such pointers into helpers, right?
We do that forraw_tpto preserve compat, but it would just exacerbate the issue if we start doing it everywhere.
So it's just that dereferencing a_or_nullpointer becomes an ok thing to do? Let me mull over this for a bit.I'm not sure whether not doing the NULL check is better or worse though. On one hand everything will work without checking for NULL, on the other hand people may also assume the verifier isn't complaining because the pointer is valid, and then they read data from the pointer which always ends up being zero, meaning different things for different kinds of fields.
Quoted 6 lines
Just thinking out loud, but one of the other concerns would be that we're encouraging people not to do these NULL checks, which means a potential page fault penalty everytime that pointer _is_ NULL, instead of a simple branch, which would certainly be a bit expensive. If this becomes the common case, I think the prog execution latency penalty will be big. It is something to consider.
Quoted 4 lines
Ah, no, my bad, this won't be a problem now, as the JIT does emit a branch to check for kernel addresses, but it probably will be if
https://lore.kernel.org/bpf/[email protected]/
gets accepted.
Quoted 7 lines
I applied this and tried to find out the time it takes to dereference a NULL pointer using
bpf_ktime_get_ns
With the patch above: time=3345 ns
Without (bpf-next right now): time=170 nsSo I guess that means I should probably drop the patch above if we decide to allow dereferencing NULL for all programs.
Your concerns are valid.
Accepting deref of trusted_or_null generically will cause these issues long term. It's indeed better to make programmers add explicit !=NULL to their programs.
So scratch my earlier suggestion. Let's keep this patch as-is with special hack for raw_tp only that we will hopefully resolve later either with __nullable suffix or compiler detection of nullability. The latter we discussed briefly with Eduard.
It's doable to teach llvm to emit btf_tag to aid the verifier. gcc may catch up eventually.
Re: [PATCH bpf-next v1 1/2] bpf: Mark raw_tp arguments with PTR_MAYBE_NULL
Quoted 121 lines · Alexei Starovoitov
Quoted 119 lines · Kumar Kartikeya Dwivedi
Quoted 107 lines · Kumar Kartikeya Dwivedi
Quoted 98 lines · Kumar Kartikeya Dwivedi
Quoted 27 lines · Alexei Starovoitov
Quoted 24 lines · Andrii Nakryiko
Quoted 10 lines
(quoted diff) +4
-1@@ -6693,7 +6709,21 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, if (ret < 0) return ret; - + /* For raw_tp progs, we allow dereference of PTR_MAYBE_NULL + * trusted PTR_TO_BTF_ID, these are the ones that are possibly + * arguments to the raw_tp. Since internal checks in for trusted + * reg in check_ptr_to_btf_access would consider PTR_MAYBE_NULLReview static int check_ptr_to_btf_access(struct bpf_verifier_env *env, · line 6725
Quoted 10 lines
(quoted diff) +10
-0+ * modifier as problematic, mask it out temporarily for the + * check. Don't apply this to pointers with ref_obj_id > 0, as + * those won't be raw_tp args. + * + * We may end up applying this relaxation to other trusted + * PTR_TO_BTF_ID with maybe null flag, since we cannot + * distinguish PTR_MAYBE_NULL tagged for arguments vs normal + * tagging, but that should expand allowed behavior, and not + * cause regression for existing behavior. + */Yeah, I'm not sure why this has to be raw tp-specific?.. What's wrong with the same behavior for BPF iterator programs, for example?
Quoted 3 lines
Quoted 3 lines
It seems nicer if we can avoid this temporary masking and instead support this as a generic functionality? Or are there complications?
We _can_ do this for all programs. The thought process here was to leave existing
raw_tpprograms unbroken if possible if we're marking their arguments asPTR_MAYBE_NULL, since most of them won't be performing any NULL checks at all.Quoted 40 lines
Quoted 8 lines
Quoted 8 lines
(quoted diff) +1
-0+ mask = mask_raw_tp_reg(env, reg); if (ret != PTR_TO_BTF_ID) { /* just mark; */@@ -6754,8 +6784,13 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, clear_trusted_flags(&flag); }Quoted 8 lines
Review static int check_ptr_to_btf_access(struct bpf_verifier_env *env, · line 6793
Quoted 8 lines
(quoted diff) +6
-1- if (atype == BPF_READ && value_regno >= 0) + if (atype == BPF_READ && value_regno >= 0) { mark_btf_ld_reg(env, regs, value_regno, ret, reg->btf, btf_id, flag); + /* We've assigned a new type to regno, so don't undo masking. */ + if (regno == value_regno) + mask = false; + } + unmask_raw_tp_reg(reg, mask);Kumar,
I chatted with Andrii offline. All other cases of mask/unmask should probably stay
raw_tpspecific, but it seems we can make this particular case to be generic.
Something like the following:kernel/bpf/verifier.c +4
-0@@ -6703,7 +6703,11 @@ static int check_ptr_to_btf_access(structbpf_verifier_env *env, */ flag =PTR_UNTRUSTED ; + }else if (reg->type == (PTR_TO_BTF_ID |PTR_TRUSTED |PTR_MAYBE_NULL )) { + flag |=PTR_MAYBE_NULL ; + goto trusted; }else if (is_trusted_reg (reg) ||is_rcu_reg (reg)) { +trusted:Quoted 5 lines
With the idea that
trusted_or_nullstays that way for all prog types andbpf_iter__task->taskderef staystrusted_or_nullinstead of being downgraded toptr_to_btf_idwithout any flags. So progs can do few less != null checks.
Need to think it through.Ok. But don't allow passing such pointers into helpers, right?
We do that forraw_tpto preserve compat, but it would just exacerbate the issue if we start doing it everywhere.
So it's just that dereferencing a_or_nullpointer becomes an ok thing to do? Let me mull over this for a bit.I'm not sure whether not doing the NULL check is better or worse though. On one hand everything will work without checking for NULL, on the other hand people may also assume the verifier isn't complaining because the pointer is valid, and then they read data from the pointer which always ends up being zero, meaning different things for different kinds of fields.
Quoted 6 lines
Just thinking out loud, but one of the other concerns would be that we're encouraging people not to do these NULL checks, which means a potential page fault penalty everytime that pointer _is_ NULL, instead of a simple branch, which would certainly be a bit expensive. If this becomes the common case, I think the prog execution latency penalty will be big. It is something to consider.
Quoted 4 lines
Ah, no, my bad, this won't be a problem now, as the JIT does emit a branch to check for kernel addresses, but it probably will be if
https://lore.kernel.org/bpf/[email protected]/
gets accepted.I applied this and tried to find out the time it takes to dereference a NULL pointer using
bpf_ktime_get_ns
With the patch above: time=3345 ns
Without (bpf-next right now): time=170 ns
Quoted 7 lines
Quoted 2 lines
So I guess that means I should probably drop the patch above if we decide to allow dereferencing NULL for all programs.
Your concerns are valid.
Accepting deref oftrusted_or_nullgenerically will cause these issues long term. It's indeed better to make programmers add explicit !=NULL to their programs.
Agreed. Not a huge fan of mutating reg->type in place with mask/unmask, but it's fine overall. I think we could have removed half of unmasks because they are done in error cases, at which point it doesn't matter, but that's a minor complaint.
Quoted 6 lines
So scratch my earlier suggestion. Let's keep this patch as-is with special hack for
raw_tponly that we will hopefully resolve later either with__nullablesuffix or compiler detection of nullability. The latter we discussed briefly with Eduard.
It's doable to teach llvm to emitbtf_tagto aid the verifier. gcc may catch up eventually.
Kumar Kartikeya Dwivedi
maintainerPATCH 2/2 selftests/bpf: Add tests for raw_tp null handling
In reply to Kumar Kartikeya DwivediEnsure that trusted PTR_TO_BTF_ID accesses perform PROBE_MEM handling in raw_tp program. Without the previous fix, this selftest crashes the kernel due to a NULL-pointer dereference. Also ensure that dead code elimination does not kick in for checks on the pointer.
--- .../bpf/bpf_testmod/bpf_testmod-events.h | 8 ++++++ .../selftests/bpf/bpf_testmod/bpf_testmod.c | 2 ++ .../selftests/bpf/prog_tests/raw_tp_null.c | 25 +++++++++++++++++ .../testing/selftests/bpf/progs/raw_tp_null.c | 27 +++++++++++++++++++ 4 files changed, 62 insertions(+) create mode 100644 tools/testing/selftests/bpf/prog_tests/raw_tp_null.c create mode 100644 tools/testing/selftests/bpf/progs/raw_tp_null.c
tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h +8 -0
@@ -40,6 +40,14 @@ DECLARE_TRACE(bpf_testmod_test_nullable_bare,TP_ARGS (ctx__nullable) ); +struct sk_buff ; + +DECLARE_TRACE (bpf_testmod_test_raw_tp_null, + TP_PROTO(struct sk_buff *skb), +TP_ARGS (skb) +); + +#undef BPF_TESTMOD_DECLARE_TRACE#ifdef DECLARE_TRACE_WRITABLE #define BPF_TESTMOD_DECLARE_TRACE (call, proto, args, size) \
tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +2 -0
@@ -380,6 +380,8 @@ bpf_testmod_test_read(struct file *file, struct kobject *kobj, (void )bpf_testmod_test_arg_ptr_to_struct (&struct_arg1_2); + (void )trace_bpf_testmod_test_raw_tp_null (NULL ); + struct_arg3 =kmalloc ((sizeof (struct bpf_testmod_struct_arg_3 ) +sizeof (int )),GFP_KERNEL );if (struct_arg3 !=NULL ) {
tools/testing/selftests/bpf/prog_tests/raw_tp_null.c new file +25 -0
@@ -0,0 +1,25 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */ + +#include <test_progs.h> +#include "raw_tp_null.skel.h" + +void test_raw_tp_null (void ) +{ +struct raw_tp_null *skel; + + skel =raw_tp_null__open_and_load (); +if (!ASSERT_OK_PTR (skel,"raw_tp_null__open_and_load" )) +return ; + + skel->bss->tid =gettid (); + +if (!ASSERT_OK (raw_tp_null__attach (skel),"raw_tp_null__attach" )) + goto end; + +ASSERT_OK (trigger_module_test_read (2 ),"trigger testmod read" ); +ASSERT_EQ (skel->bss->i,3 ,"invocations" ); + +end: +raw_tp_null__destroy (skel); +}
tools/testing/selftests/bpf/progs/raw_tp_null.c new file +27 -0
@@ -0,0 +1,27 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */ + +#include <vmlinux.h> +#include <bpf/bpf_tracing.h> + +char _license[] SEC("license") = "GPL"; + +int tid; +int i; + +SEC("tp_btf/bpf_testmod_test_raw_tp_null") +int BPF_PROG (test_raw_tp_null ,struct sk_buff *skb) +{ +if (bpf_get_current_task_btf ()->pid == tid) { + i = i + skb->mark +1 ; + +/* If dead code elimination kicks in, the increment below will +* be removed. For raw_tp programs, we mark input arguments as +* PTR_MAYBE_NULL, so branch prediction should never kick in. +*/ +if (!skb) + i +=2 ; + } + +return 0 ; +}
Signature · 1 line
2.43.5
Re: [PATCH bpf-next v1 2/2] selftests/bpf: Add tests for raw_tp null handling
Quoted 59 lines · Kumar Kartikeya Dwivedi
Ensure that trusted
PTR_TO_BTF_IDaccesses performPROBE_MEMhandling inraw_tpprogram. Without the previous fix, this selftest crashes the kernel due to a NULL-pointer dereference. Also ensure that dead code elimination does not kick in for checks on the pointer.Signed-off-by Kumar Kartikeya Dwivedi <[email protected]>--- .../bpf/bpf_testmod/bpf_testmod-events.h | 8 ++++++ .../selftests/bpf/bpf_testmod/bpf_testmod.c | 2 ++ .../selftests/bpf/prog_tests/raw_tp_null.c | 25 +++++++++++++++++ .../testing/selftests/bpf/progs/raw_tp_null.c | 27 +++++++++++++++++++ 4 files changed, 62 insertions(+) create mode 100644 tools/testing/selftests/bpf/prog_tests/raw_tp_null.c create mode 100644 tools/testing/selftests/bpf/progs/raw_tp_null.ctools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h +8
-0@@ -40,6 +40,14 @@ DECLARE_TRACE(bpf_testmod_test_nullable_bare,TP_ARGS (ctx__nullable) ); +struct sk_buff ; + +DECLARE_TRACE (bpf_testmod_test_raw_tp_null, + TP_PROTO(struct sk_buff *skb), +TP_ARGS (skb) +); + +#undef BPF_TESTMOD_DECLARE_TRACE#ifdef DECLARE_TRACE_WRITABLE #define BPF_TESTMOD_DECLARE_TRACE (call, proto, args, size) \tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +2
-0@@ -380,6 +380,8 @@ bpf_testmod_test_read(struct file *file, struct kobject *kobj, (void )bpf_testmod_test_arg_ptr_to_struct (&struct_arg1_2); + (void )trace_bpf_testmod_test_raw_tp_null (NULL ); + struct_arg3 =kmalloc ((sizeof (struct bpf_testmod_struct_arg_3 ) +sizeof (int )),GFP_KERNEL );if (struct_arg3 !=NULL ) {tools/testing/selftests/bpf/prog_tests/raw_tp_null.c new file +5
-0@@ -0,0 +1,25 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */ + +#include <test_progs.h> +#include "raw_tp_null.skel.h"
tools/testing/selftests/bpf/prog_tests/raw_tp_null.c line 15
Quoted 10 lines
tools/testing/selftests/bpf/prog_tests/raw_tp_null.c new file +10
-0+ +void test_raw_tp_null (void ) +{ +struct raw_tp_null *skel; + + skel =raw_tp_null__open_and_load (); +if (!ASSERT_OK_PTR (skel,"raw_tp_null__open_and_load" )) +return ; + + skel->bss->tid =gettid ();
this is not available everywhere, we just recently had a fix. Other tests call syscall() directly. It might be time to add macro in one of the helpers headers, though. But that can be done as a separate clean up patch outside of this change (there is enough to review and discuss)
Quoted 21 lines
tools/testing/selftests/bpf/prog_tests/raw_tp_null.c new file +10
-0+ +if (!ASSERT_OK (raw_tp_null__attach (skel),"raw_tp_null__attach" )) + goto end; + +ASSERT_OK (trigger_module_test_read (2 ),"trigger testmod read" ); +ASSERT_EQ (skel->bss->i,3 ,"invocations" ); + +end: +raw_tp_null__destroy (skel); +}tools/testing/selftests/bpf/progs/raw_tp_null.c new file +5
-0@@ -0,0 +1,27 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */ + +#include <vmlinux.h> +#include <bpf/bpf_tracing.h>
tools/testing/selftests/bpf/progs/raw_tp_null.c line 15
Quoted 10 lines
tools/testing/selftests/bpf/progs/raw_tp_null.c new file +10
-0+ +char _license[] SEC("license") = "GPL"; + +int tid; +int i; + +SEC("tp_btf/bpf_testmod_test_raw_tp_null") +int BPF_PROG (test_raw_tp_null ,struct sk_buff *skb) +{ +if (bpf_get_current_task_btf ()->pid == tid) {
nit: avoid unnecessary nesting. Check condition and return early. Also seems nicer to have task_struct local variable for this, tbh:
struct task_struct *t = bpf_get_current_task_btf();
if (t->pid != tid)
return 0;
/* the rest follows, unnested */
Quoted 15 lines
tools/testing/selftests/bpf/progs/raw_tp_null.c new file +12
-1+ i = i + skb->mark +1 ; + +/* If dead code elimination kicks in, the increment below will +* be removed. For raw_tp programs, we mark input arguments as +* PTR_MAYBE_NULL, so branch prediction should never kick in. +*/ +if (!skb) + i +=2 ; + } + +return 0 ; +} --2.43.5
Kumar Kartikeya Dwivedi
maintainerRe: [PATCH bpf-next v1 2/2] selftests/bpf: Add tests for raw_tp null handling
Quoted 70 lines · Andrii Nakryiko
Quoted 67 lines · Kumar Kartikeya Dwivedi
Ensure that trusted
PTR_TO_BTF_IDaccesses performPROBE_MEMhandling inraw_tpprogram. Without the previous fix, this selftest crashes the kernel due to a NULL-pointer dereference. Also ensure that dead code elimination does not kick in for checks on the pointer.Signed-off-by Kumar Kartikeya Dwivedi <[email protected]>--- .../bpf/bpf_testmod/bpf_testmod-events.h | 8 ++++++ .../selftests/bpf/bpf_testmod/bpf_testmod.c | 2 ++ .../selftests/bpf/prog_tests/raw_tp_null.c | 25 +++++++++++++++++ .../testing/selftests/bpf/progs/raw_tp_null.c | 27 +++++++++++++++++++ 4 files changed, 62 insertions(+) create mode 100644 tools/testing/selftests/bpf/prog_tests/raw_tp_null.c create mode 100644 tools/testing/selftests/bpf/progs/raw_tp_null.ctools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h +8
-0@@ -40,6 +40,14 @@ DECLARE_TRACE(bpf_testmod_test_nullable_bare,TP_ARGS (ctx__nullable) ); +struct sk_buff ; + +DECLARE_TRACE (bpf_testmod_test_raw_tp_null, + TP_PROTO(struct sk_buff *skb), +TP_ARGS (skb) +); + +#undef BPF_TESTMOD_DECLARE_TRACE#ifdef DECLARE_TRACE_WRITABLE #define BPF_TESTMOD_DECLARE_TRACE (call, proto, args, size) \tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +2
-0@@ -380,6 +380,8 @@ bpf_testmod_test_read(struct file *file, struct kobject *kobj, (void )bpf_testmod_test_arg_ptr_to_struct (&struct_arg1_2); + (void )trace_bpf_testmod_test_raw_tp_null (NULL ); + struct_arg3 =kmalloc ((sizeof (struct bpf_testmod_struct_arg_3 ) +sizeof (int )),GFP_KERNEL );if (struct_arg3 !=NULL ) {tools/testing/selftests/bpf/prog_tests/raw_tp_null.c new file +13
-0@@ -0,0 +1,25 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */ + +#include <test_progs.h> +#include "raw_tp_null.skel.h" + +void test_raw_tp_null (void ) +{ +struct raw_tp_null *skel; + + skel =raw_tp_null__open_and_load (); +if (!ASSERT_OK_PTR (skel,"raw_tp_null__open_and_load" )) +return ;
Quoted 8 lines
tools/testing/selftests/bpf/prog_tests/raw_tp_null.c line 15
Quoted 2 lines
tools/testing/selftests/bpf/prog_tests/raw_tp_null.c new file +2
-0+ + skel->bss->tid =gettid ();this is not available everywhere, we just recently had a fix. Other tests call
syscall()directly. It might be time to add macro in one of the helpers headers, though. But that can be done as a separate clean up patch outside of this change (there is enough to review and discuss)
Ok, I can include a clean up patch in the next revision.
Quoted 36 lines
Quoted 22 lines
tools/testing/selftests/bpf/prog_tests/raw_tp_null.c new file +10
-0+ +if (!ASSERT_OK (raw_tp_null__attach (skel),"raw_tp_null__attach" )) + goto end; + +ASSERT_OK (trigger_module_test_read (2 ),"trigger testmod read" ); +ASSERT_EQ (skel->bss->i,3 ,"invocations" ); + +end: +raw_tp_null__destroy (skel); +}tools/testing/selftests/bpf/progs/raw_tp_null.c new file +5
-0@@ -0,0 +1,27 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */ + +#include <vmlinux.h> +#include <bpf/bpf_tracing.h> tools/testing/selftests/bpf/progs/raw_tp_null.c line 15
Quoted 10 lines
tools/testing/selftests/bpf/progs/raw_tp_null.c new file +10
-0+ +char _license[] SEC("license") = "GPL"; + +int tid; +int i; + +SEC("tp_btf/bpf_testmod_test_raw_tp_null") +int BPF_PROG (test_raw_tp_null ,struct sk_buff *skb) +{ +if (bpf_get_current_task_btf ()->pid == tid) {nit: avoid unnecessary nesting. Check condition and return early. Also seems nicer to have
task_structlocal variable for this, tbh:
Quoted 6 lines
struct
task_struct*t =bpf_get_current_task_btf();if (
t->pid!= tid)return 0;/* the rest follows, unnested */
Ack.
Quoted 16 lines
Quoted 16 lines
tools/testing/selftests/bpf/progs/raw_tp_null.c new file +12
-1+ i = i + skb->mark +1 ; + +/* If dead code elimination kicks in, the increment below will +* be removed. For raw_tp programs, we mark input arguments as +* PTR_MAYBE_NULL, so branch prediction should never kick in. +*/ +if (!skb) + i +=2 ; + } + +return 0 ; +} --2.43.5
Kumar Kartikeya Dwivedi
maintainerQuoted 24 lines · Kumar Kartikeya Dwivedi
More context is available in [0], but the TLDR; is that the verifier incorrectly assumes that any raw tracepoint argument will always be non-NULL. This means that even when users correctly check possible NULL arguments, the verifier can remove the NULL check due to incorrect knowledge of the NULL-ness of the pointer. Secondly, kernel helpers or kfuncs taking these trusted tracepoint arguments incorrectly assume that all arguments will always be valid non-NULL.
In this set, we mark
raw_tparguments asPTR_MAYBE_NULLon top ofPTR_TRUSTED, but special case their behavior when dereferencing them or pointer arithmetic over them is involved. When passing trusted args to helpers or kfuncs,raw_tpprograms are permitted to pass possibly NULL pointers in such cases.Any loads into such maybe NULL trusted
PTR_TO_BTF_IDis promoted to aPROBE_MEMload to handle emanating page faults. The verifier will ensure NULL checks on such pointers are preserved and do not lead to dead code elimination.This new behavior is not applied when
ref_obj_idis non-zero, as those pointers do not belong toraw_tparguments, but instead acquired objects.
Quoted 7 lines
Since helpers and kfuncs already require attention for
PTR_TO_BTF_ID(non-trusted) pointers, we do not implement any protection for such cases in this patch set, and leave it as future work for an upcoming series.A selftest is included with this patch set to verify the new behavior, and it crashes the kernel without the first patch.
I see that all selftests except one passed. The one that didn't appears to have been cancelled after running for an hour, and stalled after select_reuseport:OK.
Looking at the LLVM 18
(https://github.com/kernel-patches/bpf/actions/runs/11621768944/job/32366412581?pr=7999)
run instead of LLVM 17
(https://github.com/kernel-patches/bpf/actions/runs/11621768944/job/32366400714?pr=7999,
which failed), it seems the next test send_signal_tracepoint.
Is this known to be flaky? I'm guessing not and it is probably caused by my patch, but just want to confirm before I begin debugging.
Quoted 3 lines
[...]
Eduard Zingerman
maintainerOn Fri, 2024-11-01 at 14:18 +0100, Kumar Kartikeya Dwivedi wrote:
[...]
Quoted 11 lines
I see that all selftests except one passed. The one that didn't appears to have been cancelled after running for an hour, and stalled after select_reuseport:OK.
Looking at the LLVM 18
(https://github.com/kernel-patches/bpf/actions/runs/11621768944/job/32366412581?pr=7999)
run instead of LLVM 17
(https://github.com/kernel-patches/bpf/actions/runs/11621768944/job/32366400714?pr=7999,
which failed), it seems the next testsend_signal_tracepoint.Is this known to be flaky? I'm guessing not and it is probably caused by my patch, but just want to confirm before I begin debugging.
I suspect this is a test send_signal.
It started to hang for me yesterday w/o any apparent reason (on master branch). I added workaround to avoid stalls, but this does not address the issue with the test. Workaround follows.
---
tools/testing/selftests/bpf/prog_tests/send_signal.c +40 -8
@@ -18,6 +18,38 @@ static void sigusr1_siginfo_handler(int s, siginfo_t *i, void *v)static char log_buf[64 *1024 ]; +static ssize_t read_with_timeout (int fd,void *buf,size_t count) +{ +struct timeval tv = {1 ,0 }; +fd_set fds; +int err; + +FD_ZERO (&fds); +FD_SET (fd, &fds); + err =select (fd +1 , &fds,NULL ,NULL , &tv); +if (!ASSERT_GE (err,0 ,"read select" )) +return err; +if (FD_ISSET (fd, &fds)) +return read (fd, buf, count); +return -EAGAIN ; +} + +static ssize_t write_with_timeout (int fd,void *buf,size_t count) +{ +struct timeval tv = {1 ,0 }; +fd_set fds; +int err; + +FD_ZERO (&fds); +FD_SET (fd, &fds); + err =select (fd +1 ,NULL , &fds,NULL , &tv); +if (!ASSERT_GE (err,0 ,"write select" )) +return err; +if (FD_ISSET (fd, &fds)) +return write (fd, buf, count); +return -EAGAIN ; +} +static void test_send_signal_common (struct perf_event_attr *attr,bool signal_thread,bool remote) {
@@ -75,10 +107,10 @@ static void test_send_signal_common(struct perf_event_attr *attr, }/* notify parent signal handler is installed */ -ASSERT_EQ (write (pipe_c2p[1 ], buf,1 ),1 ,"pipe_write" ); +ASSERT_EQ (write_with_timeout (pipe_c2p[1 ], buf,1 ),1 ,"pipe_write" );/* make sure parent enabled bpf program to send_signal */ -ASSERT_EQ (read (pipe_p2c[0 ], buf,1 ),1 ,"pipe_read" ); +ASSERT_EQ (read_with_timeout (pipe_p2c[0 ], buf,1 ),1 ,"pipe_read" );/* wait a little for signal handler */ for (int i =0 ; i <1000000000 && !sigusr1_received; i++) {
@@ -94,10 +126,10 @@ static void test_send_signal_common(struct perf_event_attr *attr, buf[0 ] = sigusr1_received;ASSERT_EQ (sigusr1_received,8 ,"sigusr1_received" ); -ASSERT_EQ (write (pipe_c2p[1 ], buf,1 ),1 ,"pipe_write" ); +ASSERT_EQ (write_with_timeout (pipe_c2p[1 ], buf,1 ),1 ,"pipe_write" );/* wait for parent notification and exit */ -ASSERT_EQ (read (pipe_p2c[0 ], buf,1 ),1 ,"pipe_read" ); +ASSERT_EQ (read_with_timeout (pipe_p2c[0 ], buf,1 ),1 ,"pipe_read" );/* restore the old priority */ if (!remote)
@@ -158,7 +190,7 @@ static void test_send_signal_common(struct perf_event_attr *attr, }/* wait until child signal handler installed */ -ASSERT_EQ (read (pipe_c2p[0 ], buf,1 ),1 ,"pipe_read" ); +ASSERT_EQ (read_with_timeout (pipe_c2p[0 ], buf,1 ),1 ,"pipe_read" );/* trigger the bpf send_signal */ skel->bss->signal_thread = signal_thread;
@@ -172,7 +204,7 @@ static void test_send_signal_common(struct perf_event_attr *attr, }/* notify child that bpf program can send_signal now */ -ASSERT_EQ (write (pipe_p2c[1 ], buf,1 ),1 ,"pipe_write" ); +ASSERT_EQ (write_with_timeout (pipe_p2c[1 ], buf,1 ),1 ,"pipe_write" );/* For the remote test, the BPF program is triggered from this * process but the other process/thread is signaled.
@@ -188,7 +220,7 @@ static void test_send_signal_common(struct perf_event_attr *attr, }/* wait for result */ - err =read (pipe_c2p[0 ], buf,1 ); + err =read_with_timeout (pipe_c2p[0 ], buf,1 );if (!ASSERT_GE (err,0 ,"reading pipe" )) goto disable_pmu;if (!ASSERT_GT (err,0 ,"reading pipe error: size 0" )) {
@@ -199,7 +231,7 @@ static void test_send_signal_common(struct perf_event_attr *attr,ASSERT_EQ (buf[0 ],8 ,"incorrect result" );/* notify child safe to exit */ -ASSERT_EQ (write (pipe_p2c[1 ], buf,1 ),1 ,"pipe_write" ); +ASSERT_EQ (write_with_timeout (pipe_p2c[1 ], buf,1 ),1 ,"pipe_write" ); disable_pmu:close (pmu_fd);
Quoted 14 lines · Eduard Zingerman
On Fri, 2024-11-01 at 14:18 +0100, Kumar Kartikeya Dwivedi wrote:
[...]
Quoted 9 lines
I see that all selftests except one passed. The one that didn't appears to have been cancelled after running for an hour, and stalled after select_reuseport:OK.
Looking at the LLVM 18
(https://github.com/kernel-patches/bpf/actions/runs/11621768944/job/32366412581?pr=7999)
run instead of LLVM 17
(https://github.com/kernel-patches/bpf/actions/runs/11621768944/job/32366400714?pr=7999,
which failed), it seems the next testsend_signal_tracepoint.
Quoted 7 lines
Quoted 2 lines
Is this known to be flaky? I'm guessing not and it is probably caused by my patch, but just want to confirm before I begin debugging.
I suspect this is a test
send_signal.
It started to hang for me yesterday w/o any apparent reason (on master branch). I added workaround to avoid stalls, but this does not address the issue with the test. Workaround follows.
Hmm.
Puranjay touched it last with extra logic.
And before that David Vernet tried to address flakiness in commit 4a54de65964d.
Yonghong also noticed lockups in paravirt
and added workaround 7015843afc.
Your additional timeout/workaround makes sense to me, but would be good to bisect whether Puranjay's change caused it.
Quoted 108 lines
---
tools/testing/selftests/bpf/prog_tests/send_signal.c +40
-8@@ -18,6 +18,38 @@ static void sigusr1_siginfo_handler(int s, siginfo_t *i, void *v)static char log_buf[64 *1024 ]; +static ssize_t read_with_timeout (int fd,void *buf,size_t count) +{ +struct timeval tv = {1 ,0 }; +fd_set fds; +int err; + +FD_ZERO (&fds); +FD_SET (fd, &fds); + err =select (fd +1 , &fds,NULL ,NULL , &tv); +if (!ASSERT_GE (err,0 ,"read select" )) +return err; +if (FD_ISSET (fd, &fds)) +return read (fd, buf, count); +return -EAGAIN ; +} + +static ssize_t write_with_timeout (int fd,void *buf,size_t count) +{ +struct timeval tv = {1 ,0 }; +fd_set fds; +int err; + +FD_ZERO (&fds); +FD_SET (fd, &fds); + err =select (fd +1 ,NULL , &fds,NULL , &tv); +if (!ASSERT_GE (err,0 ,"write select" )) +return err; +if (FD_ISSET (fd, &fds)) +return write (fd, buf, count); +return -EAGAIN ; +} +static void test_send_signal_common (struct perf_event_attr *attr,bool signal_thread,bool remote) {@@ -75,10 +107,10 @@ static void test_send_signal_common(struct perf_event_attr *attr, }/* notify parent signal handler is installed */ -ASSERT_EQ (write (pipe_c2p[1 ], buf,1 ),1 ,"pipe_write" ); +ASSERT_EQ (write_with_timeout (pipe_c2p[1 ], buf,1 ),1 ,"pipe_write" );/* make sure parent enabled bpf program to send_signal */ -ASSERT_EQ (read (pipe_p2c[0 ], buf,1 ),1 ,"pipe_read" ); +ASSERT_EQ (read_with_timeout (pipe_p2c[0 ], buf,1 ),1 ,"pipe_read" );/* wait a little for signal handler */ for (int i =0 ; i <1000000000 && !sigusr1_received; i++) {@@ -94,10 +126,10 @@ static void test_send_signal_common(struct perf_event_attr *attr, buf[0 ] = sigusr1_received;ASSERT_EQ (sigusr1_received,8 ,"sigusr1_received" ); -ASSERT_EQ (write (pipe_c2p[1 ], buf,1 ),1 ,"pipe_write" ); +ASSERT_EQ (write_with_timeout (pipe_c2p[1 ], buf,1 ),1 ,"pipe_write" );/* wait for parent notification and exit */ -ASSERT_EQ (read (pipe_p2c[0 ], buf,1 ),1 ,"pipe_read" ); +ASSERT_EQ (read_with_timeout (pipe_p2c[0 ], buf,1 ),1 ,"pipe_read" );/* restore the old priority */ if (!remote)@@ -158,7 +190,7 @@ static void test_send_signal_common(struct perf_event_attr *attr, }/* wait until child signal handler installed */ -ASSERT_EQ (read (pipe_c2p[0 ], buf,1 ),1 ,"pipe_read" ); +ASSERT_EQ (read_with_timeout (pipe_c2p[0 ], buf,1 ),1 ,"pipe_read" );/* trigger the bpf send_signal */ skel->bss->signal_thread = signal_thread;@@ -172,7 +204,7 @@ static void test_send_signal_common(struct perf_event_attr *attr, }/* notify child that bpf program can send_signal now */ -ASSERT_EQ (write (pipe_p2c[1 ], buf,1 ),1 ,"pipe_write" ); +ASSERT_EQ (write_with_timeout (pipe_p2c[1 ], buf,1 ),1 ,"pipe_write" );/* For the remote test, the BPF program is triggered from this * process but the other process/thread is signaled. @@ -188,7 +220,7 @@ static void test_send_signal_common(struct perf_event_attr *attr, }/* wait for result */ - err =read (pipe_c2p[0 ], buf,1 ); + err =read_with_timeout (pipe_c2p[0 ], buf,1 );if (!ASSERT_GE (err,0 ,"reading pipe" )) goto disable_pmu;if (!ASSERT_GT (err,0 ,"reading pipe error: size 0" )) {@@ -199,7 +231,7 @@ static void test_send_signal_common(struct perf_event_attr *attr,ASSERT_EQ (buf[0 ],8 ,"incorrect result" );/* notify child safe to exit */ -ASSERT_EQ (write (pipe_p2c[1 ], buf,1 ),1 ,"pipe_write" ); +ASSERT_EQ (write_with_timeout (pipe_p2c[1 ], buf,1 ),1 ,"pipe_write" ); disable_pmu:close (pmu_fd);
Eduard Zingerman
maintainerOn Fri, 2024-11-01 at 17:29 -0700, Alexei Starovoitov wrote:
[...]
Quoted 10 lines
Hmm.
Puranjay touched it last with extra logic.And before that David Vernet tried to address flakiness in commit
4a54de65964d.
Yonghong also noticed lockups in paravirt
and added workaround7015843afc.Your additional timeout/workaround makes sense to me, but would be good to bisect whether Puranjay's change caused it.
I'll debug what's going on some time later today or on Sat.
Eduard Zingerman
maintainerQuoted 12 lines · Eduard Zingerman
On Fri, 2024-11-01 at 17:29 -0700, Alexei Starovoitov wrote:
[...]
Quoted 8 lines
Hmm.
Puranjay touched it last with extra logic.And before that David Vernet tried to address flakiness in commit
4a54de65964d.
Yonghong also noticed lockups in paravirt
and added workaround7015843afc.
Quoted 4 lines
Quoted 2 lines
Your additional timeout/workaround makes sense to me, but would be good to bisect whether Puranjay's change caused it.
I'll debug what's going on some time later today or on Sat.
I finally had time to investigate this a bit.
First, here is how to trigger lockup:
t1=send_signal/send_signal_perf_thread_remote; \ t2=send_signal/send_signal_nmi_thread_remote; \ for i in $(seq 1 100); do ./test_progs -t $t1,$t2; done
Must be both tests for whatever reason.
The failing test is 'send_signal_nmi_thread_remote'.
The test is organized as parent and child processes communicating various events to each other. The intended sequence of events:
- child:
- install SIGUSR1 handler
- notify parent
- wait for parent
- parent:
- open PERF_COUNT_SW_CPU_CLOCK event
- attach BPF program to the event
- notify child
- enter busy loop for 10^8 iterations
- wait for child
- BPF program:
- send SIGUSR1 to child
- child:
- poll for SIGUSR1 in a busy loop
- notify parent
- parent:
- check value communicated by child,
terminate test.
The lockup happens because on every other test run perf event is not triggered, child does not receive SIGUSR1 and thus both parent and child are stuck.
For 'send_signal_nmi_thread_remote' perf event is defined as:
struct perf_event_attr attr = {
.sample_period = 1,
.type = PERF_TYPE_HARDWARE,
.config = PERF_COUNT_HW_CPU_CYCLES,
};And is opened for parent process pid.
Apparently, the perf event is not always triggered between lines send_signal.c:165-180. And at line 180 parent enters system call, so cpu cycles stop ticking for 'parent', thus if perf event had not been triggered already it won't be triggered at all (as far as I understand).
Applying same fix as Yonghong did in 7015843afc is sufficient to reliably trigger perf event:
tools/testing/selftests/bpf/prog_tests/send_signal.c +2 -1
@@ -223,7 +223,8 @@ static void test_send_signal_perf(bool signal_thread, bool remote)static void test_send_signal_nmi (bool signal_thread,bool remote) {struct perf_event_attr attr = { - .sample_period =1 , + .freq =1 , + .sample_freq =1000 , .type =PERF_TYPE_HARDWARE , .config =PERF_COUNT_HW_CPU_CYCLES , };
But I don't understand why.
As far as I can figure from kernel source code, sample_period is measured in nanoseconds (is it?),
so busy loop at send_signal.c:174-175 should run long enough for perf event to be triggered before.
Can someone with understanding of how perf event work explain why above change helps?
Quoted 86 lines · Eduard Zingerman
Quoted 12 lines · Eduard Zingerman
On Fri, 2024-11-01 at 17:29 -0700, Alexei Starovoitov wrote:
[...]
Quoted 8 lines
Hmm.
Puranjay touched it last with extra logic.And before that David Vernet tried to address flakiness in commit
4a54de65964d.
Yonghong also noticed lockups in paravirt
and added workaround7015843afc.Quoted 4 lines
Quoted 2 lines
Your additional timeout/workaround makes sense to me, but would be good to bisect whether Puranjay's change caused it.
I'll debug what's going on some time later today or on Sat.
I finally had time to investigate this a bit.
First, here is how to trigger lockup:t1=send_signal/send_signal_perf_thread_remote; \ t2=send_signal/send_signal_nmi_thread_remote; \ for i in $(seq 1 100); do ./test_progs -t $t1,$t2; doneMust be both tests for whatever reason.
The failing test is 'send_signal_nmi_thread_remote'.The test is organized as parent and child processes communicating various events to each other. The intended sequence of events:
- child:
- install SIGUSR1 handler
- notify parent
- wait for parent
- parent:
- openPERF_COUNT_SW_CPU_CLOCKevent
- attach BPF program to the event
- notify child
- enter busy loop for 10^8 iterations
- wait for child
- BPF program:
- send SIGUSR1 to child
- child:
- poll for SIGUSR1 in a busy loop
- notify parent
- parent:
- check value communicated by child,
terminate test.The lockup happens because on every other test run perf event is not triggered, child does not receive SIGUSR1 and thus both parent and child are stuck.
For '
send_signal_nmi_thread_remote' perf event is defined as:struct perf_event_attr attr = { .sample_period = 1, .type = PERF_TYPE_HARDWARE, .config = PERF_COUNT_HW_CPU_CYCLES, };And is opened for parent process pid.
Apparently, the perf event is not always triggered between lines send_signal.c:165-180. And at line 180 parent enters system call, so cpu cycles stop ticking for 'parent', thus if perf event had not been triggered already it won't be triggered at all (as far as I understand).
Applying same fix as Yonghong did in
7015843afcis sufficient to reliably trigger perf event:tools/testing/selftests/bpf/prog_tests/send_signal.c +2
-1@@ -223,7 +223,8 @@ static void test_send_signal_perf(bool signal_thread, bool remote)static void test_send_signal_nmi (bool signal_thread,bool remote) {struct perf_event_attr attr = { - .sample_period =1 , + .freq =1 , + .sample_freq =1000 , .type =PERF_TYPE_HARDWARE , .config =PERF_COUNT_HW_CPU_CYCLES , };
Quoted 3 lines
But I don't understand why.
As far as I can figure from kernel source code,sample_periodis measured in nanoseconds (is it?),
I believe sample_period is a number of samples. 1 means that perf suppose to generate event very often. It means nanoseconds only for SW cpu_cycles.
let's apply above workaround and move on. Pls send a patch.
Yonghong Song
reviewerQuoted 84 lines · Alexei Starovoitov
Quoted 83 lines · Eduard Zingerman
Quoted 7 lines · Eduard Zingerman
On Fri, 2024-11-01 at 17:29 -0700, Alexei Starovoitov wrote:
[...]
Quoted 3 lines
Hmm.
Puranjay touched it last with extra logic.Quoted 8 lines
Quoted 7 lines
And before that David Vernet tried to address flakiness in commit
4a54de65964d.
Yonghong also noticed lockups in paravirt
and added workaround7015843afc.Your additional timeout/workaround makes sense to me, but would be good to bisect whether Puranjay's change caused it.
I'll debug what's going on some time later today or on Sat.
I finally had time to investigate this a bit.
First, here is how to trigger lockup:t1=send_signal/send_signal_perf_thread_remote; \ t2=send_signal/send_signal_nmi_thread_remote; \ for i in $(seq 1 100); do ./test_progs -t $t1,$t2; doneMust be both tests for whatever reason.
The failing test is 'send_signal_nmi_thread_remote'.The test is organized as parent and child processes communicating various events to each other. The intended sequence of events:
- child:
- install SIGUSR1 handler
- notify parent
- wait for parent
- parent:
- openPERF_COUNT_SW_CPU_CLOCKevent
- attach BPF program to the event
- notify child
- enter busy loop for 10^8 iterations
- wait for child
- BPF program:
- send SIGUSR1 to child
- child:
- poll for SIGUSR1 in a busy loop
- notify parent
- parent:
- check value communicated by child,
terminate test.The lockup happens because on every other test run perf event is not triggered, child does not receive SIGUSR1 and thus both parent and child are stuck.
For '
send_signal_nmi_thread_remote' perf event is defined as:struct perf_event_attr attr = { .sample_period = 1, .type = PERF_TYPE_HARDWARE, .config = PERF_COUNT_HW_CPU_CYCLES, };And is opened for parent process pid.
Apparently, the perf event is not always triggered between lines send_signal.c:165-180. And at line 180 parent enters system call, so cpu cycles stop ticking for 'parent', thus if perf event had not been triggered already it won't be triggered at all (as far as I understand).
Applying same fix as Yonghong did in
7015843afcis sufficient to reliably trigger perf event:tools/testing/selftests/bpf/prog_tests/send_signal.c +2
-1@@ -223,7 +223,8 @@ static void test_send_signal_perf(bool signal_thread, bool remote)static void test_send_signal_nmi (bool signal_thread,bool remote) {struct perf_event_attr attr = { - .sample_period =1 , + .freq =1 , + .sample_freq =1000 , .type =PERF_TYPE_HARDWARE , .config =PERF_COUNT_HW_CPU_CYCLES , };
Quoted 8 lines
Quoted 3 lines
But I don't understand why.
As far as I can figure from kernel source code,sample_periodis measured in nanoseconds (is it?),I believe
sample_periodis a number of samples. 1 means that perf suppose to generate event very often. It means nanoseconds only for SWcpu_cycles.let's apply above workaround and move on. Pls send a patch.
sample_period = 1 intends to make sure we can get at one sample since many samples will be generated. But too many samples may cause *rare* issues in internal kernel logic in certain *corner* cases. Agree with Alexei, let us just use a reasonable sample_freq to fix the issue. Thanks!