On Wed, Nov 15, 2023 at 9:18 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> Add an ability to specify messages that should not be found in the
> test verifier log. Similar to LLVM's FileCheck tool for the following
> test specification:
>
> __success
> __msg("a")
> __not_msg("b")
> __msg("c")
> void foo(...) { ... }
>
> - message "a" is expected to be in the log;
> - message "b" is not expected after message "a"
> (but could be present before "a");
> - message "c" is expected to be in the log after message "a".
I think this implementation has an undesired surprising behavior.
Imagine you have a log like this:
A
C
D
B
And you specify
__msg("A")
__nomsg("B")
__msg("C")
__msg("D")
__msg("B")
Log matches the spec, right? But your implementation will eagerly reject it.
I think you can implement more coherent behavior if you only strstr()
__msg() specs, skipping __nomsg() first. But on each __msg one, if
successful, you go back and validate that there are no matches for all
__nomsg() specs that you skipped, taking into account matched
positions for current __msg() and last __msg() (or the start of the
log, of course).
Not sure if I explained clearly, but the idea is to postpone __nomsg()
until we anchor ourselves between two __msg()s. And beginning/end of
verifier log are two other anchoring positions.
>
> Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> ---
> tools/testing/selftests/bpf/progs/bpf_misc.h | 9 +++
> tools/testing/selftests/bpf/test_loader.c | 82 ++++++++++++++------
> 2 files changed, 68 insertions(+), 23 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/progs/bpf_misc.h b/tools/testing/selftests/bpf/progs/bpf_misc.h
> index 799fff4995d8..f24fcda6fc0b 100644
> --- a/tools/testing/selftests/bpf/progs/bpf_misc.h
> +++ b/tools/testing/selftests/bpf/progs/bpf_misc.h
> @@ -22,7 +22,13 @@
> *
> * __msg Message expected to be found in the verifier log.
> * Multiple __msg attributes could be specified.
> + * When multiple messages are specified they are
> + * matched one after another.
> + * __not_msg Message not expected to be found in the verifier log.
> + * Matched from the end of the last checked __msg or
> + * from log start, if no __msg had been matched yet.
> * __msg_unpriv Same as __msg but for unprivileged mode.
> + * __not_msg_unpriv Same as __not_msg but for unprivileged mode.
ok, it's bikeshedding, but __nomsg and __nomsg_unpriv seems a bit
nicer names for this (very subjective, of course)
> *
> * __success Expect program load success in privileged mode.
> * __success_unpriv Expect program load success in unprivileged mode.
> @@ -59,10 +65,13 @@
> * __auxiliary_unpriv Same, but load program in unprivileged mode.
> */
> #define __msg(msg) __attribute__((btf_decl_tag("comment:test_expect_msg=" msg)))
> +#define __not_msg(msg) __attribute__((btf_decl_tag("comment:test_dont_expect_msg=" msg)))
> #define __failure __attribute__((btf_decl_tag("comment:test_expect_failure")))
> #define __success __attribute__((btf_decl_tag("comment:test_expect_success")))
> #define __description(desc) __attribute__((btf_decl_tag("comment:test_description=" desc)))
> #define __msg_unpriv(msg) __attribute__((btf_decl_tag("comment:test_expect_msg_unpriv=" msg)))
> +#define __not_msg_unpriv(msg) \
> + __attribute__((btf_decl_tag("comment:test_dont_expect_msg_unpriv=" msg)))
test_expect_no_msg ?
> #define __failure_unpriv __attribute__((btf_decl_tag("comment:test_expect_failure_unpriv")))
> #define __success_unpriv __attribute__((btf_decl_tag("comment:test_expect_success_unpriv")))
> #define __log_level(lvl) __attribute__((btf_decl_tag("comment:test_log_level="#lvl)))
> diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
> index 37ffa57f28a1..def16d9aeae2 100644
> --- a/tools/testing/selftests/bpf/test_loader.c
> +++ b/tools/testing/selftests/bpf/test_loader.c
[...]
> -static int push_msg(const char *msg, struct test_subspec *subspec)
> +static int push_msg(const char *msg, struct test_subspec *subspec, bool expected)
> {
> + size_t cnt = subspec->expect_msg_cnt;
> void *tmp;
>
> - tmp = realloc(subspec->expect_msgs, (1 + subspec->expect_msg_cnt) * sizeof(void *));
> + tmp = realloc(subspec->expect_msgs,
> + (1 + subspec->expect_msg_cnt) * sizeof(*subspec->expect_msgs));
nit: 1 + cnt ?
> if (!tmp) {
> ASSERT_FAIL("failed to realloc memory for messages\n");
> return -ENOMEM;
> }
> subspec->expect_msgs = tmp;
> - subspec->expect_msgs[subspec->expect_msg_cnt++] = msg;
> + subspec->expect_msgs[cnt].str = msg;
> + subspec->expect_msgs[cnt].expected = expected;
> + subspec->expect_msg_cnt++;
>
> return 0;
> }
[...]
